Created attachment 301139 [details] [review]
mark nicknames which are no longer available

Desaturates nicknames which are no longer in the room.
A convenience to have when scrolling through chat history.
or realizing that a member disconnected during a conversation.

Attached patch is a WIP patch. It does not seem to work in practice yet. Can I use GJS to debug this further somehow? I'm curious as to how you would do that in practice.

Info on how I might call _dimNick() for each member in the room passed inside _onChannelChanged() would be helpful too.

s/GJS/Gjs-inspector/

(In reply to Bastian from comment #2)
> Attached patch is a WIP patch. It does not seem to work in practice yet.

That's because _dimNick is bogus:

    let pos = 0;
    let nickLocation = text.indexOf(text, pos); // always 0
    while (nickLocation) { // 0 evaluates to false, so the loop is never run }

However even with those issues fixed, the approach looks questionable:

  - nick names in mentions are dimmed as well
  - nicks may be partially dimmed:
       <fmuellner> Yadda yadda yadda
                      fmuellner_ joined
       <fmuellner_> Sorry, got thrown out
                      fmuellner disconnected
       <fmuellner_> Why is my nick dimmed?
  - no undimming when a nick reconnects

If you really want to iterate over the entire text each time a nick changes status, this is how you should do it:
  - split the existing nick tag into something like
       * nick (for the margin)
       * nick-active (for the active color)
       * nick-inactive (for the inactive color)
  - use GtkTextBuffer's iter API (untested pseudo code,
    so take it with a huge grain of salt):

      let iter = buffer.get_start_iter();
      let start;
      do {
          if (iter.begins_tag(nickTag))
              start = iter;
          if (!iter.ends_tag(nickTag))
              continue;
          buffer.remove_tag(activeNickTag, start, iter);
          buffer.apply_tag(inactiveNickTag, start, iter);
      } while(iter.forward_char());

However I'd at least consider individual nick tags:
  - use existing 'nick' text only for margin
  - do something like this in _insertMessage():
      let nickTag = this._lookupTag(message.nick);
      if (!nickTag) {
          nickTag = new Gtk.TextTag({ name: message.nick });
          this._view.get_buffer().get_text_table().add(nickTag);
      }
      tags.push(nickTag);
  - _dimNick() then becomes as simple as:
      let nickTag = this._lookupTag(nick);
      if (nickTag)
          nickTag.foreground_rgba = this._inactiveNickColor;

This approach means you keep additional tags around for every user that says something, but I suspect it's a price worth paying for not iterating the entire buffer each time a lurker connects or disconnects.


> Can I use GJS to debug this further somehow? I'm curious as to how you would
> do that in practice.

I hadn't tried until yesterday, but it looks like gjs doesn't like being run twice in the same process - it works fine for non-js apps, but crashes polari/documents/maps.
So until that is fixed, I'm afraid the answer is no :-(

(The inspector without the interactive module is still useful though)


> Info on how I might call _dimNick() for each member in the room passed
> inside _onChannelChanged() would be helpful too.

You can get the list with channel.group_dup_members_contacts().

Created attachment 301483 [details] [review]
mark nicknames which are no longer available

Desaturates nicknames which are no longer in the room.
A convenience to have when scrolling through chat history.
or realizing that a member disconnected during a conversation.

This patch is functional in all cases I've tried so far.

One question left is what color I should use for the the inactive nickname. For now it's simply gnome's black (Gtk.StateFlags.NORMAL), and not desaturated (#aaaaaa). Is there a desaturated version of Gtk.StateFlags.LINK I can use?

Created attachment 301538 [details] [review]
mark nicknames which are no longer available

Desaturates nicknames which are no longer in the room.
A convenience to have when scrolling through chat history.
or realizing that a member disconnected during a conversation.

(fixed whitespaces)

Created attachment 301540 [details] [review]
mark nicknames which are no longer available

Desaturates nicknames which are no longer in the room.
A convenience to have when scrolling through chat history.
or realizing that a member disconnected during a conversation.

Review of attachment 301540 [details] [review]:

(In reply to Bastian from comment #6)
> Is there a desaturated version of Gtk.StateFlags.LINK
> I can use?

No, but you could copy the activeNick color and desaturate it yourself

::: src/chatView.js
@@ +196,2 @@
         let linkColor = context.get_color(Gtk.StateFlags.LINK);
+        this._activeNickColor = context.get_color(Gtk.StateFlags.LINK);

Why the change from selected to link?

@@ -199,3 @@
         let tags = [
-          { name: 'nick',
-            foreground_rgba: selectedColor },

You lose updating the nick color on style changes (it's obviously more tricky now with two colors depending on online status, but simply removing it is still wrong)

@@ +306,3 @@
+                let members = this._channel.group_dup_members_contacts();
+                for (let j = 0; j < members.length; j++) {
+                    if (message.nick == members[j].get_identifier()) {

Should use TpContact:alias instead of :identifier for consistency (the two are identical for IRC contacts). Also you can write this somewhat more nicely with something like:

let nicks = this._channel.group_dup_members_contacts().map(
    function(m) {
        return m.alias;
    });
if (nicks.indexOf(message.nick) != -1)
    this._styleNick(message.nick, this._activeNickColor);

@@ +569,3 @@
+            tagTable.foreach(Lang.bind(this,
+                    function(tag) {
+                        if (tag.name.indexOf('nick') != -1)

Could use tag.name.startsWith('nick'). In either case, this will match the 'nick' tag as well, which is a bit meh - maybe use "&& tag.name.length > 4" as well?

@@ +604,3 @@
         this._insertStatus(_("%s is now known as %s").format(oldMember.alias,
                                                              newMember.alias));
+        this._styleNick(oldMember.alias, this._inactiveNickColor);

Misses
  this._styleNick(newMember.alias, this._activeNickColor);

@@ +828,3 @@
             if (state.lastNick != message.nick) {
                 let tags = [this._lookupTag('nick')];
+                let nickTag = this._lookupTag('nick'+message.nick);

Style nit: whitespace around operators

@@ +830,3 @@
+                let nickTag = this._lookupTag('nick'+message.nick);
+                if (!nickTag) {
+                    nickTag = new Gtk.TextTag({ name: 'nick'+message.nick });

dto

Created attachment 301682 [details] [review]
mark nicknames which are no longer available

Desaturates nicknames which are no longer in the room.
A convenience to have when scrolling through chat history.
or realizing that a member disconnected during a conversation.

adressed most issues. still two remaining as far as my testing has shown:

- need to rebase my patch with master..
- If you activate HighContrast theme _activeNickColor and _inactiveNickColor becomes equivalent (pure black). When you switch back to Adwaita, all nicks are then marked with the active color. What would be the best approach to avoid this? monkey-patching too?

(In reply to Bastian from comment #12)
> - If you activate HighContrast theme _activeNickColor and _inactiveNickColor
> becomes equivalent (pure black). When you switch back to Adwaita, all nicks
> are then marked with the active color. What would be the best approach to
> avoid this? monkey-patching too?

That's what I was suggesting monkey-patching for - matching the tag name for a particular prefix is ok to determine whether it's a certain type of tag, it's the checking of a color to preserve state that I don't like. Of course if you add something like nickTag._status, you can then replace the string comparison by something like (nickTag._status != undefined).
Stylistically, I would replace _styleNick() by something a bit more general like _setNickStatus() (or _setNickActive() or _updateNick() or so):

_setNickStatus(nick, status) {
    let nickTag = this._lookupTag('nick' + nick);
    if (!nickTag)
        return;

    // alternatively, status could be an isOnline boolean or a custom
    // { ACTIVE, INACTIVE } enum
    nickTag._status = status;
    if (status == Tp.ConnectionPresenceType.AVAILABLE)
        nickTag.foreground_rgba = this._activeNickColor;
    else
        nickTag.foreground_rgba = this._inactiveNickColor;
}

Created attachment 301757 [details] [review]
mark nicknames which are no longer available

I see now. Decided to use Tp.ConnectionPresenceType.AVAILABLE and Tp.ConnectionPresenceType.OFFLINE enums. Confirmed that this implementation works well even when changing back and forth between HighContrast and Adwaita.

the patch seem to produce the following warning in the commandline:

(org.gnome.Polari:9160): tp-logger-CRITICAL **: log_store_pidgin_set_basedir: assertion 'self->priv->basedir == NULL' failed

how do I trace down what causes it?

That looks like a tp-logger bug, and unrelated to your patch. If you want to debug it, you can run polari in gdb using something like

  gdb --args gjs-console $JHBUILD_PREFIX/share/polari/org.gnome.Polari

in a jhbuild shell (assuming you use jhbuild - adjust to your install prefix otherwise)
The "nice" thing about bugs in C code is that at least the established debugging tools work :-)

Review of attachment 301757 [details] [review]:

First things first: Nice work! I've only been using it for a day now, and already got used to it :-)

One issue aside, all comments on the code are style nits. The commit message however - ugh:

 - subject should (usually) use a prefix: and be capitalized:
   "chatView: Mark nicknames which are no longer available"

 - don't write in 3rd person:
   "Desaturate nicknames ..." instead of "(The patch) Desaturates ..."

 - use actual sentences - the first one is fine, the other two are not

::: src/chatView.js
@@ +183,2 @@
     _onStyleUpdated: function() {
+

Unnecessary space change

@@ +194,3 @@
+        let desaturatedNickColor = (this._activeNickColor.red +
+                                    this._activeNickColor.blue +
+                                    this._activeNickColor.green)/3;

Spaces around operators!

@@ +196,3 @@
+                                    this._activeNickColor.green)/3;
+        this._inactiveNickColor = new Gdk.RGBA ({
+                                  red: desaturatedNickColor,

Odd indentation. Should be either

color = new Gdk.RGBA({
            red: red,
            green: green
        });
(i.e. one additional indentation level)

or

color = new Gdk.RGBA({ red: red,
                       green: green });

The latter is the preferred style, with the former alternative to avoid super-long lines.

@@ +214,3 @@
             foreground_rgba: linkColor }
         ];
+

Another odd whitespace change

@@ +227,3 @@
+            if (tag.name && tag.name.startsWith('nick') && tag.name.length > 4) {
+                this._setNickStatus(tag.name.substring(4), tag._status);
+            }

Style nit: no braces for one-line blocks

The condition could be replaced with a simple
  if (tag._status)
but I'm unconvinced myself:
 - the current code assumes a monkey-patched _status property
   for all tags that match the 'nick'+something pattern
 - the alternative assumes that tags with a _status property
   follow a 'nick'+something naming pattern

@@ +321,3 @@
+                    if (message.nick == members[j].get_alias()) {
+                        this._setNickStatus(message.nick, Tp.ConnectionPresenceType.AVAILABLE);
+                        break;

This is the one bit I really don't like - you end up with nMembers * nMessages temporary TpContact objects, and potentially iterate over all of them.

I see two options how you can avoid the extra copy:

 (1) Do something like

     let members;
     if (channel)
         members = channel.dup_members().map(function(m) { return m.alias; });
     else
         members = [];

  *before* looping over pending messages, then something like

      let status = members.indexOf(message.nick) != -1 ? Presence.AVAILABLE
                                                       : Presence.OFFLINE;
      this._setNickStatus(message.nick, status);

   inside the loop.

 (2) Inside the loop, do something like

       this._insertMessage(iter, message, state);
       this._setNickStatus(message.nick, Presence.OFFLINE);

     then use the same code as in _onChannelChanged() to update active nicks.

Unless you really object to setting a potentially wrong status first (which you shouldn't, because your current code does it as well), I'd go with (2) - it is simple (which is good) and allows for a bit of code sharing with _onChannelChanged() (which is also good)

@@ +615,3 @@
+        for (let j = 0; j < members.length; j++) {
+            this._setNickStatus(members[j].get_alias(), Tp.ConnectionPresenceType.AVAILABLE);
+        }

Nit: no braces

Created attachment 301885 [details] [review]
chatView: Mark nicknames which are no longer available

Decided to go with using "if (tag._status)" rather than checking for "'nick' && length > 4" etc. With the attached patch, the only reason to that we still append 'nick' to nickTags in _insertMessage() is to avoid collision with the other tags ('nick', 'status', 'url' etc.). nickTags are only identified via the monkeypatched "_status" attribute. Would you say this makes sense?

The rest should also be fixed now.

Review of attachment 301885 [details] [review]:

(In reply to Bastian from comment #18)
> Decided to go with using "if (tag._status)" rather than checking for "'nick'
> && length > 4" etc.

OK.


> With the attached patch, the only reason to that we
> still append 'nick' to nickTags in _insertMessage() is to avoid collision
> with the other tags ('nick', 'status', 'url' etc.).

Yes, and there wasn't really another reason in previous iterations either - it's a corner case, but it's good to handle it (and the way you do it makes sense).


> Would you say this makes sense?

Yes. There's one bit that now becomes harder to read outside the context of this patch (let's say when working on the code a year from now), with a small suggestion to address it below.


> The rest should also be fixed now.

Not quite, but we are getting there ...

::: src/chatView.js
@@ +223,3 @@
+            if (tag._status) {
+                this._setNickStatus(tag.name.substring(4), tag._status);
+            }

Nit: no braces.

I haven't thought of that before, but now the substring() is a bit 'magic'. The code could be made more self-explaining by:
 - adding a
     const NICKTAG_PREFIX = 'nick';
   at the top and use it in _insertMessage()
 - write the code here as
     let offset = NICKTAG_PREFIX.length;
     tagTable.foreach(
       if (tag._status)
           this._setNickStatus(tag.name.subtring(offset), tag._status);
     )

@@ +314,3 @@
+
+            if (this._channel != null) {
+                let members = this._channel.group_dup_members_contacts();

No. The main point in the last review was to get this part out of the pendingMessages loop - move this down

@@ +316,3 @@
+                let members = this._channel.group_dup_members_contacts();
+                for (let j = 0; j < members.length; j++)
+                this._setNickStatus(members[j].get_alias(), Tp.ConnectionPresenceType.AVAILABLE);

Nit: missing indentation

Created attachment 301895 [details] [review]
chatView: Mark nicknames which are no longer available

ah, that seems sensible.

Review of attachment 301895 [details] [review]:

Phew - last nit pick, promise; good to push with that fixed.

::: src/chatView.js
@@ +322,3 @@
+            let members = this._channel.group_dup_members_contacts();
+            for (let j = 0; j < members.length; j++)
+            this._setNickStatus(members[j].get_alias(),

Missing indentation.

Pushed it now
https://git.gnome.org/browse/polari/commit/?id=c96929c701bd07f2ffe345e64b054f10cd7e0876

but I also accidentically pushed it to gnome-3-16 though. how do I revert that? :/

On the 3-16 branch:
git revert HEAD # assuming you are reverting the last commit
git push origin HEAD:gnome-3-16

Attachment 301895 [details] pushed as c96929c - chatView: Mark nicknames which are no longer available

Reverted the commit now. Thanks for the help!

https://git.gnome.org/browse/polari/commit/?h=gnome-3-16&id=4b803c5d117ad2e0db1cd1cbe401eabd6331b43e