Created attachment 148855 [details] [review]
Add search.js, rebase search system on top

The high level goal is to separate the concern of searching for
things with display of those things; for example in newer mockups,
applications are displayed exactly the same as they look in the
AppWell.

Another goal was optimizing for speed; for example,
application search was pushed mostly down into C, and we avoid
lowercasing and normalizing every item over and over.

(In reply to comment #1)
> Created an attachment (id=148855) [details] [review]
> Add search.js, rebase search system on top

search.js is missing in the patch:

Error("Chained exception")@:0
("Chained exception")@gjs_throw:0
@<main>:1
'
    JS ERROR: !!!     message = 'No JS module 'search' found in search path'
Window manager warning: Log level 32: Execution of main.js threw exception: Error: No JS module 'search' found in search path

Program exited with code 01.

Created attachment 148947 [details] [review]
Add search.js, rebase search system on top

The high level goal is to separate the concern of searching for
things with display of those things; for example in newer mockups,
applications are displayed exactly the same as they look in the
AppWell.

Another goal was optimizing for speed; for example,
application search was pushed mostly down into C, and we avoid
lowercasing and normalizing every item over and over.

Created attachment 148951 [details] [review]
[StOverflowBox] Vertical box which skips painting underallocated children

Review of attachment 148951 [details] [review]:

Generally looks like it makes sense. I don't like the wholesale cut-and-paste of StBoxLayout but the two alternatives:

 A) Add this into StBoxLayout
 B) Add hooks into StBoxLayout so that we could subclass it

Don't seem better, because of the simplifying reductions in functionality you've made here. Hopefully the layout manager stuff in Clutter-1.2 will help with this, though I'm a bit worried that unless we try to implement this on top of that there will be missing pieces (it seems something like gtk_widget_set_child_visible() is needed for this.)

::: src/st/st-overflow-box.c
@@ +26,3 @@
+ *
+ * This is a "flexible" box which will paint as many actors as it can within
+ * its given allocation; its minimum height request will always be 0.

This isn't true with min-children.

@@ +29,3 @@
+ *
+ * Every child will be allocated the full width of the box, and always be
+ * given its preferred height.

Might be worth pointing out that children that aren't drawn still contribute to the minimum/preferred width of the box

@@ +45,3 @@
+                                                st_overflow_box_container_iface_init));
+
+#define BOX_LAYOUT_PRIVATE(o) \

This macro should have been renamed

@@ +68,3 @@
+static void
+st_overflow_box_add_actor (ClutterContainer *container,
+                            ClutterActor     *actor)

Consistent alignment problems for function parameters - looks like you didn't do a fixup pass after renaming

@@ +396,3 @@
+
+  min_height += priv->spacing * MAX(0, n_min_children - n_fixed - 1);
+  natural_height += priv->spacing * MAX(0, n_children - n_fixed - 1);

