374887 - Enable people to find "important" messages in the channel window by clicking them in the network view

Status: RESOLVED FIXED

(Other Applications :: ChatZilla, enhancement)

Description

[:Gijs (he/him)]

As in summary. I'll go back to dinner now. :-)

Comment 1

[Wayne Mery (:wsmwk)]

definitely would be nice.

Comment 2

[Glen Mailer]

Attachment: tentative idea

Featuring silver's rather handy scrolling function.

This implementation of the feature works fine, but could probably improved. I'm not too sure on the placement of the chatzilla command link.

Basically, i've added a new internal command that goes to a channel and tries to find an element with an ID. Then important messages get an ID and the copied message gets a clickable command.

Comment 3

[:Gijs (he/him)]

Awesome! As I said on IRC, it'd be best, imho, to use an incremented per-channel counter, allowing us to add support for looping through the messages using shortcuts or the like. With that, you can have r+ -- please to be submitting another patch, though? Thanks! :D

Comment 4

[Glen Mailer]

Attachment: incremental message IDs, counter on channel object

updated as per comments.

Comment 5

[:Gijs (he/him)]

Comment on attachment 323756 [details] [diff] [review]
incremental message IDs, counter on channel object

<snip>
>Index: mozilla/extensions/irc/xul/content/commands.js
>@@ -3896,12 +3897,46 @@ function cmdSetCurrentView(e)
>     if ("lockView" in e.view)
>         delete e.view.lockView;
> 
>     setCurrentObject(e.view);
> }
> 
>+function cmdJumpToAnchor(e)
>+{
>+    if (e.hasOwnProperty("channelName"))
>+    {
>+        e.channel = new CIRCChannel(e.server, e.channelName);
>+    }
>+    else
>+    {
>+        if (!e.channel)
>+        {
>+            display(getMsg(MSG_ERR_REQUIRED_PARAM, "channel-name"), MT_ERROR);
>+            return;
>+        }
>+    }

Nit: Please put this in an else if (!e.channel), and save two lines. :-)
You mentioned on IRC that you copied it from somewhere else - assuming there are no other bits in the else statement there, please do the same thing there.

>+    if (!e.channel.frame)
>+    {
>+        display(getMsg(MSG_JUMPTO_ERR_NOCHAN, e.channel.unicodeName),MT_ERROR);
>+        return;
>+    }
>+    
>+    var document = getContentDocument(e.channel.frame);
>+    var row = document.getElementById(e.anchor);
>+    
>+    if (!row)
>+    {
>+        display(getMsg(MSG_JUMPTO_ERR_NOANCHOR), MT_ERROR);
>+        return;
>+    }
>+    
>+    dispatch("set-current-view", {view: e.channel});
>+    e.channel.scrollToElement(row,"inview");

Would it be nicer to use "center" here? Why/why not? r= either way, we can discuss on IRC if you want.

<snip>

