Closed Bug 954993 Opened 7 years ago Closed 7 years ago

Integrate Show Nick

Categories

(Instantbird :: Conversation, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: clokep, Assigned: florian)

References

(Depends on 1 open bug)

Details

(Whiteboard: [1.3-wanted])

Attachments

(1 file, 2 obsolete files)

*** Original post on bio 1562 at 2012-06-29 13:00:00 UTC ***

I think we've mostly decided that Show Nick is useful (we're all surprised when it's not available on new profiles). It should be integrated into Instantbird by default.

Should it be integrated at the message style level or as part of the conversation UI or some other way? It would be nice to be able to disable it (as some users will dislike it) and change the style of it (that was added in bug 954634 (bio 1202)).
Whiteboard: [1.3-wanted]
*** Original post on bio 1562 at 2012-07-11 11:10:06 UTC ***

I think we want to add a span node automatically, and let message themes handle the theming. Maybe with a default handling for message themes that don't support nicks inside messages; similar to what we did for the unread ruler.
Attached patch WIP1 (obsolete) — Splinter Review
*** Original post on bio 1562 as attmnt 2037 at 2012-11-03 20:09:00 UTC ***

This integrates the Show Nick add-on. The code has approximatively the same quality that the add-on has, but it integrates correctly with the message themes.

If we were out of time, I think we could take this as-is, but I would like to experiment a bit with also detecting nicks that are no longer in the room, and I think that can let us optimize things a bit (as we no longer need to change the regexp each time someone leaves the room for example, and never remove nicks, so when rebuilding a new regexp, we can keep the old nicks that have already been escaped, and just append the new ones...).
Attachment #8353797 - Flags: feedback?
Assignee: nobody → florian
Status: NEW → ASSIGNED
*** Original post on bio 1562 at 2012-11-03 21:50:05 UTC ***

We may want a hidden pref to turn this off. (In case for some reason it looks really horrible on some message themes, or in case it has horrible performance issues in some cases that we have never tested.)
Attached patch Patch v2 (obsolete) — Splinter Review
*** Original post on bio 1562 as attmnt 2038 at 2012-11-04 00:02:00 UTC ***

