Created attachment 300398 [details] [review]
WIP patch for highlight matches.

I gave it a go, but I haven't tried the patch in practice at all. I would like to know if I am in the right direction, though?

(In reply to Bastian from comment #1)
> I would like to know if I am in the right direction, though?

I'm afraid not. Pango is pretty bad when it comes to introspection, just look our for "introspectable="0"" in $prefix/share/gir-1.0/Pango-1.0.gir and weep ...
I suspect your best bet is to use markup:
  label.label = preMatch + '<b>' + match + '</b>' + postMatch;

On a high-level, I'm not quite convinced that changing the label from the outside is the best approach, something along those lines might be a bit nicer:


_filterRows: function(row) {
    row.setFilter(this._filter);
    return row.shouldShow(); // !row.isFiltered()?
}

Then Row.setFilter can update the highlight as necessary.

Created attachment 300495 [details] [review]
patch to highlight matches in user list search (wip)

updated the patch, thanks for the feedback!

atm polari cannot be compiled with the patch, though. receiving a JS ERROR: missing } after property list @ resource:///org/gnome/Polari/js/userList.js:327

(In reply to Bastian from comment #3)
> atm polari cannot be compiled with the patch, though. receiving a JS ERROR:
> missing } after property list @
> resource:///org/gnome/Polari/js/userList.js:327

The error message is a bit obscure, the issue is actually a missing ',' on line 325 (and 334 and 513) ...

Created attachment 300502 [details] [review]
patch to highlight matches in user list search (wip)

Thanks. Here's another iteration. Getting a:
"JS ERROR: TypeError: row.setFilter is not a function" (Line 506).

My initial guess would be that i have failed to make row.setFilter() public somehow. Looking at _userList.setFilter() I cannot quite see the difference, though.

Review of attachment 300502 [details] [review]:

