Closed Bug 955355 Opened 10 years ago Closed 10 years ago

Sort channel list in IRC tooltips

Categories

(Chat Core :: IRC, defect)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: benediktp, Assigned: benediktp)

Details

Attachments

(1 file, 1 obsolete file)

*** Original post on bio 1918 at 2013-04-04 10:22:00 UTC ***

*** Due to BzAPI limitations, the initial description is in comment 1 ***
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1918 as attmnt 2319 at 2013-04-04 10:22:00 UTC ***

The list of channels in IRC participants'/contacts' tooltips should be sorted to make it easier to look for a particular channel.

The attached patch sorts the list, ignoring the prefixes of the channel and user.
Attachment #8354085 - Flags: review?(clokep)
Comment on attachment 8354085 [details] [diff] [review]
Patch

*** Original change on bio 1918 attmnt 2319 at 2013-04-04 10:36:28 UTC ***

This is a good idea. :)

>diff --git a/chat/protocols/irc/irc.js b/chat/protocols/irc/irc.js
>+    // Sort the list of channels, ignoring the prefixes of channel and user.
>+    // "self" is favored over bind(this) here because of the nested functions.
>+    let self = this;
>+    let sortChannels = function(aChannelStr) {
>+      let prefixes = self.userPrefixes.concat(self.channelPrefixes);
The list of prefixes doesn't change, we can define it outside of the function, which will then use less "selfs" and probably be easier to figure out.

>+      let noPrefixSort = function (a, b) {
>+        a = self.normalize(a, prefixes);
>+        b = self.normalize(b, prefixes);
>+        return a < b ? -1 : a > b ? 1 : 0;
>+      };
>+      return aChannelStr.split(" ").filter(function(name) name != "")
>+                        .sort(noPrefixSort).join(" ");
Does aChannelStr.trim().split(" ").sort(noPrefixSort).join(" "); do the same thing?

>     const kFields = {
[...]
>       away: null,
Somewhere down here it should say "channels: null", you need to set it to use the sortChannels function.
Attachment #8354085 - Flags: review?(clokep) → review-
Attached patch Patch v2Splinter Review
*** Original post on bio 1918 as attmnt 2321 at 2013-04-04 18:22:00 UTC ***

Using trim and regex-splitting on the string now instead of filtering the array, as suggested. The nested functions are gone as suggested on IRC, too.
Attachment #8354087 - Flags: review?(clokep)
Comment on attachment 8354085 [details] [diff] [review]
Patch

*** Original change on bio 1918 attmnt 2319 at 2013-04-04 18:22:07 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354085 - Attachment is obsolete: true
Assignee: nobody → benediktp
Comment on attachment 8354087 [details] [diff] [review]
Patch v2

*** Original change on bio 1918 attmnt 2321 at 2013-04-04 18:37:59 UTC ***

This looks fine and is a good improvement, I think.
Attachment #8354087 - Flags: review?(clokep) → review+
Whiteboard: [checkin-needed]
*** Original post on bio 1918 at 2013-04-04 23:05:32 UTC ***

http://hg.instantbird.org/instantbird/rev/d460a4aafe68

Thanks!
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed]
Target Milestone: --- → 1.4
You need to log in before you can comment on or make changes to this bug.