315913 - Replace RDF userlist with direct nsITreeView implementation

Status: RESOLVED FIXED

(Other Applications :: ChatZilla, enhancement)

Description

[[e-mail]]

This is mainly a personal rant, because I've only just written the patch so I haven't had time to profile it properly, but I get the feeling that Chatzilla spends too much time sorting the nick list. This is particularly evident when an op pings out; what tends to happen is that you get join, auto-op, part and nick changes in quick succession, which seems to kill my CPU.

The main thrust of my proposal is changing the tree from a content view to a builder view, achieved by setting flags="dont-build-content" on the tree. This already allows us to automatically sort when switching channel, or when new nicks join a channel. The sort order is initialized once, and then the tree maintains its sort, although this can of course be toggled. Nick (and op changes, in sort by op mode) are handled by removing and reinserting the nick.

Calls to updateUserList are therefore removed in setCurrentObject, my_cjoin, my_cmode and my_cnick although in the latter two cases calls to _removeUserFromGraph and _addUserToGraph are used to maintain the nick list.

I changed setListMode to clear the tree cache directly; it appeared to be relying on some unknown side effect for its attribute change to take effect.

The only other change necessary in the current code was the code that extracts the nick from a given tree row; as the builder view does not permit arbitrary attributes I had to extract the unicode name from the RDF data source. However the reverse operation of obtaining the tree row from a nick is simplified to tree.builderView.getIndexOfResource(nick.getGraphResource()).

I'm not sure what style is preferred so I tested for nsITreeColumn in my new sortUserList() method although I have only tested it on 1.8+ so far.

Comment 1

[[e-mail]]

Attachment: Proposed patch

Comment 2

[[e-mail]]

This patch also mitigates bug 271989 somewhat, in that unless it's the current user changing their nick then the selection is not lost, and the scrolling is more stable i.e. the tree tries to maintain the top nick although obviously there are edge cases such as the user scrolled to the top changing their nick.

Comment 3

[James Ross]

Thanks for filing this bug in the wrong state, and thank you Bugzilla for not letting me fix the state.

Comment 4

[:Gijs (out for now; he/him)]

Attachment: Patch to switch to a JS userlist

Woo! Why not just rip out rdf altogether. I promise, it will make us happier.

So this basically uses tree-utils.js to get a JS treeview going. There are a number of advantages to this approach:

- It's faster. Period.
- We can do away with the stupid selection keeping crap, because of the separate views selections are kept automagically. Sweet!
- Scroll is kept during joins/parts/nick-changes/mode-changes, so that bug is fixed too.
- We explicitly reassign the view whenever the userlist changes sides, fixing that regression on trunk immediately (we should probably still have someone get that fixed in trunk for rdf trees, but hey...)
- It's much better for accessibility, because those rdf re-roots are really not going to do add/removal detection any good. That said, I'm not sure as I haven't tested this. Seems logical enough, though.
- PS:  7 files changed, 186 insertions(+), 369 deletions(-)

I guess the only problem is that this patch is rather, uh, big. Sorry 'bout that.

Comment 5

[:Gijs (out for now; he/him)]

Oh, and obviously after this is checked in we can CVS remove rdf.js (not done in the patch)

Comment 6

[James Ross]

Damnit, you're being productive again. *Goes to find reviewing hat* :)

PS: Changed summary to match what's going on.

Comment 7

[James Ross]

Comment on attachment 281557 [details] [diff] [review]
Patch to switch to a JS userlist

This is not a full review, just a selection of comments for now.

