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

RESOLVED FIXED

Status

Other Applications
ChatZilla
--
enhancement
RESOLVED FIXED
11 years ago
9 years ago

People

(Reporter: Gijs, Assigned: Glen Mailer)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [cz-0.9.83])

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

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

Comment 1

10 years ago
definitely would be nice.
(Assignee)

Comment 2

10 years ago
Created attachment 322714 [details] [diff] [review]
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.
Assignee: rginda → glenjamin+bmo
Status: NEW → ASSIGNED
(Reporter)

Comment 3

10 years ago
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
(Assignee)

Comment 4

10 years ago
Created attachment 323756 [details] [diff] [review]
incremental message IDs, counter on channel object

updated as per comments.
Attachment #322714 - Attachment is obsolete: true
Attachment #323756 - Flags: review?(gijskruitbosch+bugs)
(Reporter)

Comment 5

10 years ago
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.
Attachment #323756 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Comment 6

10 years ago
Created attachment 323765 [details] [diff] [review]
nits fixed, centers jumped message
Attachment #323756 - Attachment is obsolete: true
Attachment #323765 - Flags: review?(gijskruitbosch+bugs)
(Reporter)

Comment 7

10 years ago
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.
Attachment #323765 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 8

10 years ago
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]).
Attachment #323765 - Flags: review+ → review-
(Assignee)

Comment 9

10 years ago
Created attachment 325486 [details] [diff] [review]
much neater display of link and more nits nuked.

Fixed as per comments above and discussion in #chatzilla
Attachment #323765 - Attachment is obsolete: true
Attachment #325486 - Flags: review?(silver)

Comment 10

10 years ago
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.
Attachment #325486 - Flags: review?(silver) → review+
(Assignee)

Comment 11

10 years ago
Created attachment 325987 [details] [diff] [review]
patch corrected as per comment #10
Attachment #325486 - Attachment is obsolete: true
Attachment #325987 - Flags: review?(silver)

Updated

10 years ago
Attachment #325987 - Flags: review?(silver) → review+

Comment 12

10 years ago
Checked in --> FIXED.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Whiteboard: [cz-0.9.83]
(Assignee)

Comment 13

10 years ago
Created attachment 327088 [details] [diff] [review]
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

10 years ago
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)

Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 15

10 years ago
Created attachment 327257 [details] [diff] [review]
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".
Attachment #327088 - Attachment is obsolete: true
Attachment #327257 - Flags: review?(silver)

Comment 16

10 years ago
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).
Attachment #327257 - Flags: review?(silver) → review+

Updated

10 years ago
Status: REOPENED → ASSIGNED
(Reporter)

Comment 17

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

Comment 18

9 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.