::: src/userList.js
@@ +503,3 @@
     },
 
     _filterRows: function(row) {

I can get you something nicer tomorrow, for now, you can change this to:

_filterRows: function(rowWidget) {
    let row = this._rows[rowWidget.user];
     ... // what you have now
}

Created attachment 300598 [details] [review]
WIP patch for highlight matches.

I am in a bit of a mystery here.
In master: https://git.gnome.org/browse/polari/tree/src/userList.js
at line 412 in setFilter(filter) this._filter is declared and set to filter.toLowerCase().

then somehow this._filter is accessed from _filterRows() at line 495. Isn't it out of scope?

My patch is also trying to access this._filter from _filterRows() at line 495 and tries to pass its content to row.setFilter() and row.shouldShow(). However, I'm getting errors that "filter is undefined at line 330".

I'm thinking of calling row.setFilter() and row.shouldShow() from line 105 in _updateFilter() instead hmm..

(In reply to Bastian from comment #7)
> In master: https://git.gnome.org/browse/polari/tree/src/userList.js
> at line 412 in setFilter(filter) this._filter is declared and set to
> filter.toLowerCase().
> 
> then somehow this._filter is accessed from _filterRows() at line 495. Isn't
> it out of scope?

No. filter in not a local variable, but a property of the UserList object (this), so it is available to all other methods of the UserList object as well.


> My patch is also trying to access this._filter from _filterRows() at line
> 495 and tries to pass its content to row.setFilter() and row.shouldShow().

That should be fine.


> However, I'm getting errors that "filter is undefined at line 330".

Are you sure? You are using this._filter in UserListRow.setFilter() and UserListRow.shouldShow(), which is indeed undefined (because "this" is not the UserList object which has the _filter property in that case but UserListRow). It's not line 330 though, at least according to splinter ...

Created attachment 300627 [details] [review]
WIP patch for highlight matches.

getting closer!

this patch fixes the aforementioned issue by initializing filter to '' in _init. It handles cases where the filter is written, then deleted. and cases where a letter not available in any nickname is entered.

one problem: <b></b> is not parsed as markup by GTK. I can see that it was handled in line 228 in userList.js without any escaping for the GtkLabel '_lastHeader' in UserListDetails class.

I tried some escaping with 'Glib.markup.escape.text()'[1] and setting the markup for this._label.label using 'this._label.set_markup()'[2] but so far with no success either. Hmm...

[1] https://people.gnome.org/~gcampagna/docs/GLib-2.0/GLib.markup_escape_text.html
[2] https://people.gnome.org/~gcampagna/docs/Gtk-3.0/Gtk.Label.set_markup.html

By default, labels don't use markup, you'll have to turn that on explicitly[0].

[0] https://developer.gnome.org/gtk3/stable/GtkLabel.html#GtkLabel--use-markup

Created attachment 300671 [details] [review]
Highlight matches in user list search (ready for review)

This patch should be ready to review.

I am using <b></b> as florian wrote in the reply above, but I can see that gedit is using <span weight =\"heavy\" color =\"black\"></span> for their implementation. [1]

Writing text in the search entry which does not correspond to any nickname, will make polari spit out a Gjs-WARNING **: JS ERROR: TypeError: topRow is null
@resource:///org/gnome/Polari/js/userList.js:423. As far as I can see, this error stems from code in master and is not caused by this patch, though.

[1] https://git.gnome.org/browse/gedit/commit/?id=b34a0698d5b04d1a85491f3c2996b1039cc9f52d

(In reply to Bastian from comment #11)
> I am using <b></b> as florian wrote in the reply above, but I can see that
> gedit is using <span weight =\"heavy\" color =\"black\"></span> for their
> implementation. [1]

Hardcoding a color will bring up the Why-u-break-dark-theme pitchfork-swinging crowd, other than that I don't care about the particular markup used.


> Writing text in the search entry which does not correspond to any nickname,
> will make polari spit out a Gjs-WARNING **: JS ERROR: TypeError: topRow is
> null
> @resource:///org/gnome/Polari/js/userList.js:423. As far as I can see, this
> error stems from code in master and is not caused by this patch, though.

Indeed: https://git.gnome.org/browse/polari/commit/?id=c75e8c38d88

Review of attachment 300671 [details] [review]:

Yay!

Apart from the code comments outlined below, please be a bit more verbose in the commit message (see http://lists.cairographics.org/archives/cairo/2008-September/015092.html), it should follow this syntax:

userList: Highlight matches

Short rationale of the change, can be just a short sentence in this case.

https://bugzilla.gnome.org/show_bug.cgi?id=745743

(git-bz is a very handy tool for attaching/updating patches in bugzilla, and will also make sure to add the correct bug URL to the commit message - this and more are outlined on https://wiki.gnome.org/Git/WorkingWithPatches)

::: src/userList.js
@@ +300,3 @@
                                  halign: Gtk.Align.START,
                                  hexpand: true,
+                                 ellipsize: Pango.EllipsizeMode.END });

Indentation

@@ +301,3 @@
                                  hexpand: true,
+                                 ellipsize: Pango.EllipsizeMode.END });
+        this._label.set_use_markup(true);

Please add "use_markup: true" to the property list above

@@ +326,3 @@
+    },
+
+    setFilter: function(filter) {

This works, but is not quite what I had in mind - I had imagined setFilter() to be similar to UserList.setFilter(), but then updating the highlighted string rather than invalidating the list filter. With your code, the name doesn't really make sense and would be better as something like highlightString(), however I still think something like this would be better:

setFilter: function(filter) {
    this._filter = filter.toLowerCase();
    this._updateLabel(); // that's basically your setFilter() code
}

Reasons for this are:
 - it keeps track of state information that is needed to update the label even when
   not filtering, for instance to handle alias changes
 - make less assumptions between UserList and UserListRow, for instance currently
   the latter assumes that the former has lower-cased the filter for case insensitive
   matches - generally I'd like to see the matching code in one place, not split between
   two

@@ +328,3 @@
+    setFilter: function(filter) {
+        if (filter == '') {
+          this._label.label = this.widget.user.alias;

Style nit: indentation should be four spaces, not two.

@@ +330,3 @@
+          this._label.label = this.widget.user.alias;
+        } else {
+          let filterIndex = this.widget.user.alias.toLowerCase().indexOf(filter);

While you are making sure that setFilter() is only called when there's a match, I don't like that approach. A safer variant could use something like:

  let filterIndex = -1;
  if (filter)
      filterIndex = this.widget.user.alias.toLowerCase().indexOf(filter);

  if (filterIndex < 0) {
      this._label.label = this.widget.user.alias;
  } else {
      ...
  }

@@ +335,3 @@
+          let postMatch = this.widget.user.alias.substring(filterIndex+filter.length);
+          this._label.label = preMatch + '<b>' + theMatch + '</b>' + postMatch;
+                                 

trailing whitespace

@@ +372,3 @@
         this._list.connect('size-allocate',
                            Lang.bind(this, this._updateContentHeight));
+        

trailing whitespace

Created attachment 300704 [details] [review]
userList: Higlight matches

A convenience so its easy to spot letters which match your query.

(In reply to Florian Müllner from comment #13)
> ::: src/userList.js
> @@ +300,3 @@
>                                   halign: Gtk.Align.START,
>                                   hexpand: true,
> +                                 ellipsize: Pango.EllipsizeMode.END });
> 
> Indentation

How do I go about fixing this? All I see are spaces which seem to match fine to me o.O

(In reply to Bastian from comment #15)
> How do I go about fixing this? All I see are spaces which seem to match fine
> to me o.O

You changed:

let foo = new Foo({ bar: 'bar',
                    baz: 'baz' });

to

this._fooStuff = new Foo({ bar: 'bar',
                    baz: 'baz' });

So as the first line got longer, the properties on consecutive lines no longer align ...

Created attachment 300705 [details] [review]
userList: Higlight matches

A convenience so its easy to spot letters which match your query.

Review of attachment 300704 [details] [review]:

Looks mostly good to me now, thanks.

Some minor nits still:

::: src/userList.js
@@ +323,3 @@
 
+    shouldShow: function(filter) {
+        return this.widget.user.alias.toLowerCase().indexOf(filter) != -1;

Please use this._filter here, so all assumptions about using lower-case for matching are actually in UserListRow

@@ +327,3 @@
+
+    setFilter: function(filter) {
+        this._filter = filter.toLowerCase();

Now with filtering moved from UserList to UserListRow, you can pass in the unmodified search string here (means: UserList.setFilter() no longer needs to lower-case the filter, as that's now in UserListRow)

@@ +340,3 @@
+        } else {
+            let preMatch = this.widget.user.alias.substring(0, filterIndex);
+            let theMatch = this.widget.user.alias.substring(filterIndex, filterIndex+this._filter.length);

Nit: spaces around '+'

@@ +341,3 @@
+            let preMatch = this.widget.user.alias.substring(0, filterIndex);
+            let theMatch = this.widget.user.alias.substring(filterIndex, filterIndex+this._filter.length);
+            let postMatch = this.widget.user.alias.substring(filterIndex+this._filter.length);

Dto.

@@ +521,3 @@
+        let shouldShow = row.shouldShow(this._filter);
+        if (shouldShow)
+            row.setFilter(this._filter);

This doesn't need to be conditional, use
   row.setFilter(this._filter);
   return row.shouldShow();

Review of attachment 300705 [details] [review]:

Meh, you're too fast - comments from the other review still apply :-)

Created attachment 300706 [details] [review]
userList: Higlight matches

A convenience so its easy to spot letters which match your query.

Review of attachment 300706 [details] [review]:

(In reply to Florian Müllner from comment #18)
> +    setFilter: function(filter) {
> +        this._filter = filter.toLowerCase();
> 
> Now with filtering moved from UserList to UserListRow, you can pass in the
> unmodified search string here (means: UserList.setFilter() no longer needs
> to lower-case the filter, as that's now in UserListRow)

Can you do this as well? Good to push otherwise!

Created attachment 300707 [details] [review]
userList: Higlight matches

A convenience so its easy to spot letters which match your query.

(In reply to Florian Müllner from comment #21)
> Review of attachment 300706 [details] [review] [review]:
> 
> (In reply to Florian Müllner from comment #18)
> > +    setFilter: function(filter) {
> > +        this._filter = filter.toLowerCase();
> > 
> > Now with filtering moved from UserList to UserListRow, you can pass in the
> > unmodified search string here (means: UserList.setFilter() no longer needs
> > to lower-case the filter, as that's now in UserListRow)
> 
> Can you do this as well? Good to push otherwise!

I bet you have no idea how much having two functions named 'setFilter' has confused me.. :-)

patch is pushed 
https://git.gnome.org/browse/polari/commit/?id=deacb4f63f396f1ebed45e49bf423c26041f01a0

(In reply to Bastian from comment #23)
> I bet you have no idea how much having two functions named 'setFilter' has
> confused me.. :-)

Oh I figure - in particular as you removed it from the wrong one :-p

Scrap that, somehow the conflict I got when rebasing suggested otherwise. Code is fine, thanks (again)!