>Index: mozilla/extensions/irc/xul/content/static.js
<snip>
>@@ -4406,14 +4412,22 @@ function __display(message, msgtype, sou
>         }
>     }
> 
>     // Copy Important Messages [to network view].
>     if (isImportant && client.prefs["copyMessages"] && (o.network != this))
>     {
>-        o.network.displayHere("{" + this.unicodeName + "} " + message, msgtype,
>-                              sourceObj, destObj);
>+        var prefix = "";
>+        if (importantId)
>+        {
>+            var cmd = getMsg(MSG_JUMPTO_BUTTON, "jump-to-anchor " +
>+                          importantId + " " + o.channel.unicodeName);
Nit: Please line this up to the brace.

<snip>

with the nits fixed, and some discussion about "inview" vs. "center", r=me.

Comment 6

[Glen Mailer]

Attachment: nits fixed, centers jumped message

Comment 7

[:Gijs (he/him)]

Comment on attachment 323765 [details] [diff] [review]
nits fixed, centers jumped message

r=me

Whoever checks in (prolly me), remember to fix cmdNames on checkin.

Comment 8

[James Ross]

Comment on attachment 323765 [details] [diff] [review]
nits fixed, centers jumped message

>+msg.jumpto.button         = [[Jump To][Jump to this Message][%S]]
>+msg.jumpto.err.nochan     = ``%S'' is no longer open.
>+msg.jumpto.err.noanchor   = The anchor cannot be found.

The jump button should be labeled with the channel name, as a replacement for the existing {#foo}.

e.g. [[%1$S][Jump to this message in %1$S][%2$S]]
or [[%S][Jump to this message][%S]]


>+function cmdJumpToAnchor(e)
>+        display(getMsg(MSG_JUMPTO_ERR_NOCHAN, e.channel.unicodeName),MT_ERROR);

Nit: space after comma here.

>+    e.channel.scrollToElement(row,"center");

Nit: space after comma here.


>@@ -4343,12 +4344,17 @@ function __display(message, msgtype, sou
>+            msgRow.setAttribute("id",importantId);

Nit: space after comma here.

>+            var cmd = getMsg(MSG_JUMPTO_BUTTON, "jump-to-anchor " +
>+                             importantId + " " + o.channel.unicodeName);
>+            prefix = cmd + " {" + this.unicodeName + "} ";

So, as mentioned above, drop the {#foo} part and just have the button (which'll end up as [#foo]).

Comment 9

[Glen Mailer]

Attachment: much neater display of link and more nits nuked.

Fixed as per comments above and discussion in #chatzilla

Comment 10

[James Ross]

Comment on attachment 325486 [details] [diff] [review]
much neater display of link and more nits nuked.

>+         ["jump-to-anchor",    cmdJumpToAnchor,                   CMD_NEED_SRV],

I just realised, this should be CMD_NEED_NET. CMD_NEED_SRV would require you still be connected, which should not be required here.

Nits: blank lines have trailing whitespace, plus an unnecessary line before the end of the cmdJumpToAnchor function.

>+        var prefix = "";
>+        if (importantId)

You only used 'prefix' inside the 'if', and declared it with 'var' inside as well, so drop this 'var' line entirely.

>+            var channel = o.channel.unicodeName;

This could use this.unicodeName instead of o.channel.unicodeName (see the old code).

r=silver with those fixed.

Comment 11

[Glen Mailer]

Attachment: patch corrected as per comment #10

Comment 12

[James Ross]

Checked in --> FIXED.

Comment 13

[Glen Mailer]

Attachment: dont button munge the main message

So, rather foolishly the entire message is munged with the inline-button munger enabled - which allows malicious users to put things like [[Click Me][Honest, it's fine][disconnect-all]] and have it munged into a button on the network tab.

I've munged the prefix and the message separately now, however this means that the message copied into the network list isn't marked as important and thus the tab goes green instead of red.

Any ideas?

Comment 14

[James Ross]

The latest patch will fail in the case an important message has no importantId, although I don't know how likely that is to occur.

Since non-string messages passed to display() are only allowed to be DOM nodes, why don't we allow them to have an attribute set for isImportant for this code:

4232                 if ((typeof message == "string") && me)

Comment 15

[Glen Mailer]

Attachment: dont button munge the main message

As suggested by silver, copied messages are now pre-munged DOM elements with an isImportant attribute set to "true".

Comment 16

[James Ross]

Comment on attachment 327257 [details] [diff] [review]
dont button munge the main message

Nits: trailing whitespace (on the blank lines), missing space after comma (setAttribute).

Comment 17

[:Gijs (he/him)]

This hasn't been checked in, it seems? Can Glen do this himself by now? ;-)

Comment 18

[:Gijs (he/him)]

Checking in mozilla/extensions/irc/xul/content/static.js;
/cvsroot/mozilla/extensions/irc/xul/content/static.js,v  <--  static.js
new revision: 1.282; previous revision: 1.281
done

w/ nits fixed.