Created attachment 258184 [details] [review]
tabCompletion: Implement smart completion

Currently the order of completions is alphabetical.
Implement a smarter completion mechanism giving preference
to members who spoken recently and members who mentioned you
or you mentioned them.

The current order of preference is, first preference to members
who you mentioned or they mentioned you; second, members who
has spoken recently in the channel.

(In reply to comment #0)
> Just use this bug report to follow the discussion. Current brainstorm is:
> 
> // Order taking into account:
> // (1) mentioned you before on that channel
> // (2) is currently active in a conversation
> // (3) you had private chats/mentions previously

part 3 is not done in the patch, but works fine enough (don't think it will work better with that) with the two firsts rules

Created attachment 264069 [details] [review]
tabCompletion: Implement smart completion

Rebased to master.

Created attachment 264523 [details] [review]
tabCompletion: Implement smart completion

Currently the order of completions is alphabetical.
Implement a smarter completion mechanism giving preference
to members who spoken recently and members who mentioned you
or you mentioned them.

The current order of preference is, first preference to members
who you mentioned or they mentioned you; second, members who
has spoken recently in the channel.

(In reply to comment #4)
> Created an attachment (id=264523) [details] [review]
> tabCompletion: Implement smart completion
> 
> Currently the order of completions is alphabetical.
> Implement a smarter completion mechanism giving preference
> to members who spoken recently and members who mentioned you
> or you mentioned them.
> 
> The current order of preference is, first preference to members
> who you mentioned or they mentioned you; second, members who
> has spoken recently in the channel.

Florian, this is for testing since has debug logs. Can you try for a hour or so and tell me how do you feel? (if it is not too much slow changing rooms or when you receive a message, etc)

(In reply to comment #5)
> (In reply to comment #4)
> > Created an attachment (id=264523) [details] [review] [details] [review]
> > tabCompletion: Implement smart completion
> > 
> > Currently the order of completions is alphabetical.
> > Implement a smarter completion mechanism giving preference
> > to members who spoken recently and members who mentioned you
> > or you mentioned them.
> > 
> > The current order of preference is, first preference to members
> > who you mentioned or they mentioned you; second, members who
> > has spoken recently in the channel.
> 
> Florian, this is for testing since has debug logs. Can you try for a hour or so
> and tell me how do you feel? (if it is not too much slow changing rooms or when
> you receive a message, etc)

ah, debug logs is profiling. So you can see how much time the algorithm spend in each phase (1:Retrieving contacts from hard drive, 2: Sorting)

Now I attach the patch to review

Created attachment 264525 [details] [review]
tabCompletion: Implement smart completion

Currently the order of completions is alphabetical.
Implement a smarter completion mechanism giving preference
to members who spoken recently and members who mentioned you
or you mentioned them.

The current order of preference is, first preference to members
who you mentioned or they mentioned you; second, members who
has spoken recently in the channel.

(In reply to comment #7)
> Created an attachment (id=264525) [details] [review]
> tabCompletion: Implement smart completion
> 
> Currently the order of completions is alphabetical.
> Implement a smarter completion mechanism giving preference
> to members who spoken recently and members who mentioned you
> or you mentioned them.
> 
> The current order of preference is, first preference to members
> who you mentioned or they mentioned you; second, members who
> has spoken recently in the channel.

This is the patch to review. Hope you like it! =)

Created attachment 264539 [details] [review]
tabCompletion: Implement smart completion

Currently the order of completions is alphabetical.
Implement a smarter completion mechanism giving preference
to members who spoken recently and members who mentioned you
or you mentioned them.

The current order of preference is, first preference to members
who you mentioned or they mentioned you; second, members who
has spoken recently in the channel.

Created attachment 264542 [details] [review]
Profiling

Better in this way, comment #9 is the patch to review. If you want profiling just add above that patch the patch in comment #10. (patches before those were wrong since I unwanted changed a line which disable the reorder of autocompletion).

Review of attachment 264539 [details] [review]:

Hey, sorry this took so long - on a higher level, I still think that eventually we want to generalize this, as we'll need ranking in other places as well:
 - "Favorites" section in user list
 - room order
 - possibly join/message-user dialogs

Continuously hitting the logger for all of these will be quite ugly, so we'd need a better approach longer-term (for instance caching the stuff we're actually interested in and keep it updated without going through the logger while we get "live" events).

However those are future ideas for improvement, for now let's just go with this ...

::: src/tabCompletion.js
@@ +39,3 @@
+
+        this._roomManager = new ChatroomManager.getDefault();
+        this._roomManager.connect('active-changed',

It might make sense to look into using the newly added 'active-state-changed' signal instead? The signal basically combines 'active-changed' and 'notify::channel' (of the active room) ...

@@ +87,3 @@
+
+            this._active = false;
+            return;

I don't quite like how this is organized - basically an "early" return is a bit unexpected at this point. How about something like:

let active = this._room == room;
if (active == this._active)
    return;

this._active = active;
this._cachedEvents = [];
this._tpLoggerEvents = [];

if (active) {
    this._membersChangedId = ...;
   this._roomChannelId = ...;
   this._onChannelChanged();
} else {
  this._room.disconnect(...);
 ...
}

@@ +104,3 @@
+                               Lang.bind(this, this._onMembersChanged));
+        // Channel signals
+        if (this._room.channel) {

Eeeks:

if (this._room.channel) {
   //stuff
}

someOtherStuff();

if (this._room.channel) {
  // more channel stuff
}

Also: both if blocks together are just _onChannelChanged()?

@@ +117,3 @@
                 function() {
                     this._popup.disconnect(id);
+                    this._onActiveRoomChanged();

This looks wrong - the point for these 'unmap' shenanigans is to not modify the popup's contents while it is up. However active room changes are just one trigger, we now miss member changes or sent/received messages - so that code should be in _setupCompletions instead.

@@ +144,3 @@
+
+            if (this._room.channel.has_interface(Tp.IFACE_CHANNEL_INTERFACE_GROUP))
+                this._members = this._room.channel.group_dup_members_contacts();

Maybe add
  else
     this._members = [];
for clarity? At least in the IRC case, rooms don't change between public and private, but it looks clearer to me to cover both cases in the same place rather than handling one in the constructor ...
Actually - is there any particular reason to cache members in the first place? Just calling group_dup_member_contacts() in _setupCompletions() shouldn't be terribly expensive, no?

@@ +149,3 @@
+            let target = Tpl.Entity.new_from_room_id(this._room.channel.identifier);
+
+            this._retrieveEventsAndSetupCompletions(account, target, HISTORY_DAYS);

We should not hit the logger unless (members.length > 1) - there's no popup in private chats, and there's nothing to sort if I'm the only person in a room ...

@@ +169,3 @@
+                    this._setupCompletions();
+                else
+                    this._retrieveEventsAndSetupCompletions(account, target, days);

This reads a tad bit odd IMHO - how about doing

if (days <= 0) {
  this._setupCompletions();
  return;
}

at the beginning of the function, and then just do

  this._tpLoggerEvents = ...;
  this._retrieveEventsAndSetupCompletions(account, target, days - 1);

in the callback?

@@ +212,3 @@
+
+        let sortedMembers = activeMembers.concat(members);
+        // Makes each member unique in the array, taking advance of the fact

s/advance/advantage/

@@ +218,3 @@
+            function(elem, pos) { return sortedMembers.indexOf(elem) == pos; });
+        // Filter not connected members
+        sortedMembers = sortedMembers.filter(

Any reason for not doing this at the beginning when constructing activeMembers?

let activeMembers = events.map(function(e) {
    return e[EventInfoPair.SENDER_ALIAS];
}).filter(function(m) {
    // filter out offline members
    return members.indexOf(m) != -1;
});

looks more self-explanatory to me ...

@@ +238,3 @@
+            // To split the message the regex take into account
+            // "x,", "x:" and "x "
+            let splitedMessage = message.split(/,| |:/);

Would be "splittedMessage" if split was a regular verb - however the correct form is "splitMessage".

But what do you think of changing this to something like:

let selfRegex = new RegExp('\\b' + ownNick + '\\b');
let othersRegex = new RegExp('\\b(' + members.join('|') + ')\\b');

for (let i = 0; i < events.length; i++) {
    let [sender, message] = events[i];
    if (sender != ownNick) {
        // someone mentioned the user
        if (selfRegex.test(message) {
            ...
        }
    } else {
        // someone was mentioned by the user
       if (othersRegex.test(message) {
           ...
      }
    }
}

@@ +247,3 @@
+                        mentions.push(events[i][EventInfoPair.SENDER_ALIAS]);
+                }
+            } else if (events[i][EventInfoPair.SENDER_ALIAS] == ownNick) {

Hm? That's
if (foo) {} else if (!foo), no?

@@ +264,3 @@
+        // Filter not connected members
+        sortedMembers = sortedMembers.filter(
+            function(elem, pos) { return members.indexOf(elem) >= 0; });

As above, would it make sense to filter out "offline" events beforehand?

@@ +269,3 @@
+    },
+
+    _addCompletionsToWidget: function(completions) {

Misleading name - the method *replaces* existing completions rather than amending the new ones. As it's only used in one place anyway, I'd suggest moving the code there rather than coming up with a better name.

(In reply to comment #12)
> However those are future ideas for improvement, for now let's just go with this

Ah, one more thing I intended to dump here but forgot:
It would be really fancy to normalize all nicks:
 (1) I get a ping as "fmuellner-afk"
 (2) I get back and set nick to "fmuellner"
 (3) I want to respond to the ping in (1)

Basically: IRC is horribly quirky in many places, and we should eventually think about working around it a bit :-)
(Of course, I don't expect you to address any of this *now*)

*** Bug 766365 has been marked as a duplicate of this bug. ***

GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org.
As part of that, we are mass-closing older open tickets in bugzilla.gnome.org
which have not seen updates for a longer time (resources are unfortunately
quite limited so not every ticket can get handled).

If you can still reproduce the situation described in this ticket in a recent
and supported software version of Polari, then please follow
  https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines
and create a new ticket at
  https://gitlab.gnome.org/GNOME/polari/-/issues/

Thank you for your understanding and your help.