>Index: mozilla/extensions/irc/xul/content/commands.js
>     if (elem.getAttribute("collapsed") == "true")
>-    {
>-        if (e.thing == "userlist")
>-        {
>-            if (sourceObject.TYPE == "IRCChannel")
>-            {
>-                client.rdf.setTreeRoot("user-list",
>-                                       sourceObject.getGraphResource());
>-                setSelectedNicknames(document.getElementById("user-list"),
>-                                     sourceObject.userlistSelection);
>-                
>-            }
>-            else
>-            {
>-                client.rdf.setTreeRoot("user-list", client.rdf.resNullChan);
>-            }
>-        }
>-
>         newState = "false";
>-    }
>     else
>-    {
>-        if ((e.thing == "userlist") && (sourceObject.TYPE == "IRCChannel"))
>-        {
>-            var rv = getSelectedNicknames(document.getElementById("user-list"));
>-            sourceObject.userlistSelection = rv;
>-        }
>         newState = "true";
>-    }

Please collapse this into a single-line ternary (?:). Heck, I think you could go as far as this:

  var newState = (elem.getAttribute("collapsed") != "true");

Then let the automatic toString() give you a string for setAttribute.


>Index: mozilla/extensions/irc/xul/content/handlers.js
>+function UserEntry(userObj, channelListShare)
>+{
>+    var self = this;
>+    function getUName()
>+    {
>+        return self._userObj.unicodeName;
>+    };
>+    function getSortFn()
>+    {
>+        if (client.prefs["sortUsersByMode"])
>+            return ule_sortByMode;
>+        return ule_sortByName;
>+    };
>+    XULTreeViewRecord.call(this, channelListShare);
>+    this.setColumnPropertyName("usercol", getUName);
>+    this._userObj = userObj;
>+    userObj.chanListEntry = this;
>+    this.__defineGetter__("sortCompare", getSortFn);

I can hear baby Jesus crying from here.

Definately include a comment for the XULTreeViewRecord.call line, explaining why we're calling it like this.

I'm not entirely happy with the back-reference (_userObj), although you don't need to manually delete as you've done in the patch either. getUName could certainly avoid using it by directly returning the argument userObj.

I don't like using a getter for sortCompare, but I'm guessing you did that to avoid the rediculious overhead of XPConnect for each individual compare in the sort?

>+}
>+
>+UserEntry.prototype = XULTreeViewRecord.prototype;
>+
>+var ule_sortByName =
>+function ule_sortByName(a, b)
>+{
>+    if (!a || !b || (a._userObj.unicodeName == b._userObj.unicodeName))

You can omit the !a and !b checks here, as we control the complete child list and they will always be UserEntry objects.

>+        return 0;
>+    var aName = a._userObj.unicodeName.toLowerCase();
>+    var bName = b._userObj.unicodeName.toLowerCase();
>+    return ((aName < bName) ? -1 : 1);

Nit: drop the inner parentheses.


>Index: mozilla/extensions/irc/xul/content/static.js
>+
>+        if (!("userList" in source))
>+        {
>+            source.userListShare = {};

Nit: new Object() please.

>+            source.userList = new XULTreeView(source.userListShare);
>+            source.userList.getCellProperties = ul_getcellprops;
>+        }

>+// Properties getter for user list tree view
>+var ul_getcellprops = 
>+function ul_getcellprops(index, column, properties)
>+{
>+    if ((index < 0) || (index >= this.childData.childData.length) ||
>+        !properties)
>+    {
>+        return;
>+    }
>+
>+    var userObj = this.childData.childData[index]._userObj;
>+    var atomSvc = getService("@mozilla.org/atom-service;1", "nsIAtomService");

I'd consider loading this onto client during initialisation, since getCellProperties is called something silly by the tree code (each time a cell is painted or hovered over, IIRC).

Comment 8

[[e-mail]]

(In reply to comment #7)
>   var newState = (elem.getAttribute("collapsed") != "true");
> 
> Then let the automatic toString() give you a string for setAttribute.
FYI you could consider the .collapsed property which is already boolean i.e.
var newState = !elem.collapsed;
...
elem.collapsed = newState;

Comment 9

[James Ross]

If that's supported in Mozilla 1.0 - as I think it is - then yeah, we should probably do that (assuming it actually works under testing).

Comment 10

[:Gijs (out for now; he/him)]

Attachment: Patch with initial comments addressed

OK, this should take care of the comments.

I've left the explicit deletion of the _userObj thing because (1) I don't think it *hurts*, and (2) I'm overly cautious for leaks (we have two bugs filed on us and I'm not taking chances in making things worse). If some of those stupid functions for sorting end up in the wrong place and keep a reference to the userlist user object, which still has a reference to the channel user, to the channel, to the network, to the frame... we're going to leak just about the world.

Comment 11

[James Ross]

Comment on attachment 288355 [details] [diff] [review]
Patch with initial comments addressed

>             this.parent.eventPump.addEvent(ev);
>+            

Nit: trailing whitespace.

>+            // User must be a channel user, update sort name for userlist:
>+            cuser.updateSortName();


>@@ -2523,12 +2466,18 @@ function my_cpart (e)
>                 msg = MSG_SOMEONE_LEFT_REASON;
> 
>             this.display(getMsg(msg, [e.user.unicodeName, e.channel.unicodeName,
>                                       e.reason]),
>                          "PART", e.user, this);
>         }
>+
>+        var idx = e.user.chanListEntry.childIndex;
>+        this.userList.childData.removeChildAtIndex(idx);
>+        // clean up:

This comment seems a bit pointless. Perhaps have one before the |var idx| which says you're removing the user's entry from the userlist and the userlist object from the object model.

>+        delete e.user.chanListEntry._userObj;
>+        delete e.user.chanListEntry;


>@@ -2573,15 +2523,20 @@ function my_ckick (e)
>         }
> 
>         this.display(getMsg(MSG_SOMEONE_GONE,
>                             [e.lamer.unicodeName, e.channel.unicodeName,
>                              enforcerProper, e.reason]),
>                      "KICK", e.user, this);
>+
>+        var idx = e.lamer.chanListEntry.childIndex;
>+        this.userList.childData.removeChildAtIndex(idx);
>+        // clean up:

Same as previous comment. Seems like common code too, perhaps it could be factored out?

>+        delete e.lamer.chanListEntry._userObj;
>+        delete e.lamer.chanListEntry;


>@@ -2666,17 +2617,41 @@ function my_cquit (e)
>                                 [e.user.unicodeName,
>                                  e.server.parent.unicodeName, e.reason]),
>                          "QUIT", e.user, this);
>         }
>     }
> 
>-    this._removeUserFromGraph(e.user);
>     this.removeUsers([e.user]);
>+    var idx = e.user.chanListEntry.childIndex;
>+    this.userList.childData.removeChildAtIndex(idx);
>+    // clean up:

And again, comment + common code.

>+    delete e.user.chanListEntry._userObj;
>+    delete e.user.chanListEntry;
>+
>     this.updateHeader();
> }
> 
>+CIRCChannel.prototype.clearUserList =

I think this should be a private method ("_" prefix).

>+function my_clearuserlist()


>+var ule_sortByName =

Nit: Unnecessary, the function name is automatically in the global scope.

>+function ule_sortByName(a, b)


>+var ule_sortByMode =

Nit: Ditto.

>+function ule_sortByMode(a, b)


>@@ -2853,50 +2804,32 @@ function updateUserList()
>         return;
> 
>     // We'll lose the selection in a bit, if we don't save it if necessary:

This comment is no longer correct, is it?

>     if (("currentObject" in client) && client.currentObject &&
>         client.currentObject.TYPE == "IRCChannel")
>     {
>-        chan = client.currentObject;
>-        chan.userlistSelection = getSelectedNicknames(node, chan);
>+        reSortUserlist(client.currentObject);
>     }
>-    reSortUserlist(node);
>-
>-    // If this is a channel, restore the selection in the userlist.
>-    if (chan)
>-        setSelectedNicknames(node, client.currentObject.userlistSelection);
> }


