375825 - Make "join channel" a non-modal window and allow user to enter search string and start search

Status: RESOLVED FIXED

(Other Applications Graveyard :: ChatZilla, enhancement)

Description

[worms]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1.2) Gecko/20070222 SeaMonkey/1.1.1
Build Identifier: 

"join channel" usage current behaviour: on large networks, time taken to search obliges the user to either cancel the ongoing search or wait untill the search is finished before he can do anything else (switching to a new tab for instance)

Prefered (and usable) behaviour for this dialog would be:

- Open the window "join channel"
- Enter (or not) a string to search in the "quick search"
- Start the search by clicking on a "start search" button. Possibility to "cancel" or "pause" and "refresh" 

- Searching should NOT disallow switching to another tab but should execute in "background" 





Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 2

[James Ross]

There's multiple things described in this bug; I'm not sure it is wise to keep them as one bug, but I'll leave it for now.

Phase 1: Remove modality of dialog. Easy.
Phase 2: Make loading and searching not hang dialog (as much?). Medium.

I do not believe a start search or cancel button are useful. If we can get phase 2 right, there is no need for either: the user can just type another string and it'll stop the current search, and it wont prevent them using the main window, clicking Refresh, or closing the dialog.

Comment 3

[James Ross]

Attachment: [Checked in] Phase 1: Make Join Channel dialog be non-modal

Turns out the Join Channel dialog was already setting network.joinDialog to itself onload and removing it onunload, so this just makes that useful by not being modal and focusing if already open. The centering code is the same as the preferences window.

Comment 4

[James Ross]

Comment on attachment 273175 [details] [diff] [review]
[Checked in] Phase 1: Make Join Channel dialog be non-modal

Checked in.

Comment 5

[James Ross]

I've taken to rewriting the state machine used in the channel dialog; it is now a multi-state machine, where each of "list", "load" and "filter" can be running at any time. The core code has just been moved about, although I tweaked the loading code (which gave an almost 10x speed-up!). Probably have a patch tomorrow, if I get some time for it.

Comment 6

[Stéphane Grégoire (yamo)]

When testing this on irc.epiknet.org, I have this message on ChatZilla :

Usage of /list for listing all channels is deprecated. Please use "/squery alis help" instead.
	===	LIST output too large, truncated
	===	Usage of /list for listing all channels is deprecated. Please use "/squery alis help" instead.


my ChatZilla version is : ChatZilla 0.9.78.1-rdmsoft [XULRunner 1.8.0.10/0000000000]

Comment 7

[:Gijs (he/him)]