This handles nicks that are no longer in the room (note: the list of old nicks isn't kept when putting the conversation on hold and redisplaying it).
Unfortunately this doesn't let me optimize as much as I hoped, as it's not possible to just append new nicks to the array to rebuild the regexp, the array also has to be sorted again :-(.

If the sort() call each time a nick is added turns out to be expensive, a possible optimization would be to append nicks at the end of an array of nicks of the same length, and to concat all the arrays to build the regexp. The nicks would then only be sorted by length, but that should produce the same result. Doing this without solid data showing that the sort is expensive may just be code obfuscation, so I haven't done it.


I also added boolean hidden pref named messenger.conversations.showNicks to let users turn this off from about:config.
Attachment #8353798 - Flags: review?(bugzilla)
Comment on attachment 8353797 [details] [diff] [review]
WIP1

*** Original change on bio 1562 attmnt 2037 at 2012-11-04 00:02:17 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353797 - Attachment is obsolete: true
Attachment #8353797 - Flags: feedback?
Comment on attachment 8353798 [details] [diff] [review]
Patch v2

*** Original change on bio 1562 attmnt 2038 at 2012-11-04 16:19:47 UTC ***

>+pref("messenger.conversations.showNicks", true);
I wonder if we should have two prefs: one to turn off colouring the nicks, and one to turn off tagging the nodes. The first would be an aesthetic choice (maybe for people using message styles that look bad with the defaults), the second would be for performance. That way we could use the nick tagging for e.g. context menus on nicks without that functionality breaking for people who turn off Show Nick.

>+     <method name="trackNick">
>+       <parameter name="aNick"/>
>+       <body>
>+       <![CDATA[
>+         if (aNick && ("_showNickArray" in this)) {
>+           this._showNickArray.push(aNick.replace(this._nickEscape, "\\$&"));
>+           delete this._showNickRegExp;
>+         }
When can it happen that aNick can be empty/undefined? Why are we worried about this?
Since we never remove nicks from the array, shouldn't we check we the nick is not already there?

>+     <method name="getShowNickModifier">
>+       <body>
>+       <![CDATA[
>+         return (function (aNode) {
>+           if (!("_showNickRegExp" in this)) {
>+             const echap = /([\][)(\\|?^$*+])/g;
You can reuse this._nickEscape as the function has 'this' set to the binding.

>+             if (!("_showNickArray" in this)) {
>+               this._showNickArray = Object.keys(this.buddies)
>+                                           .filter(function(nick) nick) // avoid the empty string
>+                                           .map(function(x) x.replace(this._nickEscape, "\\$&"), this);
>+             }
>+
>+             if (!this._showNickArray.length) {
>+               // nobody, disable...
>+               this._showNickRegExp = {exec: function() null};
>+               return 0;
>+             }
>+
>+             // The reverse sort ensures that if we have "foo" and "foobar",
>+             // "foobar" will be matched first by the regexp.
>+             let nicks = this._showNickArray.sort().reverse().join("|");
>+             this._showNickRegExp = new RegExp("\\b(" + nicks + ")\\b", "g");
Since we match the entire nick at word boundaries, unless I am missing something we can do without the sort. This means we can get rid of the _showNickArray initializer above, set it to empty when the binding is created, and then just add nicks as they come in.

>+               let color = buddy.color;
>+               elt.setAttribute("style", this._getColorStyle(color));
>+               elt.setAttribute("nickColor", color);
This can just be
elt.setAttribute("style", buddy.colorStyle));
elt.setAttribute("nickColor", buddy.color);

I have not tested the CSS for the message styles yet.
Attachment #8353798 - Flags: review?(bugzilla) → review-
*** Original post on bio 1562 at 2012-11-04 17:12:43 UTC ***

(In reply to comment #5)
> Comment on attachment 8353798 [details] [diff] [review] (bio-attmnt 2038) [details]
> Patch v2
> 
> >+pref("messenger.conversations.showNicks", true);
> I wonder if we should have two prefs: one to turn off colouring the nicks, and
> one to turn off tagging the nodes. The first would be an aesthetic choice
> (maybe for people using message styles that look bad with the defaults), the
> second would be for performance. That way we could use the nick tagging for
> e.g. context menus on nicks without that functionality breaking for people who
> turn off Show Nick.

That makes sense, but until we actually do something else that's useful with the tags (and I don't see that coming for 1.3), I don't want people to turn off the highlights and still have degraded performances, so I think this would be better handled in the follow-up that will add the better context menus.

> 
> >+     <method name="trackNick">
> >+       <parameter name="aNick"/>
> >+       <body>
> >+       <![CDATA[
> >+         if (aNick && ("_showNickArray" in this)) {
> >+           this._showNickArray.push(aNick.replace(this._nickEscape, "\\$&"));
> >+           delete this._showNickRegExp;
> >+         }
> When can it happen that aNick can be empty/undefined? Why are we worried about
> this?

I'm worried because inserting an empty nick in the regexp means matching everything, ie taking 100% CPU forever.
It happened at the time I developed the add-on. I don't remember why. Probably as a consequence of other bugs.

> Since we never remove nicks from the array, shouldn't we check we the nick is
> not already there?

Probably :-(.


> >+     <method name="getShowNickModifier">
> >+       <body>
> >+       <![CDATA[
> >+         return (function (aNode) {
> >+           if (!("_showNickRegExp" in this)) {
> >+             const echap = /([\][)(\\|?^$*+])/g;
> You can reuse this._nickEscape as the function has 'this' set to the binding.

Already done, I just forgot to remove this obsolete regexp. Thanks for catching it.

> >+             if (!("_showNickArray" in this)) {
> >+               this._showNickArray = Object.keys(this.buddies)
> >+                                           .filter(function(nick) nick) // avoid the empty string
> >+                                           .map(function(x) x.replace(this._nickEscape, "\\$&"), this);
> >+             }
> >+
> >+             if (!this._showNickArray.length) {
> >+               // nobody, disable...
> >+               this._showNickRegExp = {exec: function() null};
> >+               return 0;
> >+             }
> >+
> >+             // The reverse sort ensures that if we have "foo" and "foobar",
> >+             // "foobar" will be matched first by the regexp.
> >+             let nicks = this._showNickArray.sort().reverse().join("|");
> >+             this._showNickRegExp = new RegExp("\\b(" + nicks + ")\\b", "g");
> Since we match the entire nick at word boundaries, unless I am missing
> something we can do without the sort.

A nick can contain word boundaries (eg. flo and flo|away).
Attached patch Patch v3Splinter Review
*** Original post on bio 1562 as attmnt 2042 at 2012-11-04 18:19:00 UTC ***

(an interdiff is at http://pastebin.instantbird.com/95462)
Attachment #8353803 - Flags: review?(bugzilla)
Comment on attachment 8353798 [details] [diff] [review]
Patch v2

*** Original change on bio 1562 attmnt 2038 at 2012-11-04 18:19:27 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353798 - Attachment is obsolete: true
Comment on attachment 8353803 [details] [diff] [review]
Patch v3

*** Original change on bio 1562 attmnt 2042 at 2012-11-04 18:31:07 UTC ***

Looking forward to having this built in! :)
Attachment #8353803 - Flags: review?(bugzilla) → review+
Whiteboard: [1.3-wanted] → [1.3-wanted][checkin-needed]
Depends on: 955189
*** Original post on bio 1562 at 2012-11-05 01:48:11 UTC ***

Checked in as http://hg.instantbird.org/instantbird/rev/38fa9b323195

Can't wait to try thi sin the nightlies!
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [1.3-wanted][checkin-needed] → [1.3-wanted]
Target Milestone: --- → 1.3
Depends on: 955193
Depends on: 955282
Depends on: 955323
Depends on: 955423
You need to log in before you can comment on or make changes to this bug.