Nav stack cleanup (#1460)

* Minor nav stack fixes
* Nav stack cleanup
* Additional cleanup, fix notepad crash
* Fix abort/cancel
* Fix for nasty focus bug
* Format

firmware/application/apps/ui_fileman.cpp

@@ -108,7 +108,9 @@ bool partner_file_prompt(
     if (partner.empty())
         return false;
 
-    nav.push_under_current<ModalMessageView>(
+    // Display the continuation UI once the current has been popped.
+    nav.set_on_pop([&nav, partner, action_name, on_partner_action] {
+        nav.display_modal(
             "Partner File",
             partner.filename().string() + "\n" + action_name + " this file too?",
             YESNO,
@@ -116,6 +118,7 @@ bool partner_file_prompt(
                 if (on_partner_action)
                     on_partner_action(partner, choice);
             });
+    });
 
     return true;
 }
@@ -189,8 +192,8 @@ FileManBaseView::FileManBaseView(
                   &text_current,
                   &button_exit});
 
-    button_exit.on_select = [this, &nav](Button&) {
+    button_exit.on_select = [this](Button&) {
-        nav.pop();
+        nav_.pop();
     };
 
     if (!sdcIsCardInserted(&SDCD1)) {
@@ -325,9 +328,9 @@ FileLoadView::FileLoadView(
         if (get_selected_entry().is_directory) {
             push_dir(get_selected_entry().path);
         } else {
-            nav_.pop();
             if (on_changed)
                 on_changed(get_selected_full_path());
+            nav_.pop();
         }
     };
 }

firmware/application/apps/ui_freqman.cpp

@@ -92,9 +92,9 @@ void FreqManBaseView::focus() {
 
     // TODO: Shouldn't be on focus.
     if (error_ == ERROR_ACCESS) {
-        nav_.display_modal("Error", "File access error", ABORT, nullptr);
+        nav_.display_modal("Error", "File access error", ABORT);
     } else if (error_ == ERROR_NOFILES) {
-        nav_.display_modal("Error", "No database files\nin /FREQMAN", ABORT, nullptr);
+        nav_.display_modal("Error", "No database files\nin /FREQMAN", ABORT);
     } else {
         options_category.focus();
     }

firmware/application/apps/ui_numbers.cpp

@@ -39,7 +39,7 @@ namespace ui {
 
 void NumbersStationView::focus() {
     if (file_error)
-        nav_.display_modal("No voices", "No valid voices found in\nthe /numbers directory.", ABORT, nullptr);
+        nav_.display_modal("No voices", "No valid voices found in\nthe /numbers directory.", ABORT);
     else
         button_exit.focus();
 }

firmware/application/apps/ui_recon.cpp

@@ -459,8 +459,7 @@ ReconView::ReconView(NavigationView& nav)
     rssi.set_focusable(true);
     rssi.set_peak(true, 500);
     rssi.on_select = [this](RSSI&) {
-        nav_.pop();
+        nav_.replace<LevelView>();
-        nav_.push<LevelView>();
     };
 
     // TODO: *BUG* Both transmitter_model and receiver_model share the same pmem setting for target_frequency.

firmware/application/apps/ui_remote.cpp

@@ -555,7 +555,6 @@ void RemoteView::open_remote() {
         save_remote();
         load_remote(std::move(path));
         refresh_ui();
-        ;
     };
 }
 

firmware/application/apps/ui_sd_wipe.cpp

@@ -44,15 +44,21 @@ void WipeSDView::focus() {
     dummy.focus();
 
     if (!confirmed) {
-        nav_.push<ModalMessageView>("Warning !", "Wipe FAT of SD card?", YESCANCEL, [this](bool choice) {
+        nav_.push<ModalMessageView>(
+            "Warning !",
+            "Wipe FAT of SD card?",
+            YESNO,
+            [this](bool choice) {
                 if (choice)
                     confirmed = true;
+                else
+                    nav_.pop(false);  // Pop w/o update so the modal will pop off the app.
             });
     } else {
         if (sdcGetInfo(&SDCD1, &block_device_info) == CH_SUCCESS) {
             thread = chThdCreateFromHeap(NULL, 2048, NORMALPRIO, WipeSDView::static_fn, this);
         } else {
-            nav_.pop();  // Just silently abort for now
+            nav_.pop();  // Just silently abort for now.
         }
     }
 }

firmware/application/apps/ui_sd_wipe.hpp

@@ -38,7 +38,7 @@ class WipeSDView : public View {
     ~WipeSDView();
     void focus() override;
 
-    std::string title() const override { return "Wipe sdcard"; };
+    std::string title() const override { return "Wipe SD Card"; };
 
    private:
     NavigationView& nav_;

firmware/application/apps/ui_spectrum_painter_image.cpp

@@ -40,17 +40,13 @@ SpectrumInputImageView::SpectrumInputImageView(NavigationView& nav) {
         auto open_view = nav.push<FileLoadView>(".bmp");
 
         constexpr auto data_directory = u"SPECTRUM";
-        if (std::filesystem::is_directory(data_directory) == false) {
+        ensure_directory(data_directory);
-            if (make_new_directory(data_directory).ok())
-                open_view->push_dir(data_directory);
-        } else
         open_view->push_dir(data_directory);
 
         open_view->on_changed = [this](std::filesystem::path new_file_path) {
             this->file = new_file_path.string();
             painted = false;
             this->set_dirty();
-
             this->on_input_avaliable();
         };
     };

firmware/application/apps/ui_sstvtx.cpp

@@ -35,7 +35,7 @@ namespace ui {
 
 void SSTVTXView::focus() {
     if (file_error)
-        nav_.display_modal("No files", "No valid bitmaps\nin /sstv directory.", ABORT, nullptr);
+        nav_.display_modal("No files", "No valid bitmaps\nin /sstv directory.", ABORT);
     else
         options_bitmaps.focus();
 }

firmware/application/apps/ui_text_editor.cpp

@@ -29,14 +29,6 @@
 using namespace portapack;
 namespace fs = std::filesystem;
 
-namespace {
-/*void log(const std::string& msg) {
-    LogFile log{};
-    log.append("LOGS/Notepad.txt");
-    log.write_entry(msg);
-}*/
-}  // namespace
-
 namespace ui {
 
 /* TextViewer *******************************************************/
@@ -413,16 +405,9 @@ TextEditorView::TextEditorView(NavigationView& nav)
     };
 
     menu.on_open() = [this]() {
-        /*show_save_prompt([this]() {
+        show_save_prompt([this]() {
             show_file_picker();
-        });*/
+        });
-        // HACK: above should work but it's faulting.
-        if (!file_dirty_) {
-            show_file_picker();
-        } else {
-            show_save_prompt(nullptr);
-            show_file_picker(false);
-        }
     };
 
     menu.on_save() = [this]() {
@@ -530,16 +515,16 @@ void TextEditorView::hide_menu(bool hidden) {
     set_dirty();
 }
 
-void TextEditorView::show_file_picker(bool immediate) {
+void TextEditorView::show_file_picker() {
-    // TODO: immediate is a hack until nav_.on_pop is fixed.
+    auto open_view = nav_.push<FileLoadView>("");
-    auto open_view = immediate ? nav_.push<FileLoadView>("") : nav_.push_under_current<FileLoadView>("");
-
-    if (open_view) {
     open_view->on_changed = [this](std::filesystem::path path) {
-            open_file(path);
+        // Can't update the UI focus while the FileLoadView is still up.
+        // Do this on a continuation instead of in on_changed.
+        nav_.set_on_pop([this, p = std::move(path)]() {
+            open_file(p);
             hide_menu();
+        });
     };
-    }
 }
 
 void TextEditorView::show_edit_line() {

firmware/application/apps/ui_text_editor.hpp

@@ -19,10 +19,6 @@
  * Boston, MA 02110-1301, USA.
  */
 
-/* TODO:
- * - Busy indicator while reading files.
- */
-
 #ifndef __UI_TEXT_EDITOR_H__
 #define __UI_TEXT_EDITOR_H__
 
@@ -238,7 +234,7 @@ class TextEditorView : public View {
     void refresh_ui();
     void update_position();
     void hide_menu(bool hidden = true);
-    void show_file_picker(bool immediate = true);
+    void show_file_picker();
     void show_edit_line();
     void show_save_prompt(std::function<void()> continuation);
 
@@ -252,8 +248,8 @@ class TextEditorView : public View {
     bool has_temp_file_{false};
 
     TextViewer viewer{
-        /* 272 = 320 - 16 (top bar) - 32 (bottom controls) */
+        /* 272 = screen_height - 16 (top bar) - 32 (bottom controls) */
-        {0, 0, 240, 272}};
+        {0, 0, screen_width, 272}};
 
     TextEditorMenu menu{};
 

firmware/application/apps/ui_view_wav.cpp

@@ -138,7 +138,9 @@ ViewWavView::ViewWavView(
     reset_controls();
     button_open.on_select = [this, &nav](Button&) {
         auto open_view = nav.push<FileLoadView>(".WAV");
-        open_view->on_changed = [this](std::filesystem::path file_path) {
+        open_view->on_changed = [this, &nav](std::filesystem::path file_path) {
+            // Can't show new dialogs in an on_changed handler, so use continuation.
+            nav.set_on_pop([this, &nav, file_path]() {
                 if (!wav_reader->open(file_path)) {
                     nav_.display_modal("Error", "Couldn't open file.");
                     return;
@@ -149,6 +151,7 @@ ViewWavView::ViewWavView(
                 }
                 load_wav(file_path);
                 field_pos_seconds.focus();
+            });
         };
     };
 

firmware/application/ui/ui_geomap.cpp

@@ -413,7 +413,7 @@ void GeoMapView::focus() {
     geopos.focus();
 
     if (!map_opened)
-        nav_.display_modal("No map", "No world_map.bin file in\n/ADSB/ directory", ABORT, nullptr);
+        nav_.display_modal("No map", "No world_map.bin file in\n/ADSB/ directory", ABORT);
 }
 
 void GeoMapView::update_position(float lat, float lon, uint16_t angle, int32_t altitude) {

firmware/application/ui/ui_receiver.cpp

@@ -273,9 +273,8 @@ FrequencyKeypadView::FrequencyKeypadView(
     };
 
     button_close.on_select = [this, &nav](Button&) {
-        if (on_changed) {
+        if (on_changed)
             on_changed(this->value());
-        }
         nav.pop();
     };
 

firmware/application/ui/ui_textentry.cpp

@@ -41,20 +41,12 @@ void text_prompt(
     uint32_t cursor_pos,
     size_t max_length,
     std::function<void(std::string&)> on_done) {
-    // if (persistent_memory::ui_config_textentry() == 0) {
     auto te_view = nav.push<AlphanumView>(str, max_length);
     te_view->set_cursor(cursor_pos);
     te_view->on_changed = [on_done](std::string& value) {
         if (on_done)
             on_done(value);
     };
-    /*} else {
-                auto te_view = nav.push<HandWriteView>(str, max_length);
-                te_view->on_changed = [on_done](std::string * value) {
-                        if (on_done)
-                                on_done(value);
-                };
-        }*/
 }
 
 /* TextEntryView ***********************************************************/

firmware/application/ui_navigation.cpp

@@ -96,6 +96,7 @@
 
 #include "core_control.hpp"
 #include "file.hpp"
+#include "file_reader.hpp"
 #include "png_writer.hpp"
 
 using portapack::receiver_model;
@@ -406,15 +407,21 @@ View* NavigationView::push_view(std::unique_ptr<View> new_view) {
     return p;
 }
 
-void NavigationView::pop() {
+void NavigationView::pop(bool trigger_update) {
-    pop(true);
+    // Don't pop off the NavView.
-}
+    if (view_stack.size() <= 1)
+        return;
 
-void NavigationView::pop_modal() {
+    auto on_pop = view_stack.back().on_pop;
-    // Pop modal view + underlying app view.
+
-    // TODO: this shouldn't be necessary.
+    free_view();
-    pop(false);
+    view_stack.pop_back();
-    pop(true);
+
+    // NB: These are executed _after_ the view has been
+    // destroyed. The old view MUST NOT be referenced in
+    // these callbacks or it will cause crashes.
+    if (trigger_update) update_view();
+    if (on_pop) on_pop();
 }
 
 void NavigationView::display_modal(
@@ -426,49 +433,33 @@ void NavigationView::display_modal(
 void NavigationView::display_modal(
     const std::string& title,
     const std::string& message,
-    const modal_t type,
+    modal_t type,
-    const std::function<void(bool)> on_choice) {
+    std::function<void(bool)> on_choice) {
-    /* If a modal view is already visible, don't display another */
+    push<ModalMessageView>(title, message, type, on_choice);
-    if (!modal_view) {
-        modal_view = push<ModalMessageView>(title, message, type, on_choice);
-    }
-}
-
-void NavigationView::pop(bool update) {
-    if (view() == modal_view) {
-        modal_view = nullptr;
-    }
-
-    // Can't pop last item from stack.
-    if (view_stack.size() > 1) {
-        auto on_pop = view_stack.back().on_pop;
-
-        free_view();
-        view_stack.pop_back();
-
-        if (update)
-            update_view();
-
-        if (on_pop) on_pop();
-    }
 }
 
 void NavigationView::free_view() {
+    // The focus_manager holds a raw pointer to the currently focused Widget.
+    // It then tries to call blur() on that instance when the focus is changed.
+    // This causes crashes if focused_widget has been deleted (as is the case
+    // when a view is popped). Calling blur() here resets the focus_manager's
+    // focus_widget pointer so focus can be called safely.
+    this->blur();
     remove_child(view());
 }
 
 void NavigationView::update_view() {
-    const auto new_view = view_stack.back().view.get();
+    const auto& top = view_stack.back();
+    auto top_view = top.view.get();
 
-    add_child(new_view);
+    add_child(top_view);
-    new_view->set_parent_rect({{0, 0}, size()});
+    top_view->set_parent_rect({{0, 0}, size()});
 
     focus();
     set_dirty();
 
-    if (on_view_changed) {
+    if (on_view_changed)
-        on_view_changed(*new_view);
+        on_view_changed(*top_view);
-    }
 }
 
 Widget* NavigationView::view() const {
@@ -476,9 +467,8 @@ Widget* NavigationView::view() const {
 }
 
 void NavigationView::focus() {
-    if (view()) {
+    if (view())
         view()->focus();
-    }
 }
 
 bool NavigationView::set_on_pop(std::function<void()> on_pop) {
@@ -753,21 +743,19 @@ ModalMessageView::ModalMessageView(
     NavigationView& nav,
     const std::string& title,
     const std::string& message,
-    const modal_t type,
+    modal_t type,
-    const std::function<void(bool)> on_choice)
+    std::function<void(bool)> on_choice)
     : title_{title},
       message_{message},
       type_{type},
       on_choice_{on_choice} {
     if (type == INFO) {
         add_child(&button_ok);
-
         button_ok.on_select = [this, &nav](Button&) {
-            if (on_choice_)
+            if (on_choice_) on_choice_(true);
-                on_choice_(true);  // Assumes handler will pop.
-            else
             nav.pop();
         };
+
     } else if (type == YESNO) {
         add_children({&button_yes,
                       &button_no});
@@ -780,50 +768,33 @@ ModalMessageView::ModalMessageView(
             if (on_choice_) on_choice_(false);
             nav.pop();
         };
-    } else if (type == YESCANCEL) {
-        add_children({&button_yes,
-                      &button_no});
 
-        button_yes.on_select = [this, &nav](Button&) {
-            if (on_choice_) on_choice_(true);
-            nav.pop();
-        };
-        button_no.on_select = [this, &nav](Button&) {
-            // if (on_choice_) on_choice_(false);
-            nav.pop_modal();
-        };
     } else {  // ABORT
         add_child(&button_ok);
 
         button_ok.on_select = [this, &nav](Button&) {
             if (on_choice_) on_choice_(true);
-            nav.pop_modal();
+            nav.pop(false);  // Pop the modal.
+            nav.pop();       // Pop the underlying view.
         };
     }
 }
 
 void ModalMessageView::paint(Painter& painter) {
-    size_t pos, i = 0, start = 0;
-
     portapack::display.drawBMP({100, 48}, modal_warning_bmp, false);
 
-    // Break on lines.
+    // Break lines.
-    while ((pos = message_.find("\n", start)) != std::string::npos) {
+    auto lines = split_string(message_, '\n');
+    for (size_t i = 0; i < lines.size(); ++i) {
         painter.draw_string(
             {1 * 8, (Coord)(120 + (i * 16))},
             style(),
-            message_.substr(start, pos - start));
+            lines[i]);
-        i++;
-        start = pos + 1;
     }
-    painter.draw_string(
-        {1 * 8, (Coord)(120 + (i * 16))},
-        style(),
-        message_.substr(start, pos));
 }
 
 void ModalMessageView::focus() {
-    if ((type_ == YESNO) || (type_ == YESCANCEL)) {
+    if ((type_ == YESNO)) {
         button_yes.focus();
     } else {
         button_ok.focus();

firmware/application/ui_navigation.hpp

@@ -51,7 +51,6 @@ namespace ui {
 enum modal_t {
     INFO = 0,
     YESNO,
-    YESCANCEL,
     ABORT
 };
 
@@ -73,15 +72,6 @@ class NavigationView : public View {
         return reinterpret_cast<T*>(push_view(std::unique_ptr<View>(new T(*this, std::forward<Args>(args)...))));
     }
 
-    // Pushes a new view under the current on the stack so the current view returns into this new one.
-    template <class T, class... Args>
-    T* push_under_current(Args&&... args) {
-        auto new_view = std::unique_ptr<View>(new T(*this, std::forward<Args>(args)...));
-        auto new_view_ptr = new_view.get();
-        view_stack.insert(view_stack.end() - 1, ViewState{std::move(new_view), {}});
-        return reinterpret_cast<T*>(new_view_ptr);
-    }
-
     template <class T, class... Args>
     T* replace(Args&&... args) {
         pop();
@@ -90,12 +80,14 @@ class NavigationView : public View {
 
     void push(View* v);
     void replace(View* v);
-
+    void pop(bool trigger_update = true);
-    void pop();
-    void pop_modal();
 
     void display_modal(const std::string& title, const std::string& message);
-    void display_modal(const std::string& title, const std::string& message, const modal_t type, const std::function<void(bool)> on_choice = nullptr);
+    void display_modal(
+        const std::string& title,
+        const std::string& message,
+        modal_t type,
+        std::function<void(bool)> on_choice = nullptr);
 
     void focus() override;
 
@@ -110,11 +102,9 @@ class NavigationView : public View {
     };
 
     std::vector<ViewState> view_stack{};
-    Widget* modal_view{nullptr};
 
     Widget* view() const;
 
-    void pop(bool update);
     void free_view();
     void update_view();
     View* push_view(std::unique_ptr<View> new_view);
@@ -362,8 +352,8 @@ class ModalMessageView : public View {
         NavigationView& nav,
         const std::string& title,
         const std::string& message,
-        const modal_t type,
+        modal_t type,
-        const std::function<void(bool)> on_choice);
+        std::function<void(bool)> on_choice);
 
     void paint(Painter& painter) override;
     void focus() override;

firmware/common/ui_focus.cpp

@@ -43,18 +43,12 @@ void FocusManager::set_focus_widget(Widget* const new_focus_widget) {
     }
 
     if (new_focus_widget) {
-        // if( !new_focus_widget->visible() ) {
+        if (!new_focus_widget->focusable())
-        // if( new_focus_widget->hidden() ) {
-        // 	// New widget is not visible. Do nothing.
-        // 	// TODO: Should this be a debug assertion?
-        // 	return;
-        // }
-        if (!new_focus_widget->focusable()) {
             return;
     }
-    }
 
     // Blur old widget.
+    // NB: This will crash if the focus_widget is a destroyed instance.
     if (focus_widget()) {
         focus_widget()->on_blur();
         focus_widget()->set_dirty();

firmware/common/ui_widget.cpp

@@ -134,7 +134,6 @@ void Widget::focus() {
 }
 
 void Widget::on_focus() {
-    // set_dirty();
 }
 
 void Widget::blur() {
@@ -142,7 +141,6 @@ void Widget::blur() {
 }
 
 void Widget::on_blur() {
-    // set_dirty();
 }
 
 bool Widget::focusable() const {