>+function getNicknameForUserlistRow(index)
>+{
>+    // This wouldn't be so hard if APIs didn't change so much... see bug 221619
>+    var userlist = document.getElementById("user-list");
>+    if (userlist.columns)
>+        var col = userlist.columns.getNamedColumn("usercol");

Nice use of JavaScript's crazy scope rules. :)


>@@ -3198,18 +3131,45 @@ function getTabForObject (source, create
>                              "contentAreaDNDObserver);");
>         browser.source = source;
>         source.frame = browser;
>         ASSERT(client.deck, "no deck?");
>         client.deck.appendChild (browser);
>         syncOutputFrame (source);
>+
>+        if (!("userList" in source))
>+        {
>+            source.userListShare = new Object();
>+            source.userList = new XULTreeView(source.userListShare);
>+            source.userList.getCellProperties = ul_getcellprops;
>+        }

This will create a userList object for every single view, will it not? Is that really necessary?


>+// Properties getter for user list tree view
>+var ul_getcellprops = 

Nit: Same as earlier.

>+function ul_getcellprops(index, column, properties)
>+{
>+    if ((index < 0) || (index >= this.childData.childData.length) ||
>+        !properties)
>+    {
>+        return;
>+    }
>+
>+    var userObj = this.childData.childData[index]._userObj;
>+
>+    properties.AppendElement(client.atomSvc.getAtom("voice-" + userObj.isVoice));
>+    properties.AppendElement(client.atomSvc.getAtom("op-" + userObj.isOp));
>+    properties.AppendElement(client.atomSvc.getAtom("halfop-" + userObj.isHalfOp));
>+    properties.AppendElement(client.atomSvc.getAtom("admin-" + userObj.isAdmin));
>+    properties.AppendElement(client.atomSvc.getAtom("founder-" + userObj.isFounder));
>+    properties.AppendElement(client.atomSvc.getAtom("away-" + userObj.isAway));

I wonder how much the atom service costs us here... might be worth a followup patch/bug to check a) how often getCellProperties is called, and b) whether caching all the atoms in a JS object helps significantly over getAtom (it should avoid the costly XPC transition, at least).

>+}

r=silver with the nits fixed. You can fix the other things as you see fit.

Comment 12

[[e-mail]]

(In reply to comment #11)
>how often getCellProperties is called
Basically, on every paint or hit-test of a cell.

Comment 13

[:Gijs (out for now; he/him)]

(In reply to comment #11)
> >@@ -2523,12 +2466,18 @@ function my_cpart (e)
> >                 msg = MSG_SOMEONE_LEFT_REASON;
> > 
> >             this.display(getMsg(msg, [e.user.unicodeName, e.channel.unicodeName,
> >                                       e.reason]),
> >                          "PART", e.user, this);
> >         }
> >+
> >+        var idx = e.user.chanListEntry.childIndex;
> >+        this.userList.childData.removeChildAtIndex(idx);
> >+        // clean up:
> 
> This comment seems a bit pointless. Perhaps have one before the |var idx| which
> says you're removing the user's entry from the userlist and the userlist object
> from the object model.
> Same as previous comment. Seems like common code too, perhaps it could be
> factored out?

Done.

> >+CIRCChannel.prototype.clearUserList =
> 
> I think this should be a private method ("_" prefix).

Done.

> >+        if (!("userList" in source))
> >+        {
> >+            source.userListShare = new Object();
> >+            source.userList = new XULTreeView(source.userListShare);
> >+            source.userList.getCellProperties = ul_getcellprops;
> >+        }
> 
> This will create a userList object for every single view, will it not? Is that
> really necessary?

Fixed to only do it for channel objects

> > <snip> 
> I wonder how much the atom service costs us here... might be worth a followup
> patch/bug to check a) how often getCellProperties is called, and b) whether
> caching all the atoms in a JS object helps significantly over getAtom (it
> should avoid the costly XPC transition, at least).

I'll file a followup bug on that.


I found another core bug - the whole clear image + style cache thing doesn't seem to be working for me at all. I'll file that too, though I'm pretty convinced it's not our bug. We'll move when we're sure, or something.

Obviously I've fixed all the nits, so this is now FIXED. Woo!