Fast search #2179

leolost2605 · 2024-06-20T13:13:05Z

Moves search from a slow and synchronous search to a fast and incremental search. This can range from just feeling a bit snappier for optimal searches for the old system like "firefox" to going from a 20 second search leading to a "Not responding" popup to a near instantaneous search.

We already have a list of available packages so why go the route via multiple reassignments and list creations etc. via the appstream pool if we can just do it ourselves. Yields the same search results and greatly improves performance. Also switch to a Gtk.ListView so we can handle the great number of apps we sometimes get. I've left the synchronous search in place since it's easier to user with search provider etc.

In the videos the part where I search for flat is admittedly an extreme case since almost all apps are returned there but we can still see that the new search doesn't take longer than a second (vs. 20 - 30 or so with GNOME Software or our old search) to completely fill the list.

Old (I didn't stop typing, the UI just completely blocked):
Kooha-2024-06-20-21-10-14.webm

New:
Kooha-2024-06-20-21-18-55.webm

GNOME Software:
Kooha-2024-06-20-21-16-31.webm

danirabbit · 2024-06-20T20:37:28Z

I get a ton of new warnings on start with this branch about:

** (io.elementary.appcenter:128313): CRITICAL **: 13:36:51.569: app_center_core_package_get_is_local: assertion 'self != NULL' failed

** (io.elementary.appcenter:128313): CRITICAL **: 13:36:51.584: app_center_core_package_get_name: assertion 'self != NULL' failed

** (io.elementary.appcenter:128313): CRITICAL **: 13:36:51.584: app_center_core_package_get_summary: assertion 'self != NULL' failed

leolost2605 · 2024-06-21T11:37:53Z

Ok the warnings should be fixed now. I also added some additional null checks that shouldn't trigger if we do it right but better safe than sorry :)

danirabbit

Having a separate SearchEngine is awesome and definitely makes things more flexible. Love that!

It looks like in quite a few cases this branch doesn't use/breaks GObject-style construction so we want to make sure to maintain that

Is there a reason for making package settable across all these classes? It seems like it adds quite a lot of complexity

danirabbit · 2024-06-21T16:08:41Z

src/Core/SearchEngine.vala

+    public SearchEngine (Package[] packages, AppStream.Pool pool) {
+        var unique_packages = new Gee.HashMap<string, Package> ();
+        foreach (var package in packages) {
+            var package_component_id = package.normalized_component_id;
+            if (unique_packages.has_key (package_component_id)) {
+                if (package.origin_score > unique_packages[package_component_id].origin_score) {
+                    unique_packages[package_component_id] = package;
+                }
+            } else {
+                unique_packages[package_component_id] = package;
+            }
+        }
+
+        this.packages.splice (0, 0, unique_packages.values.to_array ());
+        this.pool = pool;
+    }


Can we make sure we're using GObject-style construction here? https://docs.vala.dev/tutorials/programming-language/main/03-00-object-oriented-programming/03-14-gobject-style-construction.html

Yeah I didn't use it here because we don't need the packages array but only the liststore with the unique packages so it seemed kinda pointless to first put the array into a property and then in construct extracting this stuff and then setting the property to null to free the memory. Alternatively we could ofc do all of this in the flatpackbackend and hand over the liststore right away 🤷

Yeah that might be the way to go here so we can stick to coding conventions

danirabbit · 2024-06-21T16:11:19Z

src/Views/AppInfoView.vala

+        action_stack = new ActionStack ();
+        action_stack.package = package;


Can we do:

Suggested change

action_stack = new ActionStack ();

action_stack.package = package;

action_stack = new ActionStack () {

package = package

};

danirabbit · 2024-06-21T16:14:36Z

src/Widgets/AppContainers/InstalledPackageRowGrid.vala

    public InstalledPackageRowGrid (AppCenterCore.Package package, Gtk.SizeGroup? action_size_group) {
-        Object (package: package);
+        this.package = package;


Oops don't break GObject-style construction here

danirabbit · 2024-06-21T16:15:01Z

src/Widgets/AppContainers/ListPackageRowGrid.vala

+        if (package != null) {
+            bind (package);
+        }
    }


Same here, breaks GObject-style construction

leolost2605 · 2024-06-21T16:33:58Z

The reason for making package settable was to enable recycling these widgets which is necessary for a performant listview with many packages. The big issue is that when using property setters and gobject style construction those will run before the construct block which means if we try to access widgets created there they are still null, which is why - to avoid duplicate code - I decided to break some gobject style construction . We could use notifies instead of using property setters to update stuff but that might increase complexity even more idk? I think in the long run it would probably be best to just port everything to listview/gridview and eventually get rid of constructing with a package alltogether.

danirabbit · 2024-06-21T19:11:41Z

@leolost2605 would it make more sense then to make a new clean search list row widget that has a settable package instead of modifying all the way up through the abstract widgets that are used elsewhere?

leolost2605 · 2024-06-22T22:39:02Z

That would lead to a lot of duplicate code though (or loss of functionality). Maybe we should first port everything to the new views and then just hook up this fast search?

leolost2605 and others added 11 commits June 20, 2024 15:00

Prototype fast search

2a8107c

Some improvements

e3337aa

Only show unique packages

526a379

Add category filtering

619cf8f

Search for multiple queries

3f9f18e

Cleanup

0f556c5

Rename to search engine

dfd0e73

Proper recycling

031edd2

Fix alert view not showing

15e2df3

Merge branch 'main' into leolost/fast-search

541edca

Add license header

9509b50

leolost2605 marked this pull request as ready for review June 20, 2024 19:33

leolost2605 requested a review from a team June 20, 2024 19:33

danirabbit added 2 commits June 20, 2024 13:27

Merge branch 'main' into leolost/fast-search

a498747

Merge branch 'main' into leolost/fast-search

9289e0d

leolost2605 added 2 commits June 21, 2024 13:28

Some null checking

bd1900b

Fix warnings

3e62db0

leolost2605 and others added 3 commits June 21, 2024 13:39

Merge branch 'main' into leolost/fast-search

566d26c

Less lambdas

6fee2e1

Merge branch 'main' into leolost/fast-search

26bdef7

danirabbit requested changes Jun 21, 2024

View reviewed changes

leolost2605 added 2 commits June 21, 2024 18:58

Fix memory leak

667045f

Better comment

11adc7d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fast search #2179

Fast search #2179

leolost2605 commented Jun 20, 2024 •

edited

Loading

danirabbit commented Jun 20, 2024 •

edited

Loading

leolost2605 commented Jun 21, 2024

danirabbit left a comment

danirabbit Jun 21, 2024

leolost2605 Jun 21, 2024 •

edited

Loading

danirabbit Jun 21, 2024

danirabbit Jun 21, 2024

danirabbit Jun 21, 2024

danirabbit Jun 21, 2024

leolost2605 commented Jun 21, 2024 •

edited

Loading

danirabbit commented Jun 21, 2024

leolost2605 commented Jun 22, 2024

		action_stack = new ActionStack ();
		action_stack.package = package;

Fast search #2179

Are you sure you want to change the base?

Fast search #2179

Conversation

leolost2605 commented Jun 20, 2024 • edited Loading

danirabbit commented Jun 20, 2024 • edited Loading

leolost2605 commented Jun 21, 2024

danirabbit left a comment

Choose a reason for hiding this comment

danirabbit Jun 21, 2024

Choose a reason for hiding this comment

leolost2605 Jun 21, 2024 • edited Loading

Choose a reason for hiding this comment

danirabbit Jun 21, 2024

Choose a reason for hiding this comment

danirabbit Jun 21, 2024

Choose a reason for hiding this comment

danirabbit Jun 21, 2024

Choose a reason for hiding this comment

danirabbit Jun 21, 2024

Choose a reason for hiding this comment

leolost2605 commented Jun 21, 2024 • edited Loading

danirabbit commented Jun 21, 2024

leolost2605 commented Jun 22, 2024

leolost2605 commented Jun 20, 2024 •

edited

Loading

danirabbit commented Jun 20, 2024 •

edited

Loading

leolost2605 Jun 21, 2024 •

edited

Loading

leolost2605 commented Jun 21, 2024 •

edited

Loading