LibGUI: Add ModelClient abstract class and allow registering clients

This solves a problem where the SortingProxyModel doesn't
receive the on_update call because other code overwrote
the handler later on.

Applications/Browser/BookmarksBarWidget.cpp

@@ -88,16 +88,18 @@ BookmarksBarWidget::BookmarksBarWidget(const String& bookmarks_file, bool enable
 
 BookmarksBarWidget::~BookmarksBarWidget()
 {
+    if (m_model)
+        m_model->unregister_client(*this);
 }
 
 void BookmarksBarWidget::set_model(RefPtr<GUI::Model> model)
 {
     if (model == m_model)
         return;
+    if (m_model)
+        m_model->unregister_client(*this);
     m_model = move(model);
-    m_model->on_update = [&]() {
+    m_model->register_client(*this);
-        did_update_model();
-    };
 }
 
 void BookmarksBarWidget::resize_event(GUI::ResizeEvent& event)
@@ -106,7 +108,7 @@ void BookmarksBarWidget::resize_event(GUI::ResizeEvent& event)
     update_content_size();
 }
 
-void BookmarksBarWidget::did_update_model()
+void BookmarksBarWidget::on_model_update(unsigned)
 {
     for (auto* child : child_widgets()) {
         child->remove_from_parent();

Applications/Browser/BookmarksBarWidget.h

@@ -27,11 +27,13 @@
 #pragma once
 
 #include <LibGUI/Forward.h>
+#include <LibGUI/Model.h>
 #include <LibGUI/Widget.h>
 
 namespace Browser {
 
-class BookmarksBarWidget final : public GUI::Widget {
+class BookmarksBarWidget final : public GUI::Widget
+    , private GUI::ModelClient {
     C_OBJECT(BookmarksBarWidget)
 public:
     static BookmarksBarWidget& the();
@@ -52,7 +54,7 @@ public:
 private:
     BookmarksBarWidget(const String&, bool enabled);
 
-    virtual void did_update_model();
+    virtual void on_model_update(unsigned) override;
     virtual void resize_event(GUI::ResizeEvent&) override;
 
     void update_content_size();

Applications/FileManager/DirectoryView.cpp

@@ -95,16 +95,7 @@ DirectoryView::DirectoryView()
             on_path_change(model().root_path());
     };
 
-    //  NOTE: We're using the on_update hook on the GUI::SortingProxyModel here instead of
+    m_model->register_client(*this);
-    //        the GUI::FileSystemModel's hook. This is because GUI::SortingProxyModel has already
-    //        installed an on_update hook on the GUI::FileSystemModel internally.
-    // FIXME: This is an unfortunate design. We should come up with something better.
-    m_table_view->model()->on_update = [this] {
-        for_each_view_implementation([](auto& view) {
-            view.selection().clear();
-        });
-        update_statusbar();
-    };
 
     m_model->on_thumbnail_progress = [this](int done, int total) {
         if (on_thumbnail_progress)
@@ -169,6 +160,17 @@ DirectoryView::DirectoryView()
 
 DirectoryView::~DirectoryView()
 {
+    m_model->unregister_client(*this);
+}
+
+void DirectoryView::on_model_update(unsigned flags)
+{
+    if (flags & GUI::Model::UpdateFlag::InvalidateAllIndexes) {
+        for_each_view_implementation([](auto& view) {
+            view.selection().clear();
+        });
+    }
+    update_statusbar();
 }
 
 void DirectoryView::set_view_mode(ViewMode mode)

Applications/FileManager/DirectoryView.h

@@ -34,7 +34,8 @@
 #include <LibGUI/TableView.h>
 #include <sys/stat.h>
 
-class DirectoryView final : public GUI::StackWidget {
+class DirectoryView final : public GUI::StackWidget
+    , private GUI::ModelClient {
     C_OBJECT(DirectoryView)
 public:
     virtual ~DirectoryView() override;
@@ -94,6 +95,8 @@ private:
     DirectoryView();
     const GUI::FileSystemModel& model() const { return *m_model; }
 
+    virtual void on_model_update(unsigned) override;
+
     void handle_activation(const GUI::ModelIndex&);
 
     void set_status_message(const StringView&);

Libraries/LibGUI/FilePicker.cpp

@@ -106,11 +106,11 @@ FilePicker::FilePicker(Mode mode, const StringView& file_name, const StringView&
 #endif
     toolbar.set_has_frame(false);
 
-    auto& location_textbox = upper_container.add<TextBox>();
+    m_location_textbox = upper_container.add<TextBox>();
-    location_textbox.set_size_policy(SizePolicy::Fill, SizePolicy::Fixed);
+    m_location_textbox->set_size_policy(SizePolicy::Fill, SizePolicy::Fixed);
-    location_textbox.set_preferred_size(0, 22);
+    m_location_textbox->set_preferred_size(0, 22);
-    location_textbox.set_text(path);
+    m_location_textbox->set_text(path);
-    location_textbox.set_icon(Gfx::Bitmap::load_from_file("/res/icons/16x16/filetype-folder.png"));
+    m_location_textbox->set_icon(Gfx::Bitmap::load_from_file("/res/icons/16x16/filetype-folder.png"));
 
     m_view = vertical_container.add<MultiView>();
     m_view->set_model(SortingProxyModel::create(*m_model));
@@ -122,14 +122,10 @@ FilePicker::FilePicker(Mode mode, const StringView& file_name, const StringView&
     m_view->set_column_hidden(FileSystemModel::Column::Inode, true);
     m_view->set_column_hidden(FileSystemModel::Column::SymlinkTarget, true);
     m_model->set_root_path(path);
+    m_model->register_client(*this);
 
-    m_model->on_update = [&] {
+    m_location_textbox->on_return_pressed = [this] {
-        location_textbox.set_text(m_model->root_path());
+        m_model->set_root_path(m_location_textbox->text());
-        clear_preview();
-    };
-
-    location_textbox.on_return_pressed = [&] {
-        m_model->set_root_path(location_textbox.text());
     };
 
     auto open_parent_directory_action = Action::create("Open parent directory", { Mod_Alt, Key_Up }, Gfx::Bitmap::load_from_file("/res/icons/16x16/open-parent-directory.png"), [this](const Action&) {
@@ -270,6 +266,13 @@ FilePicker::FilePicker(Mode mode, const StringView& file_name, const StringView&
 
 FilePicker::~FilePicker()
 {
+    m_model->unregister_client(*this);
+}
+
+void FilePicker::on_model_update(unsigned)
+{
+    m_location_textbox->set_text(m_model->root_path());
+    clear_preview();
 }
 
 void FilePicker::set_preview(const LexicalPath& path)

Libraries/LibGUI/FilePicker.h

@@ -31,10 +31,11 @@
 #include <LibCore/StandardPaths.h>
 #include <LibGUI/Dialog.h>
 #include <LibGUI/Image.h>
+#include <LibGUI/Model.h>
 
 namespace GUI {
 
-class FilePicker final : public Dialog {
+class FilePicker final : public Dialog, private ModelClient {
     C_OBJECT(FilePicker)
 public:
     enum class Mode {
@@ -55,6 +56,8 @@ private:
     void clear_preview();
     void on_file_return();
 
+    virtual void on_model_update(unsigned) override;
+
     FilePicker(Mode type = Mode::Open, const StringView& file_name = "Untitled", const StringView& path = Core::StandardPaths::home_directory(), Window* parent_window = nullptr);
 
     static String ok_button_name(Mode mode)
@@ -74,6 +77,7 @@ private:
     LexicalPath m_selected_file;
 
     RefPtr<TextBox> m_filename_textbox;
+    RefPtr<TextBox> m_location_textbox;
     RefPtr<Frame> m_preview_container;
     RefPtr<Image> m_preview_image;
     RefPtr<Label> m_preview_name_label;

Libraries/LibGUI/Forward.h

@@ -61,6 +61,7 @@ class Painter;
 class ResizeCorner;
 class ScrollBar;
 class Slider;
+class SortingProxyModel;
 class SpinBox;
 class Splitter;
 class StackWidget;

Libraries/LibGUI/Model.cpp

@@ -55,8 +55,9 @@ void Model::for_each_view(Function<void(AbstractView&)> callback)
 
 void Model::did_update(unsigned flags)
 {
-    if (on_update)
+    for (auto* client : m_clients)
-        on_update();
+        client->on_model_update(flags);
+
     for_each_view([&](auto& view) {
         view.did_update_model(flags);
     });
@@ -82,4 +83,14 @@ bool Model::accepts_drag(const ModelIndex&, const StringView&)
     return false;
 }
 
+void Model::register_client(ModelClient& client)
+{
+    m_clients.set(&client);
+}
+
+void Model::unregister_client(ModelClient& client)
+{
+    m_clients.remove(&client);
+}
+
 }

Libraries/LibGUI/Model.h

@@ -44,6 +44,13 @@ enum class SortOrder {
     Descending
 };
 
+class ModelClient {
+public:
+    virtual ~ModelClient() { }
+
+    virtual void on_model_update(unsigned flags) = 0;
+};
+
 class Model : public RefCounted<Model> {
 public:
     enum UpdateFlag {
@@ -95,7 +102,8 @@ public:
     void register_view(Badge<AbstractView>, AbstractView&);
     void unregister_view(Badge<AbstractView>, AbstractView&);
 
-    Function<void()> on_update;
+    void register_client(ModelClient&);
+    void unregister_client(ModelClient&);
 
 protected:
     Model();
@@ -107,6 +115,7 @@ protected:
 
 private:
     HashTable<AbstractView*> m_views;
+    HashTable<ModelClient*> m_clients;
 };
 
 inline ModelIndex ModelIndex::parent() const

Libraries/LibGUI/ModelSelection.cpp

@@ -38,8 +38,11 @@ void ModelSelection::remove_matching(Function<bool(const ModelIndex&)> filter)
         if (filter(index))
             to_remove.append(index);
     }
-    for (auto& index : to_remove)
+    if (!to_remove.is_empty()) {
-        m_indexes.remove(index);
+        for (auto& index : to_remove)
+            m_indexes.remove(index);
+        notify_selection_changed();
+    }
 }
 
 void ModelSelection::set(const ModelIndex& index)
@@ -49,7 +52,7 @@ void ModelSelection::set(const ModelIndex& index)
         return;
     m_indexes.clear();
     m_indexes.set(index);
-    m_view.notify_selection_changed({});
+    notify_selection_changed();
 }
 
 void ModelSelection::add(const ModelIndex& index)
@@ -58,7 +61,7 @@ void ModelSelection::add(const ModelIndex& index)
     if (m_indexes.contains(index))
         return;
     m_indexes.set(index);
-    m_view.notify_selection_changed({});
+    notify_selection_changed();
 }
 
 void ModelSelection::toggle(const ModelIndex& index)
@@ -68,7 +71,7 @@ void ModelSelection::toggle(const ModelIndex& index)
         m_indexes.remove(index);
     else
         m_indexes.set(index);
-    m_view.notify_selection_changed({});
+    notify_selection_changed();
 }
 
 bool ModelSelection::remove(const ModelIndex& index)
@@ -77,7 +80,7 @@ bool ModelSelection::remove(const ModelIndex& index)
     if (!m_indexes.contains(index))
         return false;
     m_indexes.remove(index);
-    m_view.notify_selection_changed({});
+    notify_selection_changed();
     return true;
 }
 
@@ -86,7 +89,17 @@ void ModelSelection::clear()
     if (m_indexes.is_empty())
         return;
     m_indexes.clear();
-    m_view.notify_selection_changed({});
+    notify_selection_changed();
+}
+
+void ModelSelection::notify_selection_changed()
+{
+    if (!m_disable_notify) {
+        m_view.notify_selection_changed({});
+        m_notify_pending = false;
+    } else {
+        m_notify_pending = true;
+    }
 }
 
 }

Libraries/LibGUI/ModelSelection.h

@@ -26,7 +26,9 @@
 
 #pragma once
 
+#include <AK/Badge.h>
 #include <AK/HashTable.h>
+#include <AK/TemporaryChange.h>
 #include <AK/Vector.h>
 #include <LibGUI/ModelIndex.h>
 
@@ -91,9 +93,25 @@ public:
 
     void remove_matching(Function<bool(const ModelIndex&)>);
 
+    template<typename Function>
+    void change_from_model(Badge<SortingProxyModel>, Function f)
+    {
+        {
+            TemporaryChange change(m_disable_notify, true);
+            m_notify_pending = false;
+            f(*this);
+        }
+        if (m_notify_pending)
+            notify_selection_changed();
+    }
+
 private:
+    void notify_selection_changed();
+
     AbstractView& m_view;
     HashTable<ModelIndex> m_indexes;
+    bool m_disable_notify { false };
+    bool m_notify_pending { false };
 };
 
 }

Libraries/LibGUI/SortingProxyModel.cpp

@@ -24,6 +24,7 @@
  * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
+#include <AK/TemporaryChange.h>
 #include <AK/QuickSort.h>
 #include <LibGUI/AbstractView.h>
 #include <LibGUI/SortingProxyModel.h>
@@ -36,13 +37,22 @@ SortingProxyModel::SortingProxyModel(NonnullRefPtr<Model>&& target)
     : m_target(move(target))
     , m_key_column(-1)
 {
-    m_target->on_update = [this] {
+    // Since the target model already called Model::did_update we can't
-        resort();
+    // assume we will get another call. So, we need to register for further
-    };
+    // updates and just call resort() right away, otherwise requests
+    // to this model won't work because there are no indices to map
+    m_target->register_client(*this);
+    resort();
 }
 
 SortingProxyModel::~SortingProxyModel()
 {
+    m_target->unregister_client(*this);
+}
+
+void SortingProxyModel::on_model_update(unsigned flags)
+{
+    resort(flags);
 }
 
 int SortingProxyModel::row_count(const ModelIndex& index) const
@@ -100,15 +110,16 @@ void SortingProxyModel::set_key_column_and_sort_order(int column, SortOrder sort
     resort();
 }
 
-void SortingProxyModel::resort()
+void SortingProxyModel::resort(unsigned flags)
 {
+    TemporaryChange change(m_sorting, true);
     auto old_row_mappings = m_row_mappings;
     int row_count = target().row_count();
     m_row_mappings.resize(row_count);
     for (int i = 0; i < row_count; ++i)
         m_row_mappings[i] = i;
     if (m_key_column == -1) {
-        did_update(Model::UpdateFlag::DontInvalidateIndexes);
+        did_update(flags);
         return;
     }
     quick_sort(m_row_mappings, [&](auto row1, auto row2) -> bool {
@@ -123,24 +134,25 @@ void SortingProxyModel::resort()
             is_less_than = data1 < data2;
         return m_sort_order == SortOrder::Ascending ? is_less_than : !is_less_than;
     });
-    did_update(Model::UpdateFlag::DontInvalidateIndexes);
     for_each_view([&](AbstractView& view) {
-        auto& selection = view.selection();
+        view.selection().change_from_model({}, [&](ModelSelection& selection) {
-        Vector<ModelIndex> selected_indexes_in_target;
+            Vector<ModelIndex> selected_indexes_in_target;
-        selection.for_each_index([&](const ModelIndex& index) {
+            selection.for_each_index([&](const ModelIndex& index) {
-            selected_indexes_in_target.append(target().index(old_row_mappings[index.row()], index.column()));
+                selected_indexes_in_target.append(target().index(old_row_mappings[index.row()], index.column()));
-        });
+            });
 
-        selection.clear();
+            selection.clear();
-        for (auto& index : selected_indexes_in_target) {
+            for (auto& index : selected_indexes_in_target) {
-            for (size_t i = 0; i < m_row_mappings.size(); ++i) {
+                for (size_t i = 0; i < m_row_mappings.size(); ++i) {
-                if (m_row_mappings[i] == index.row()) {
+                    if (m_row_mappings[i] == index.row()) {
-                    selection.add(this->index(i, index.column()));
+                        selection.add(this->index(i, index.column()));
-                    continue;
+                        continue;
+                    }
                 }
             }
-        }
+        });
     });
+    did_update(flags);
 }
 
 bool SortingProxyModel::is_column_sortable(int column_index) const

Libraries/LibGUI/SortingProxyModel.h

@@ -30,7 +30,8 @@
 
 namespace GUI {
 
-class SortingProxyModel final : public Model {
+class SortingProxyModel final : public Model
+    , private ModelClient {
 public:
     static NonnullRefPtr<SortingProxyModel> create(NonnullRefPtr<Model>&& model) { return adopt(*new SortingProxyModel(move(model))); }
     virtual ~SortingProxyModel() override;
@@ -55,10 +56,12 @@ public:
 private:
     explicit SortingProxyModel(NonnullRefPtr<Model>&&);
 
+    virtual void on_model_update(unsigned) override;
+
     Model& target() { return *m_target; }
     const Model& target() const { return *m_target; }
 
-    void resort();
+    void resort(unsigned flags = Model::UpdateFlag::DontInvalidateIndexes);
 
     void set_sorting_case_sensitive(bool b) { m_sorting_case_sensitive = b; }
     bool is_sorting_case_sensitive() { return m_sorting_case_sensitive; }
@@ -69,6 +72,7 @@ private:
     SortOrder m_sort_order { SortOrder::Ascending };
     Role m_sort_role { Role::Sort };
     bool m_sorting_case_sensitive { false };
+    bool m_sorting { false };
 };
 
 }