You lost the check on these that <n> - n_fixed is > 1 (0 doesn't matter one way or the other, but -1 will trigger warnings from Clutter, I think). And the usage of n_fixed here is wrong for the first... n_min_children doesn't contain fixed children.

@@ +436,3 @@
+
+  CLUTTER_ACTOR_CLASS (st_overflow_box_parent_class)->allocate (actor, box,
+                                                              flags);

Small indentation problem

@@ +470,3 @@
+
+      if (position + child_nat > content_box.y2)
+        break;

You need to keep iterating to get all fixed position children

@@ +493,3 @@
+  CLUTTER_ACTOR_CLASS (st_overflow_box_parent_class)->paint (actor);
+
+  for (i = 0, l = priv->children; i < priv->n_visible && l; i++, l = l->next)

I think there are problems here with both fixed-position and !visible children; you just pick the the first n_visible children, but neither fixed position nor !visible children count for n_visible. (For fixed-position children, you'll have to iterate through the entire list.)

@@ +514,3 @@
+  CLUTTER_ACTOR_CLASS (st_overflow_box_parent_class)->pick (actor, color);
+
+  for (i = 0, l = priv->children; i < priv->n_visible && l; i++, l = l->next)

Same comments as for paint()

@@ +565,3 @@
+  pspec = g_param_spec_uint ("min-children",
+                             "Min Children",
+                             "A maximum of this number of children will be added to the minimum size for this actor",

"The actor will request a minimum size large enough to include this many children" perhaps.

@@ +598,3 @@
+ * @min_children: Minimum children value
+ *
+ * Set the value of the #StOverflowBox::min-children property

It's not standard to document setters this way

@@ +681,3 @@
+ *
+ * Returns the number of children we will paint.  Only valid
+ * after the actor has been allocated.

Do you have a use case? If not, I think this is better omitted because of the funky conditions about when it is valid.

::: src/st/st-overflow-box.h
@@ +35,3 @@
+#define ST_OVERFLOW_BOX(obj) \
+  (G_TYPE_CHECK_INSTANCE_CAST ((obj), \
+  ST_TYPE_OVERFLOW_BOX, StOverflowBox))

Don't see a justification for line-wrapping here and deviating from every other GObject header file in existence

@@ +80,3 @@
+void st_overflow_box_set_min_children (StOverflowBox *self, guint min_children);
+
+void     st_overflow_box_remove_all     (StOverflowBox *box);

Obnoxious amount of vertical whitespace - would prefer to see the methods collapsed together and aligned in the normal fashion

Created attachment 149460 [details] [review]
[appDisplay] Unify Inactive/RunningWellItem, split into AppIcon, AppWellIcon

The distinction between the inactive and running was silly; just
have one class which can handle both running states.  However for
a future search patch, we do want a separation between an icon which
just has icon + name + glow, and a well icon which does the menu
integration.

Created attachment 149461 [details] [review]
Add search.js, rebase search system on top

The high level goal is to separate the concern of searching for
things with display of those things; for example in newer mockups,
applications are displayed exactly the same as they look in the
AppWell.

Another goal was optimizing for speed; for example,
application search was pushed mostly down into C, and we avoid
lowercasing and normalizing every item over and over.

(In reply to comment #5)
> 
> Don't seem better, because of the simplifying reductions in functionality
> you've made here. Hopefully the layout manager stuff in Clutter-1.2 will help
> with this, though I'm a bit worried that unless we try to implement this on top
> of that there will be missing pieces (it seems something like
> gtk_widget_set_child_visible() is needed for this.)

That makes sense; yeah I need to update myself on what's going on with 1.2.

> @@ +396,3 @@
> +
> +  min_height += priv->spacing * MAX(0, n_min_children - n_fixed - 1);
> +  natural_height += priv->spacing * MAX(0, n_children - n_fixed - 1);
> 
> You lost the check on these that <n> - n_fixed is > 1 (0 doesn't matter one way
> or the other, but -1 will trigger warnings from Clutter, I think). 

That's what the MAX(0,) fixes, no?

> @@ +565,3 @@
> +  pspec = g_param_spec_uint ("min-children",
> +                             "Min Children",
> +                             "A maximum of this number of children will be
> added to the minimum size for this actor",
> 
> "The actor will request a minimum size large enough to include this many
> children" perhaps.
> 
> @@ +598,3 @@
> + * @min_children: Minimum children value
> + *
> + * Set the value of the #StOverflowBox::min-children property
> 
> It's not standard to document setters this way

Do you see it as a problem?  It makes more sense to me than duplicating the description between the property doc and the getter/setter.

> @@ +681,3 @@
> + *
> + * Returns the number of children we will paint.  Only valid
> + * after the actor has been allocated.
> 
> Do you have a use case? If not, I think this is better omitted because of the
> funky conditions about when it is valid.

Yes; for the search area implementing keynav, we need to know how many children are visible so that we don't try to keynav into them.  Though this would likely turn out more cleanly if we had clutter_container_get_child_visible().

> ::: src/st/st-overflow-box.h
> @@ +35,3 @@
> +#define ST_OVERFLOW_BOX(obj) \
> +  (G_TYPE_CHECK_INSTANCE_CAST ((obj), \
> +  ST_TYPE_OVERFLOW_BOX, StOverflowBox))
> 
> Don't see a justification for line-wrapping here and deviating from every other
> GObject header file in existence

This is what st-box-layout.c was doing...how strongly do you feel about changing this?

Created attachment 149474 [details] [review]
[StOverflowBox] Vertical box which skips painting underallocated children

(In reply to comment #8)
> > +  min_height += priv->spacing * MAX(0, n_min_children - n_fixed - 1);
> > +  natural_height += priv->spacing * MAX(0, n_children - n_fixed - 1);
> > 
> > You lost the check on these that <n> - n_fixed is > 1 (0 doesn't matter one way
> > or the other, but -1 will trigger warnings from Clutter, I think). 
> 
> That's what the MAX(0,) fixes, no?

True, missed that. 
 
> > @@ +565,3 @@
> > +  pspec = g_param_spec_uint ("min-children",
> > +                             "Min Children",
> > +                             "A maximum of this number of children will be
> > added to the minimum size for this actor",
> > 
> > "The actor will request a minimum size large enough to include this many
> > children" perhaps.
> > 
> > @@ +598,3 @@
> > + * @min_children: Minimum children value
> > + *
> > + * Set the value of the #StOverflowBox::min-children property
> > 
> > It's not standard to document setters this way
> 
> Do you see it as a problem?  It makes more sense to me than duplicating the
> description between the property doc and the getter/setter.

It's nicer to the user not to have to chase references through the docs, and it looks better to be consistent, so doing this in one random place is a bad idea. (Also, if you want a cross-link, I think you need a single colon for a property, not a double colon.) Obviously, we aren't actually building the gtk-doc for St at the moment, so slightly academic...

> > ::: src/st/st-overflow-box.h
> > @@ +35,3 @@
> > +#define ST_OVERFLOW_BOX(obj) \
> > +  (G_TYPE_CHECK_INSTANCE_CAST ((obj), \
> > +  ST_TYPE_OVERFLOW_BOX, StOverflowBox))
> > 
> > Don't see a justification for line-wrapping here and deviating from every other
> > GObject header file in existence
> 
> This is what st-box-layout.c was doing...how strongly do you feel about
> changing this?

I think it's worth the 30 seconds to unwrap it all. It's a lot less readable wrapped, and if we start being inconsistent it's just going to grow with cut-and-paste. (st-box-layout.h we can't do much about unless we want to deviate - I fixed most of the MX header file indentation for thos when he did the NBTK => MX rename but apparently missed that bit.)

Review of attachment 149474 [details] [review]:

Some cosmetic stuff (including as noted in my other comment the GObject macros in the header and the doc comment for the setter), and a problem with stacking order with paint/pick. Otherwise looks like it fixes up the problems I noted in my last review.

::: src/st/st-overflow-box.c
@@ +27,3 @@
+ * This is a "flexible" box which will paint as many actors as it can within
+ * its given allocation; its minimum height request will be the sum of the
+ * mimimum size for the #StOverflowBox::min-children property, which is

"for as many children as specified by the ..."

@@ +49,3 @@
+
+#define OVERFLOW_BOX_LAYOUT_PRIVATE(o) \
+  (G_TYPE_INSTANCE_GET_PRIVATE ((o), ST_TYPE_OVERFLOW_BOX, StOverflowBoxPrivate))

needs to be "OVERFLOW_BOX_PRIVATE" - no "Layout"

@@ +495,3 @@
+
+static void
+st_overflow_box_internal_paint (StOverflowBox *box)

You've changed this to have two loops, but this breaks the raise/lower - things have to be painted from bottom to top for stacking order to work correctly.

@@ +583,3 @@
+  pspec = g_param_spec_uint ("min-children",
+                             "Min Children",
+                             "The actor will request a minimum size large enough to include this many ",

Missing "children" at the end

::: src/st/st-overflow-box.h
@@ +78,3 @@
+GType    st_overflow_box_get_type         (void);
+
+void     st_overflow_box_set_min_children (StOverflowBox *self,  guint min_children);

Normal alignment has the parameters one line each

Review of attachment 149460 [details] [review]:

Looks like a nice cleanup to me, couple of tiny suggestions about the code; OK to commit once you fix or decide to ignore those suggestions.

::: js/ui/appDisplay.js
@@ +459,3 @@
+            return;
+        if (!this._setWindowSelection)
+            return;

Seems like you could just call setApplicationWindowSelection(null) unconditionally and avoid the boolean member, but OK either way. Boolean member would read better to me as _windowSelectionSet, _setWindowSelection looks like a function where you've accidentally forgotten the ().

@@ +470,3 @@
+
+    _onActivate: function (event) {
+        let running = this._getRunning();

This temporary seems pointless since you use it only once

Review of attachment 149461 [details] [review]:

Generally, I like the structure a lot - it makes sense, seems nice and modular.

Lots of comments from the trivial to more substantial issues.

The main thing I see that's going to take some work is that (if I'm not mistaken) you've lost the ordering by number of search term matches. This is quite essential, since with that ordering the "OR" search basically works as well as an "AND" search, while without that ordering an "OR" search is returning essentially weird results. If I type in "word processor" I get "GNOME Word Puzzle" before "Openoffice.org Writer"

::: data/theme/gnome-shell.css
@@ +252,3 @@
+.dash-recent-docs-item-deleted {
+    text-decoration: line-through;
+}

Pretty sure you don't use this any more.

::: js/ui/appDisplay.js
@@ +137,3 @@
         } else {
             // Loop over the toplevel menu items, load the set of desktop file ids
             // associated with each one.

This comment is no longer valid

@@ +232,3 @@
+        let app = this._appSys.get_app(resultId);
+        if (!app)
+            return null;

Defensiveness doesn't seem useful; you don't check for unknown ids being passed in in other spots.

@@ +235,3 @@
+        return {'id': resultId,
+                 'name': app.get_name(),
+                 'texture': app.create_icon_texture(Search.RESULT_ICON_SIZE)};

Off by one in indentation (should have space before 'id')

@@ +288,3 @@
+
+    expandSearch: function(terms) {
+        let proc = new Shell.Process({ args: ['gnome-control-center'] });

Shell.Process() doesn't provide startup notification as far as I can see - should try to do this via launching the desktop file.

::: js/ui/dash.js
@@ +167,1 @@
 // Utility function shared between ResultPane and the DocDisplay in the main dash.

Random blank line insertion

@@ +355,3 @@
+        let content = provider.createResultActor(metaInfo, terms);
+        if (content == null) {
+            content = new St.BoxLayout({ style_class: 'dash-search-result-content' });

If it's expected that the content has this style class, then you should document it in the search provider docs.

@@ +407,3 @@
+        let nVisible = this.actor.get_n_visible();
+        let children = this.actor.get_children();
+        let delegate;

Seems clearer to just write:

 prevActor._delegate.setSelected(false);

than to have a delegate variable that is used for two different things

@@ +440,3 @@
+        this.actor.add(this._startingSearch);
+        this._selectedProvider = -1;
+        this._currentSearchResults = [];

You don't use this member

@@ +460,3 @@
+            titleBox.add(count);
+
+            providerBox.count = count;

don't like this add-hoc expando property, it's just asking for trouble when St.Box() grows a read-only "count" property that is the number of children. Would suggest making provider an array of { actor:, countActor:, resultDisplay:, provider: } JS objects.

@@ +464,3 @@
+            let resultDisplayBin = new St.Bin({ style_class: 'dash-search-section-results',
+                                                 x_fill: true,
+                                                 y_fill: true });

Off indentation

@@ +486,3 @@
+        for (let i = 0; i < children.length; i++) {
+            let child = children[i];
+            child.resultDisplay.actor.get_children().forEach(function (actor) { actor.destroy(); });

This is going to play havoc with custom resultDisplay implementations that aren't just a simple list - would suggest moving this into the SearchResultDisplay() "interface"

@@ +488,3 @@
+            child.resultDisplay.actor.get_children().forEach(function (actor) { actor.destroy(); });
+            child.hide();
+            child.resultDisplay.selectionIndex = -1;

Again, you are breaking the abstraction barrier here. (this could go into that same interface method, probably)

@@ +500,3 @@
+    startingSearch: function() {
+        this.reset();
+        this._startingSearch.show();

You can't, in my opinion, reasonably have a method called this.startingSearch(), and an actor called this._startingSearch!

@@ +514,3 @@
+        let results = this._searchSystem.updateSearch(searchString);
+        if (results == null)
+            return false;

You don't seem to use the return value at all. It also seems if this *could* happen then that would be breakage (A<delete> would leave results for A in place) - it's only OK to just return out here because the wrapper code avoids this ever happening by doing its own check for an empty search string.

@@ +552,3 @@
+
+    selectUp: function() {
+        for (let i = this._selectedProvider; i >= 0; i--) {

As I read this code, your off-the-end behavior is to just have the selection vanish. Previously code wrapped, which I believe is correct.

@@ +589,3 @@
+        let resultDisplay = providerActor.resultDisplay;
+        let children = resultDisplay.actor.get_children();
+        let targetActor = children[resultDisplay.selectionIndex];

More encapsulation breakage for SearchResultDisplay

::: js/ui/docDisplay.js
@@ +523,3 @@
+        let docInfo = this._docManager.lookupByUri(resultId);
+        if (!docInfo)
+            return null;

Defensiveness doesn't seem useful

@@ +526,3 @@
+        return {'id': resultId,
+                 'name': docInfo.name,
+                 'texture': docInfo.createIcon(Search.RESULT_ICON_SIZE)};

Off by one in indentation

::: js/ui/placeDisplay.js
@@ +332,3 @@
+        let colonIdx = id.indexOf(':');
+        if (colonIdx < 0)
+            return null;

You are being defensive against broken code that needs to be fixed; since this can't happen, if you are writing code to handle it, should add a log(). Or not write the code and assume that we'll debug id.substring(0, -1) when it gets hit.

@@ +342,3 @@
+            sourceArray = this._bookmarks;
+        else
+            return null;

Same here

@@ +479,3 @@
+        let placeInfo = Main.placesManager.lookupPlaceById(resultId);
+        if (!placeInfo)
+            return null;

Defensiveness doesn't seem useful

@@ +482,3 @@
+        return {'id': resultId,
+                 'name': placeInfo.name,
+                 'texture': placeInfo.iconFactory(Search.RESULT_ICON_SIZE) };

Off by one in indentation

@@ +503,2 @@
         }
+        return prefixResults.concat(substringResults);

There's a change here compared to the old code that here things are ordered as they are displayed (left column then right column) while previously search results were sorted alphabetically. I suspect alphabetically will make more sense to the user.

::: js/ui/search.js
@@ +38,3 @@
+     */
+    renderResults: function(results, terms) {
+    },

Seems like you should be consistent with searchProvider in doing 'throw new Error("not implemented");' for the stubs

@@ +89,3 @@
+     * is expected to be a substring match on the metadata for a given
+     * item, though results should be ordered such that likely matches
+     * (i.e. initial prefix) are before non-prefix matches.

Hard to understand. I'd say:

 "The results should contain both prefix matches and substring matches at arbitrary positions, with the prefix matches first in the results"

"metadata of the given item" is a little obscure in this context. "maybe say "user-visible string properties of the item such as name and description"

In the requirements here and the implementation of the various search providers, you've lost an important component of the old code, which was the sorting by match count - this is what makes the OR search tolerable; without sorting the multiple matches first, OR is pretty much meaningless.

@@ +92,3 @@
+     *
+     * This function should be fast; do not perform unbounded synchronous
+     * activities like full-text search or network queries.

full-text search doesn't have to be slow if there are indices. You could just say "expensive full-text search" to avoid such nit-picks :-)

@@ +119,3 @@
+     * @id: Result identifier string
+     *
+     * Return an object with 'id', 'name', and 'texture' properties

If you mean 'icon', think 'icon' would be more descriptive than 'texture'. 'texture' is like 'string' or 'int' - it's a data type without semantics.

@@ +146,3 @@
+     * Search providers may optionally override this to render a
+     * particular serch result in a custom fashion.  The default
+     * implementation will show the icon next to the name.

See, you say "icon" here not "texture" :-)

@@ +207,3 @@
+            return null;
+
+        let terms = searchString.split(/\s+/).map(String.toLowerCase);

Hmm. Say, hypothetically, that the JS toLowerCase doesn't handle tr_TR properly. In tr_TR casefold(I) = ı and casefold(İ) = i. But JS the toLowercase might think that they both lower-case to i. (This is what GLib will do in en_US.) By lowercasing here you may have damaged the ability of the provider code to do something more sophisticated.

My suggestion here is that you determine initial vs. subsequent by whether the *unmodified*" search terms are a set of extensions to the previous *unmodified* search terms - this should be fully conservative - casefolding and normalizing can't change non-search-term to search term. And in general every update of the search entry should be an unmodified extension or a non-extension - it is unlikely to be an extension only after casefolding and normalization. And then if the provide wants to do a simplistic JS toLowerCase() they can do it themselves, or they can, like your appSystem do the full correct thing.

The other alternative would be to require search providers to take casefolded NFKD input and use the GLib functions here to do the full casefolding and normalization upfront.

::: src/shell-app-system.c
@@ +318,3 @@
       /* the name is owned by the info itself */
       g_hash_table_insert (self->priv->app_id_to_info, (char*)shell_app_info_get_id (info),
                            info);

Since you aren't guarding against duplicate entries with the same id between apps and settings, you need to use g_hash_table_replace() instead of insert(); when rereading, you unref the entries from the lists before removing them from the table, so insert() could leave a freed key pointing to a different 'info'.

(I'm not sure if GMenu uniquifies for the same ID internally but in any case it's defensive to use replace() here.)

@@ +325,3 @@
 reread_menus (ShellAppSystem *self)
 {
+  GHashTable *uniquify = g_hash_table_new (g_str_hash, g_str_equal);

You use 'uniquify' here and 'unique' above - pick one or the other.

@@ +590,3 @@
+  MATCH_PREFIX,
+  MATCH_SUBSTRING
+} ShellAppInfoSearchMatch;

Since PREFIX/SUBSTRING are somewhat unexpected (match at the start of description is substring) you should document them briefly in a comment.

@@ +600,3 @@
+
+  if (info->type != SHELL_APP_INFO_TYPE_ENTRY)
+    return;

If anything other than TYPE_ENTRY got here with the current code, than that would represent some bad breakage in the code. On the other hand, if you did reuse this code, to say, search open windows, then you'd want to know that the code wasn't written to handle it as soon as possible rather than wondering why some windows didn't match. I think a g_assert() would be more appropriate than the silent return.

@@ +607,3 @@
+      normalized = g_utf8_normalize (name, -1, G_NORMALIZE_ALL);
+      info->casefolded_name = g_utf8_casefold (normalized, -1);
+      g_free (normalized);

Since you have this in three places, maybe a quick normalize_and_casefold() helper?

@@ +632,3 @@
+    shell_app_info_init_search_data (info);
+  if (G_UNLIKELY(!info->casefolded_name))
+    return MATCH_NONE;

If we want to blanket reject apps without a name, then it seems we should do it upfront, and not return them as part of the flattened list. Otherwise, I'd expect this code to treat NULL name and NULL description symmetrically.

@@ +666,3 @@
+
+  return strcmp (gmenu_tree_entry_get_name ((GMenuTreeEntry*)info_a->entry),
+                  gmenu_tree_entry_get_name ((GMenuTreeEntry*)info_b->entry));

If we are casefolding and normalizing for the search, certainly we should use g_utf8_collate() here and not sort GIMP before Gajim. (Conceivably use g_utf8_collate_key() - dropping collation work from O(n log n) to O(n) might be significant for ~500 entries when starting searching.)

@@ +740,3 @@
+ * Search through applications for the given search terms.
+ *
+ * Returns: (transfer container) (element-type utf8): List of application identifiers

I think you need to transfer full here (and for subsearch) - it's not that unlikely that we reread the menu tree while someone is typing in a search. Kaboom! Or if you want to optimize based on the fact that gjs will copy anyways, document that the returned strings are only valid until you return to the main loop and you must copy them if you want to hold on to them.

@@ +754,3 @@
+ * shell_app_system_subsearch:
+ * @self: A #ShellAppSystem
+ * @prefs: %TRUE iff we should search preferences instead of apps

The comment below about prefs being ignored should go up here, with an explicit warning that prefs has to be the same as for the previous search.

@@ +758,3 @@
+ * @terms: (element-type utf8): List of terms, logical OR
+ *
+ * Search through a previous result set.

You need to be a bit more explanative here; this isn't really interpretable. Perhaps something like:

 Refine a previous search against a more restrictive set of search term;
 in order for this to work correctly there must be the same number of search
 terms as previously, and each new search term must contain an old search
 term as a prefix.

@@ +763,3 @@
+ */
+GSList *
+shell_app_system_subsearch (ShellAppSystem   *system,

Elsewhere in the code you use something more like:

 system_initial_search()
 system_continue_search()

Consistent naming makes it easier to follow the code.

@@ +797,3 @@
+  g_slist_free (normalized_terms);
+
+  return sort_and_concat_results (system, prefix_results, substring_results);

There's a slight logic leap here that would be good to document:

 /* Note that a shorter term might have matched as a prefix, but
    when extended only as a substring, so we have to redo the 
    sort rather than reusing the existing ordering */

(In reply to comment #13)
> Review of attachment 149461 [details] [review]:
> 
> Generally, I like the structure a lot - it makes sense, seems nice and modular.
> 
> Lots of comments from the trivial to more substantial issues.
> 
> The main thing I see that's going to take some work is that (if I'm not
> mistaken) you've lost the ordering by number of search term matches. This is
> quite essential, since with that ordering the "OR" search basically works as
> well as an "AND" search, while without that ordering an "OR" search is
> returning essentially weird results. If I type in "word processor" I get "GNOME
> Word Puzzle" before "Openoffice.org Writer"

Fixed.

> +        let app = this._appSys.get_app(resultId);
> +        if (!app)
> +            return null;
> 
> Defensiveness doesn't seem useful; you don't check for unknown ids being passed
> in in other spots.

Well...this is tricky, because in theory the app could have gotten uninstalled during a search.  Probably what we really want to do here in the shell is mark these apps as "deleted", and flush deleted apps at some point later.

> @@ +552,3 @@
> +
> +    selectUp: function() {
> +        for (let i = this._selectedProvider; i >= 0; i--) {
> 
> As I read this code, your off-the-end behavior is to just have the selection
> vanish. Previously code wrapped, which I believe is correct.

Fixed.

> ::: js/ui/docDisplay.js
> @@ +523,3 @@
> +        let docInfo = this._docManager.lookupByUri(resultId);
> +        if (!docInfo)
> +            return null;
> 
> Defensiveness doesn't seem useful

Same deal as with apps.
  
> @@ +479,3 @@
> +        let placeInfo = Main.placesManager.lookupPlaceById(resultId);
> +        if (!placeInfo)
> +            return null;
> 
> Defensiveness doesn't seem useful

Likewise a USB device could disappear...


> My suggestion here is that you determine initial vs. subsequent by whether the
> *unmodified*" search terms are a set of extensions to the previous *unmodified*
> search terms - this should be fully conservative - casefolding and normalizing
> can't change non-search-term to search term. And in general every update of the
> search entry should be an unmodified extension or a non-extension - it is
> unlikely to be an extension only after casefolding and normalization. And then
> if the provide wants to do a simplistic JS toLowerCase() they can do it
> themselves, or they can, like your appSystem do the full correct thing.

Good point.  Done.

> @@ +666,3 @@
> +
> +  return strcmp (gmenu_tree_entry_get_name ((GMenuTreeEntry*)info_a->entry),
> +                  gmenu_tree_entry_get_name ((GMenuTreeEntry*)info_b->entry));
> 
> If we are casefolding and normalizing for the search, certainly we should use
> g_utf8_collate() here and not sort GIMP before Gajim. (Conceivably use
> g_utf8_collate_key() - dropping collation work from O(n log n) to O(n) might be
> significant for ~500 entries when starting searching.)

Done.

> 
> @@ +740,3 @@
> + * Search through applications for the given search terms.
> + *
> + * Returns: (transfer container) (element-type utf8): List of application
> identifiers
> 
> I think you need to transfer full here (and for subsearch) - it's not that
> unlikely that we reread the menu tree while someone is typing in a search.
> Kaboom! Or if you want to optimize based on the fact that gjs will copy
> anyways, document that the returned strings are only valid until you return to
> the main loop and you must copy them if you want to hold on to them.

Yeah, I was optimizing based on that fact.  Hmm...I wonder if it could be an explicit transfer flag.  Anyways I documented for now.

> The comment below about prefs being ignored should go up here, with an explicit
> warning that prefs has to be the same as for the previous search.

I documented this.

> 
> @@ +758,3 @@
> + * @terms: (element-type utf8): List of terms, logical OR
> + *
> + * Search through a previous result set.
> 
> You need to be a bit more explanative here; this isn't really interpretable.
> Perhaps something like:
> 
>  Refine a previous search against a more restrictive set of search term;
>  in order for this to work correctly there must be the same number of search
>  terms as previously, and each new search term must contain an old search
>  term as a prefix.

I documented a reference to js/ui/search.js; more ideally the search would be a GInterface that both the GObject and js objects could implement.
 the code.

(Unquoted comments fixed too)

Attachment 149460 [details] pushed as f5f92b2 - [appDisplay] Unify Inactive/RunningWellItem, split into AppIcon, AppWellIcon
Attachment 149461 [details] pushed as b7646d1 - Add search.js, rebase search system on top
Attachment 149474 [details] pushed as 14df7cd - [StOverflowBox] Vertical box which skips painting underallocated children