(In reply to comment #6)
> When testing this on irc.epiknet.org, I have this message on ChatZilla :
> 
> Usage of /list for listing all channels is deprecated. Please use "/squery alis
> help" instead.
>         ===     LIST output too large, truncated
>         ===     Usage of /list for listing all channels is deprecated. Please
> use "/squery alis help" instead.
> 
> 
> my ChatZilla version is : ChatZilla 0.9.78.1-rdmsoft [XULRunner
> 1.8.0.10/0000000000]
> 

That's not really related to this bug, and also not really ChatZilla's fault. It would tell you this for a normal /list as well. Please file a separate bug.

Comment 8

[:Gijs (he/him)]

Ergh, so I misunderstood - I thought it didn't even show anything when doing a /list (or using the Join Channel dialog), but it does. So what you're asking is to hide these messages and/or process them when using the Join Channel dialog to list channels? Still a different bug, but anyway...

Comment 9

[Stéphane Grégoire (yamo)]

Hi,
I have made a mistake in my comment : it was  not on epiKnet, it was on ircnet (ircnet.nerim.net)  that I test "join channel" ; on some networks /list is forbidden or make disconnect ChatZilla.

I think a warning message before doing the "/list"  would be a great help for ChatZilla users.

I've read the help on Ircnet but the alternative command
/squery alis list *
give only 60 channel names. :(

Should I report another bug?

Comment 10

[James Ross]

Attachment: Use concurrent operations to make the dialog not suck

OK, so a bit of a messy diff here. The main difference is the change from using a single state to having a state for each operation and letting them run concurrently (in small steps) if appropriate. The filtering is now done as a stepped process too, instead of in one large blob like before.

Each operation has a state which can change from IDLE, START, RUN, STOP and ERROR. The operations are LIST, LOAD and FILTER.

Each operation step attempts to run for no more than 200ms, and there is a delay of 50ms between each round of operation steps. This keeps the dialog responsive, while updating the display and progressing at a reasonable speed.

The majority of the code in the processOp* functions is taken from the old code, and rejigged a little for the new division of operations.

An additional speed-up was obtained from lower-casing the channel names and topics as they're added to the list, rather than each time it filters.

Comment 11

[:Gijs (he/him)]

Comment on attachment 283241 [details] [diff] [review]
Use concurrent operations to make the dialog not suck

<snip>
>-function runFilter()
>+function onFilter()
> {
>+    startOperation(OP_FILTER);
> }
Couldn't you just put the above line in the xul document instead of onFilter(), and do away with this?

<snip>
>+function getOperation(op)
> {
>+    ASSERT(op in OPS, "Invalid op-code for getOperation: " + op);
>+    var ops = OPS[op];
>+    return data[ops.key];
> }

From what I can tell you actually use the ASSERT + var ops = OPS[op] more than actually using the data, so wouldn't it work better to do a function getOperation(op), that returns ops? Then the state lines in functions further down (getOperation(i).state and such) could use data[getOperation(op).key].state and that would work.

> 
>-function setSubstate(newSubstate)
>+function startOperation(op)
> {
>-    state.substate = newSubstate;
>+    ASSERT(op in OPS, "Invalid op-code for startOperation: " + op);
>+    var ops = OPS[op];
>+    if (ops.ignore)
>+        return;
>+
>+    var dbg = "startOperation(" + ops.key + ")";
>+    var state = data[ops.key];
>+
>+    // STATE_ERROR operations must not do anything. Assert and bail.
>+    ASSERT(state.state != STATE_ERROR, dbg + " in STATE_ERROR");
>+    if (state.state == STATE_ERROR)
>+        return;
ASSERT returns the result of the test, so you can put the assert in the if statement. Same applies for several other uses of it, please change those as well.

<snip>
>+    state.timeTaken = PROCESS_TIME_MAX;
Why not 0?

>+    state.totalTimeTaken = 0;
>+    processOperation(op);
>+    ASSERT(state.state == STATE_RUN, dbg + " didn't enter STATE_RUN");
> }

<snip>

> 
>+function updateOperations()
> {
>+    for (var i = 1; i < OPS.length; i++)
>     {
>+        var state = getOperation(i).state;
>+        if ((state == STATE_START) || (state == STATE_RUN) ||
>+            (state == STATE_STOP))
How can state ever be STATE_START here? Doesn't startOperation take care of processing that anyway?

<snip>
    fn += ops.key.substr(0, 1).toUpperCase() + ops.key.substr(1);
Nit: ops.key[0].toUpperCase() + ops.key.substr(1);

<snip>
>+    if (timeCall)
>+    {
>+        var en = Number(new Date());
>+        state.timeTaken = (en - st);
>+        state.totalTimeTaken += state.timeTaken;

Now that I'm looking at this again, you don't actually seem to read those anywhere in the patch or file. Am I missing something, or can they go away? :-)

<snip>
>+    // Force the end and process syncronously.
Nit: Synchronously

<snip>


>+function processOpLoadRun(state)
<snip>
>+
>+    // Done if there is no more data, and we're not *expecting* any more.
>+    if ((dataLeft == 0) && !opListRunning)
>+        return STATE_STOP;
Don't you want to check if the list is in ERROR state here, too? Because in that case it won't get you any more data either, afaict

<snip>
>+function processOpFilterStart(state)
>+{
>+    state.text = document.getElementById("filterText").value.toLowerCase();
>+    state.searchTopics = document.getElementById("searchTopics").checked;
>+    state.minUsers = document.getElementById("minUsers").value * 1;
>+    state.maxUsers = document.getElementById("maxUsers").value * 1;
>+    state.exactMatch = null;
>+    state.currentIndex = 0;
>+    state.channelText = state.text;
>+    if (!state.channelText.match(/^[#&+!]/))
>+        state.channelText = "#" + state.channelText;
I believe you'll want to use the network's available (for checking) and default (for changing) channel type here.

<snip>
>+function processOpFilterStop(state)
>+{
>+    if (state.exactMatch)
>     {
>-        if (channels.length > 0)
>-            channelTreeView.childData.appendChildren(channels);
>-        state.loadFile.close();
>-        delete state.loadFile;
>-        delete state.loadPendingData;
>-        delete state.loadChunk;
>-        delete state.loadedSoFar;
>-        delete state.loadNeverComplete;
>-        updateProgress();
>-        setState(STATE_IDLE);
>-        runFilter();
>+        if (!createChannelItem.isHidden)
>+            createChannelItem.hide();
>+    }
>+    // If nothing is slected, select the "create channel" row.
Nit: selected



>+function invalidateRow(index)
>+{
>+    var tree = document.getElementById("channelList");
>+    tree.treeBoxObject.invalidateRow(index);
>+}
>+
>+function scrollToRow(index)
>+{
>+    var tree = document.getElementById("channelList");
>+    tree.treeBoxObject.scrollToRow(index);
>+}
>+
>+function ensureRowIsVisible()
>+{
>+    var tree = document.getElementById("channelList");
>+    if (channelTreeView.selectedIndex >= 0)
>+        tree.treeBoxObject.ensureRowIsVisible(channelTreeView.selectedIndex);
>+    else
>+        tree.treeBoxObject.ensureRowIsVisible(0);
> }
Could you just make global variables of the tree and/or treeboxobject and set them in onLoad? Seems like that'd be faster than doing an id lookup all the time.

r- pending the bunch of questions and/or nits above.

Comment 12

[James Ross]

Attachment: Concurrent operations v1.1

I believe this addresses all the nits, questions and issues, except:
 - I've kept the onFilter indirection from the XUL as the XUL calls it in
   multiple places and I'd like to keep as much of the implementation hidden
   from the XUL.
 - The time taken code has all been removed. It was only used by my debugging
   code (which displayed the current states, times, etc. in the title bar).

Comment 13

[James Ross]

Ignore the typo of getOperationData as getOperationDate, that's fixed locally.

Comment 14

[James Ross]

Oh, doh, one more thing I've not changed:
 - The channel prefix checking is still static, as I'm not sure what the best
   way to deal with it is - the dialog stays open when you disconnect, which
   means the actual values can both disappear, and change, during the dialog's
   lifetime.

Comment 15

[:Gijs (he/him)]

Comment on attachment 286288 [details] [diff] [review]
Concurrent operations v1.1

<snip>
>Index: xul/content/channels.js
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/irc/xul/content/channels.js,v
>retrieving revision 1.5
>diff -d -p -u -6 -r1.5 channels.js
>--- xul/content/channels.js	3 Aug 2007 23:42:40 -0000	1.5
>+++ xul/content/channels.js	26 Oct 2007 13:46:49 -0000
>@@ -38,28 +38,63 @@ var client;
> var network;
> var channels = new Array();
> var createChannelItem;
> 
> var channelTreeShare = new Object();
> var channelTreeView;
>+var channelTreeBoxObject;
>+var channelLoadLabel;
>+var channelLoadBar;
>+var channelLoadBarDesk;
>+var channelFilterText;
>+var channelSearchTopics;
>+var channelMinUsers;
>+var channelMaxUsers;
>+var channelJoinBtn;
>+var channelRefreshBtn;
>+

Nit: Might as well declare var foo, bar, baz, ...;

<snip> 
>+
>+// Define constants for the valid states of each operation.
> s = 0;
>-const SUBSTATE_START = ++s;  // Starting an operation.
>-const SUBSTATE_RUN   = ++s;  // Running...
>-const SUBSTATE_END   = ++s;  // Clean-up/ending operation.
>-const SUBSTATE_ERROR = ++s;  // Error occurred: don't try do to any more.
>+const STATE_IDLE  = ++s;  // Not doing this operation.
>+const STATE_ERROR = ++s;  // Error occurred: don't try do to any more.
>+const STATE_START = ++s;  // Starting an operation.
>+const STATE_RUN   = ++s;  // Running...
>+const STATE_STOP  = ++s;  // Clean-up/ending operation.
> delete s;
Nit: please add a comment explaining order + values matter here, too (because you compare somewhere else in this file using <=)

<snip>
>+function getOperationDate(op)
I know you told me to ignore this, just saying you typoed it more than once. Do a replace all if you haven't already ;-)


Otherwise this seems cool to me. As for the network disconnecting etc., good point, but please just cache the data then (make array and string copies onLoad or something - don't bother trying to update them though, that sounds like work ;-). With that and the above fixed, r=me.

Comment 16

[James Ross]

Checked in --> FIXED.

Any further improvements or suggestions should be filed as new bugs.