Participants Need Context Menu

RESOLVED FIXED in 1.5

Status

Instantbird
Conversation
--
enhancement
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: clokep, Assigned: aleth)

Tracking

Dependency tree / graph

Details

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

5 years ago
*** Original post on bio 451 at 2010-07-17 13:59:00 UTC ***

When right clicking on a participant in a chat there should be a context menu with what can be done "to" that user.

Off the top of my head "whois" and start a private message with are needed.  I don't think whois is implemented in Instantbird yet, however.
*** Original post on bio 451 at 2010-07-17 15:56:44 UTC ***

Maybe also a way to add the nick to the contact list if it isn't already there.
I'm not sure of what libpurple does exactly with IRC nicks in the buddy list to know if they are there. Maybe it periodically sends who commands to the server.
Status: UNCONFIRMED → NEW
Ever confirmed: true
*** Original post on bio 451 at 2010-07-18 14:21:16 UTC ***

1) General remark: we're running into the protocol-specific actions/UI problem here.
On IRC the options should depend on the powers of the user, ie options like kicking, inviting, op/de-op .. should be offered if you have sufficient powers to use them.



2) I don't think the online status is checked at all for 'IRC buddies' on the buddy list.

Fixed the platform fields on this bug btw.
OS: Windows 7 → All
Hardware: x86 → All
(Reporter)

Comment 3

5 years ago
*** Original post on bio 451 at 2010-07-18 16:08:21 UTC ***

(In reply to comment #2)
> 1) General remark: we're running into the protocol-specific actions/UI problem
> here.
> On IRC the options should depend on the powers of the user, ie options like
> kicking, inviting, op/de-op .. should be offered if you have sufficient powers
> to use them.
I was thinking about this yesterday while wondering how to implement this. Generally most protocols would behave the same, with IRC being an exception. That could (relatively) easily be solved by choosing one of two context menus when the conversation is built (note that I haven't fully looked at the implementation of the conversation yet).
I hadn't even thought of different "levels" of the user impacted this, but you're definitely correct. We could mark each menuitem with a "level" field that has "op" "half-op" "voice", etc. and on run time decide which to show, but are these even the same across servers or no? (I.e. can "half-op" on one server have powers to kick while on another it requires "op"?)

> 2) I don't think the online status is checked at all for 'IRC buddies' on the
> buddy list.
You're correct AFAIK.

We should probably think about making a full list of all commands that need to be implemented as well.
Blocks: 954011
Depends on: 953721
(Reporter)

Comment 4

5 years ago
*** Original post on bio 451 at 2011-03-03 18:59:27 UTC ***

(In reply to comment #2)
> 1) General remark: we're running into the protocol-specific actions/UI problem
> here.
> On IRC the options should depend on the powers of the user, ie options like
> kicking, inviting, op/de-op .. should be offered if you have sufficient powers
> to use them.

In other parts of libpurple/Instantbird we'll have enumerators of a particular interface and all those get added to the tooltip, for example.  Something similar could be done for the context menu I think.
(Assignee)

Updated

5 years ago
Depends on: 954473
No longer depends on: 953721
(Assignee)

Updated

5 years ago
Depends on: 953721
(Assignee)

Updated

5 years ago
No longer depends on: 953721, 954473
(Assignee)

Updated

5 years ago
Assignee: nobody → aleth
(Assignee)

Updated

5 years ago
Depends on: 955410
(Assignee)

Comment 5

5 years ago
Created attachment 8354284 [details] [diff] [review]
Patch

*** Original post on bio 451 as attmnt 2516 at 2013-06-25 12:14:00 UTC ***

This adds participant context menu entries to the participant list and to nicks in MUC messages. Protocol-specific actions are left for a followup as this patch is already long enough (but would be easy to add using something similar to the existing getActions() for messages).

The tag-contextmenu handling code is moved from blist.js to a module to allow it to be shared.
Attachment #8354284 - Flags: review?(florian)
Comment on attachment 8354284 [details] [diff] [review]
Patch

*** Original change on bio 451 attmnt 2516 at 2013-06-27 07:49:29 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354284 - Flags: review?(florian)
Attachment #8354284 - Flags: review-
Attachment #8354284 - Flags: feedback+
*** Original post on bio 451 at 2013-06-27 07:49:29 UTC ***

Comment on attachment 8354284 [details] [diff] [review] (bio-attmnt 2516)
Patch

>--- a/instantbird/locales/en-US/chrome/instantbird/instantbird.properties

>+#LOCALIZATION NOTE
>+# These appear in the content area context menu for chat participants.

Maybe it's just too early, but it took me a few seconds to understand the meaning of this sentence. Maybe say something like "nicks of chat participants highlighted in messages." ? Not sure that's really better :-/.

>+# %S is the nick of the chat participant.
>+contextmenu.nickOpenConv=Start conversation with %S

Should this say "Start private conversation with %S" or maybe just "Private Conversation with %S"?

>+contextmenu.nickShowLogs=Show logs for %S

Should we be explicit that it's the logs of messages exchanged privately with %S?

>+contextmenu.nickAddContact=Add %S to contacts...

Should we change this to "Follow %S" on Twitter?

>--- a/instantbird/content/blist.js
> /* This Source Code Form is subject to the terms of the Mozilla Public
>  * License, v. 2.0. If a copy of the MPL was not distributed with this
>  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
> Components.utils.import("resource:///modules/imStatusUtils.jsm");
>+Components.utils.import("resource:///instantbird/modules/imTagMenu.jsm");


I don't think you need the "instantbird/" part of this path.


> function buddyListContextMenu(aXulMenu) {
>   this.target  = document.popupNode;
>   this.menu    = aXulMenu;
>+  this.tagmenu = new TagMenu(this, window);

s/tagmenu/tagMenu/g ?



>--- a/instantbird/content/conversation.xml
>             <xul:listbox anonid="nicklist" class="conv-nicklist"
>                          flex="1" seltype="multiple"
>+                         xbl:inherits="contextmenu=contentcontextmenu"

I'm glad you reused the same context menu, rather than creating a new one :).


>--- a/instantbird/content/instantbird.xul

>   <stringbundleset id="stringbundleset">
>-    <stringbundle id="bundle_instantbird" src="chrome://instantbird/locale/instantbird.properties"/>
>+    <stringbundle id="instantbirdBundle" src="chrome://instantbird/locale/instantbird.properties"/>

This change seems wrong. I don't think a module loaded in 2 windows should introduce a dependency on this having the same name accross both windows.


>+      <menuitem id="context-nick-openconv"
>+                label="&openConversationCmd.label;"
>+                oncommand="gContextMenu.nickOpenConv();"/>
>+      <menuitem id="context-nick-showlogs"
>+                label="&showLogsCmd.label;"
>+                oncommand="gContextMenu.nickShowLogs();"/>
>+      <menu id="context-nick-tags"

This id seems wrong. Maybe context-nick-add-contact?

>+            label="&addContact;">
>+        <menupopup id="context-tags-popup"
>+                  oncommand="gContextMenu.tagmenu.tag(event);"

Nit: fix indent.

>+                   onpopupshowing="gContextMenu.tagmenu.tagsPopupShowing();">
>+          <menuseparator id="context-create-tag-separator"/>
>+          <menuitem id="context-create-tag"
>+                    label="&addNewTagCmd.label;"
>+                    accesskey="&addNewTagCmd.accesskey;"
>+                   oncommand="gContextMenu.tagmenu.addNewTag();"/>

Here too.


>--- a/instantbird/content/nsContextMenu.js

> function nsContextMenu(aXulMenu, aBrowser) {
>   this.target            = null;
>   this.browser           = null;
>+  this.conv              = null;
>   this.menu              = null;
>+  this.tagmenu           = null;
>   this.onLink            = false;
>   this.onMailtoLink      = false;
>   this.onSaveableLink    = false;
>   this.link              = false;
>   this.linkURL           = "";
>   this.linkURI           = null;
>   this.linkProtocol      = null;
>+  this.onNick            = false;
>+  this.nick              = "";
>+  this.buddy             = null;
>+  this.isNickOpenConv    = false;
>+  this.isNickShowLogs    = false;
>+  this.isNickTags        = false;
>   this.isTextSelected    = false;
>   this.isContentSelected = false;
>   this.shouldDisplay     = true;
>   this.ellipsis = "\u2026";

All this stuff really looks like it wants to go in the prototype, but let's keep that for another bug :-).


>   // Initialize context menu.
>   initMenu: function CM_initMenu(aPopup, aBrowser) {
>     this.menu = aPopup;
>     this.browser = aBrowser;
>+    this.conv = this.browser.selectedBrowser._conv;

Isn't this bitrotted by the recent tabbrowser large patch?

Maybe it's fine for now if the context menu is only used by conversation.xml. I'm wondering if tabs with random content will want to use the same context menu for the generic items related to links and the clipboard.


>+    if (this.onNick) {
>+      this.getNickActions();
>+      if (node.localName == "listitem") {
>+        if (!this.onNick) {

Any reason for not having these 2 tests in the same if statement?

>+          // If we're in the participant list, there will be no other entries.

You are testing twice the value of this.onNick within 4 lines. Maybe you want the comment to say explicitly that onNick will now be false if there's no nick related action?


>+        // Add nick to menu entry labels.
>+        let bundle = document.getElementById("instantbirdBundle");
>+        let nick = this.nick;
>+        let changeLabel = function (aId, aLabel) {
>+          document.getElementById(aId).label =
>+            bundle.getFormattedString(aLabel, [nick]);
>+        };
>+        changeLabel("context-nick-openconv", "contextmenu.nickOpenConv");
>+        changeLabel("context-nick-showlogs", "contextmenu.nickShowLogs");
>+        changeLabel("context-nick-tags", "contextmenu.nickAddContact");

Maybe change this so that you only do
changeLabel("openconv", "OpenConv")?
(and if the menuitem ids were selected carefully, you could even use only one parameter, with a toLowerCase call for the other.)

>+    // Standard actions.
>+    this.isNickOpenConv = !isTwitter;
>+    this.isNickShowLogs = this.nickGetLogs();

Maybe add a !! here?

>+    this.isNickTags = !isTwitter && !this.buddy;

These 3 variable names are hard to read (they don't make much sense).
For the second, "isNickWithLogs" could work. For the other 2 I'm currently out of idea.

>+
>+    // Disable everything if there are no actions.
>+    this.onNick &= (this.isNickOpenConv || this.isNickShowLogs || this.isNickTags);
>+  },
>+  normalizedName: function(aNick) {

Maybe call this "normalizeNick" ('normalizedName' is too close to the attribute we have in several interfaces, so I prefer not reusing the same name for a function).

>+    // Unfortunately there is currently no way to obtain the normalizedName
>+    // corresponding to the nick.
>+    // Therefore we may sometimes not find existing logs for a nick,
>+    // and offer "add contact" despite a buddy already existing.

This seems pretty unfortunate indeed. Let's discuss on IRC how we can improve this (possibly in a follow-up).


>+  nickOpenConv: function () {

>+  nickGetLogs: function () {

>+  nickShowLogs: function () {

What about: openConvWithNick, getNickLogs and showNickLogs?


>   // Set various context menu attributes based on the state of the world.
>   setTarget: function (aNode) {
>-
>     // Initialize contextual info.
>     this.onLink            = false;
>     this.linkURL           = "";
>     this.linkURI           = null;
>     this.linkProtocol      = "";
>+    this.onNick            = false;
>+    this.nick              = "";

This seems to be code duplication. Why do we need these 6 lines?

>+        // Nick?
>+        if (!this.onNick && this.conv.isChat &&
>+            (elem.className == "ib-nick" || elem.className == "ib-sender")) {

Should this use |classList.contains| instead of |className ==|?



>diff --git a/instantbird/modules/imTagMenu.jsm b/instantbird/modules/imTagMenu.jsm

I'm not sure the interface exposed by this module is the best we can do for reusability. It currently looks like there was some code you put in a module because you didn't want to duplicate it, but then added specific cases everywhere. Maybe we could also make this module useful for the addbuddy.xul dialog?

Some examples of what I have in mind:
- one function to create and insert the menu items. The function would take as parameter a callback (receiving the DOM element about to be inserted as parameter), called for each tag/node we insert. The callback could deal with making the item a checkbox.
- the addNewTag function would just return the new tag, not create a contact or add a tag to a buddy.

The use case specific stuff would be handled by each consummer of the module.

>+function TagMenu(aParent, aWindow, aIsNewContact=false) {

Not sure what coding style we used last time we used default parameter values, but didn't we put spaces around the "="?

>+  tagsPopupShowing: function () {

>+    if (!this.isNewContact) {
>+      var tags = this._getTarget().contact.getTags();
>+      var groupId = this._getTarget().group.groupId;

let target = this._getTarget();
Or maybe just set this.target in the TagMenu constructor and get rid of the _getTarget method, that may be even cleaner :-).

>+  addNewTag: function () {
>+    let bundle = this.document.getElementById("instantbirdBundle").stringBundle;
>+    let title = bundle.GetStringFromName("newTagPromptTitle");
>+    let message = bundle.GetStringFromName("newTagPromptMessage");

Seems better to use the XPCOM way instead of the XUL way in this case.
(Assignee)

Comment 8

5 years ago
*** Original post on bio 451 at 2013-06-27 11:23:52 UTC ***

(In reply to comment #6)
> >+# %S is the nick of the chat participant.
> >+contextmenu.nickOpenConv=Start conversation with %S
> 
> Should this say "Start private conversation with %S" or maybe just "Private
> Conversation with %S"?

"Start conversation" is what we use in the blist, but I agree it would make sense to be more specific here.

> >+contextmenu.nickShowLogs=Show logs for %S
> 
> Should we be explicit that it's the logs of messages exchanged privately with
> %S?

I think it will be obvious as the entry will only appear when such private logs exist.

> >+contextmenu.nickAddContact=Add %S to contacts...
> 
> Should we change this to "Follow %S" on Twitter?

No, that would be a protocol-specific action (not part of this patch). Also following someone and having them as a contact are not necessarily the same thing, even once twitter DMs are possible.

> >--- a/instantbird/content/nsContextMenu.js
> 
> > function nsContextMenu(aXulMenu, aBrowser) {
> >   this.target            = null;
> >   this.browser           = null;
> >+  this.conv              = null;
> >   this.menu              = null;
> >+  this.tagmenu           = null;
> >   this.onLink            = false;
> >   this.onMailtoLink      = false;
> >   this.onSaveableLink    = false;
> >   this.link              = false;
> >   this.linkURL           = "";
> >   this.linkURI           = null;
> >   this.linkProtocol      = null;
> >+  this.onNick            = false;
> >+  this.nick              = "";
> >+  this.buddy             = null;
> >+  this.isNickOpenConv    = false;
> >+  this.isNickShowLogs    = false;
> >+  this.isNickTags        = false;
> >   this.isTextSelected    = false;
> >   this.isContentSelected = false;
> >   this.shouldDisplay     = true;
> >   this.ellipsis = "\u2026";
> 
> All this stuff really looks like it wants to go in the prototype, but let's
> keep that for another bug :-).

Yup, I didn't like it either, but kept with the style of the file for this ;)

> >   // Initialize context menu.
> >   initMenu: function CM_initMenu(aPopup, aBrowser) {
> >     this.menu = aPopup;
> >     this.browser = aBrowser;
> >+    this.conv = this.browser.selectedBrowser._conv;
> 
> Isn't this bitrotted by the recent tabbrowser large patch?

Actually no, but the variable name should change to reflect that.

> Maybe it's fine for now if the context menu is only used by conversation.xml.
> I'm wondering if tabs with random content will want to use the same context
> menu for the generic items related to links and the clipboard.

If other panels start using the contentareacontextmenu, they will probably have to make other changes anyway.

> These 3 variable names are hard to read (they don't make much sense).
> For the second, "isNickWithLogs" could work. For the other 2 I'm currently out
> of idea.

They are not great, and only work in the context of the existing code (all the flags are is$MENUITEM)

> >+  normalizedName: function(aNick) {
> 
> Maybe call this "normalizeNick" ('normalizedName' is too close to the attribute
> we have in several interfaces, so I prefer not reusing the same name for a
> function).

It's normalizedName because it refers to precisely that attribute in the interfaces ;)

> >   // Set various context menu attributes based on the state of the world.
> >   setTarget: function (aNode) {
> >-
> >     // Initialize contextual info.
> >     this.onLink            = false;
> >     this.linkURL           = "";
> >     this.linkURI           = null;
> >     this.linkProtocol      = "";
> >+    this.onNick            = false;
> >+    this.nick              = "";
> 
> This seems to be code duplication. Why do we need these 6 lines?

I have no idea. I only added to them to stick with the style of the file.

> 
> >+        // Nick?
> >+        if (!this.onNick && this.conv.isChat &&
> >+            (elem.className == "ib-nick" || elem.className == "ib-sender")) {
> 
> Should this use |classList.contains| instead of |className ==|?

I'll make the change to future-proof this, but both classes only occur when IB code sets them on elements it makes itself.

> >diff --git a/instantbird/modules/imTagMenu.jsm b/instantbird/modules/imTagMenu.jsm
> 
> I'm not sure the interface exposed by this module is the best we can do for
> reusability. It currently looks like there was some code you put in a module
> because you didn't want to duplicate it, but then added specific cases
> everywhere. Maybe we could also make this module useful for the addbuddy.xul
> dialog?

I'll have to think about this.

Any requests for changing the filename? (should it be ibTagMenu.jsm?)
*** Original post on bio 451 at 2013-06-27 12:41:53 UTC ***

(In reply to comment #7)

> Any requests for changing the filename? (should it be ibTagMenu.jsm?)

Yes, 'ib' prefix please :).
(Assignee)

Comment 10

5 years ago
Created attachment 8354310 [details] [diff] [review]
Patch

*** Original post on bio 451 as attmnt 2542 at 2013-06-30 10:35:00 UTC ***

Various improvements.

>- one function to create and insert the menu items. The function would take as
>parameter a callback (receiving the DOM element about to be inserted as
>parameter), called for each tag/node we insert. The callback could deal with
>making the item a checkbox.

I can't see a way of doing this which isn't just as ugly (e.g. one would have to either get the existing tags of the contact each time in the callback or add a lazy-getter type mechanism for it). Storing the target in the constructor isn't super pretty, but seems the most readable in this case. I separated out the part of the code which could be usefully and cleanly generalized (getSortedTags).
Attachment #8354310 - Flags: review?(florian)
(Assignee)

Comment 11

5 years ago
Comment on attachment 8354284 [details] [diff] [review]
Patch

*** Original change on bio 451 attmnt 2516 at 2013-06-30 10:35:52 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354284 - Attachment is obsolete: true
(Assignee)

Comment 12

5 years ago
Created attachment 8354311 [details] [diff] [review]
Patch

*** Original post on bio 451 as attmnt 2543 at 2013-06-30 10:54:00 UTC ***

Typo fix.
Attachment #8354311 - Flags: review?(florian)
(Assignee)

Comment 13

5 years ago
Comment on attachment 8354310 [details] [diff] [review]
Patch

*** Original change on bio 451 attmnt 2542 at 2013-06-30 10:54:08 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354310 - Attachment is obsolete: true
Attachment #8354310 - Flags: review?(florian)
(Assignee)

Comment 14

5 years ago
*** Original post on bio 451 at 2013-07-23 19:14:44 UTC ***

I forgot to add a question to comment #9: Wouldn't it be a good idea to make the tagMenu.getSortedTags() in this patch the default behaviour of Services.tags.getTags()? It seems silly to use the tagMenu just for this in addBuddy, and it seems the right place to put it.
Comment on attachment 8354311 [details] [diff] [review]
Patch

*** Original change on bio 451 attmnt 2543 at 2013-08-16 23:28:31 UTC ***

Overall this seems pretty reasonable. r- is only because I think it's bitrotted, and I would like to have another quick look (and maybe try it) before it gets checked in :).

>+    let nickActions = this.getNickActions(isParticipantList);
>+    this.onNick = !nickActions.every(function (action) !action.isActive);

!foo.every(function (bar) !bar.stuff) sounds like too many negations.
Can't this be simplified with .some?

Also, .isActive confused me. Took me a while to figure out what it meant. What about just "visible"?

>+  getNormalizedName: function (aNick) {
>+    // Unfortunately there is currently no way to obtain the normalizedName
>+    // corresponding to the nick.

This comment looks like it wants to contain a bug number ;).


>+  getNickActions: function (aIsParticipantList) {
>+    let bundle = document.getElementById("bundle_instantbird");
>+    let nick = this.nick;
>+    let actions = [];
>+    let addAction = function (aId, aIsActive) {
>+      let mentionNick = (aIsParticipantList ? "" : ".withNick");

Nit: you don't need the ().

>+      let domId = "context-nick-" + aId.toLowerCase();
>+      let stringId = "contextmenu.nick" + aId + mentionNick;

Maybe just do instead:
if (!aIsParticipantList)
  stringId += ".withNick";



>+function TagMenu(aParent = null, aWindow = null, aTarget = null) {

What's the use of the default values here? Does it make a difference if the value is set to null or undefined? Or is this meant as a way to show that all parameters are optional? (a comment may be better to say that).

When looking at the implementation, it's not really clear to me why aParent and aWindow are optional. And providing aTarget or not seems to produce a significantly different behavior, I think it would be nice to explain in a comment here what to expect :).

>+  getSortedTags: function () {

I agree with your suggestion of sorting by default the tags returned by the tags service. Not sure if the tags service should also create the default group if the list is empty or not though.

>+    let tags = Services.tags.getTags();
>+    if (!tags.length)
>+      tags.push(Services.tags.createTag(_("defaultGroup")));

qheaden has just added a touch of bitrot to this ;).

>+
>+    let sortFunction = function (a, b) {
>+      [a, b] = [a.name.toLowerCase(), b.name.toLowerCase()];
>+      return a < b ? -1 : a > b ? 1 : 0;
>+    };
>+    tags.sort(sortFunction);

Shouldn't this look like:
.sort(function(a, b) a.name.toLowerCase().localeCompare(b.name.toLowerCase())

This is likely code that you just moved around though :).
Attachment #8354311 - Flags: review?(florian) → review-
(Assignee)

Comment 16

5 years ago
Created attachment 8354507 [details] [diff] [review]
Patch

*** Original post on bio 451 as attmnt 2738 at 2013-08-20 09:50:00 UTC ***

>I agree with your suggestion of sorting by default the tags returned by the
>tags service. Not sure if the tags service should also create the default group
>if the list is empty or not though.

This patch creates the default group, but this is consistent with qheaden's changes in bug 955543 (bio 2105). I don't see a problem with this as long as bug 955464 (bio 2027) gets fixed, which is desirable anyway.
Attachment #8354507 - Flags: review?(florian)
(Assignee)

Comment 17

5 years ago
Comment on attachment 8354311 [details] [diff] [review]
Patch

*** Original change on bio 451 attmnt 2543 at 2013-08-20 09:50:47 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354311 - Attachment is obsolete: true
Comment on attachment 8354507 [details] [diff] [review]
Patch

*** Original change on bio 451 attmnt 2738 at 2013-08-27 23:41:13 UTC ***

>+#LOCALIZATION NOTE
>+# These appear in the context menu of the chat participants in the
>+# participant list.
>+contextmenu.nickOpenConv=Start a Conversation

After trying it, I would suggest making this string "Private Conversation"

>+# These appear in the context menu for the nicks of chat participants
>+# highlighted in messages, and extend those for the participant list
>+# by mentioning the nick.
>+# %S is the nick of the chat participant.
>+contextmenu.nickOpenConv.withNick=Start Conversation with %S

And "Private Conversation with %S"



>+    addAction("ShowLogs", this.onNick && !!this.getLogsForNick(nick));

This test doesn't work. this.getLogsForNick returns an enumerator, so you need to check the return value of hasMoreElements()



>diff --git a/instantbird/modules/Makefile.in b/instantbird/modules/Makefile.in

This hunk doesn't apply at all...

>--- a/instantbird/modules/Makefile.in
>+++ b/instantbird/modules/Makefile.in
>@@ -8,16 +8,17 @@
> VPATH          = @srcdir@
> 
> include $(DEPTH)/config/autoconf.mk
> 
> EXTRA_JS_MODULES = \
>        ibInterruptions.jsm \

... because the indentation here is tabs. This patch has them converted as spaces.



Coding style nit:
I think our usual style is:
function name(
function(
(ie. the space is before the name, not after the 'function' keyword).
You have plenty of "function (" in this patch. In some cases it's just old code that's being moved around, so I guess I shouldn't flag it... but it in new code it would be nice if we could be consistent.
(Note: I wouldn't have bothered you with this if I didn't have the previous comments.)



I find the discoverability of the context menus on nicks inside the conversation very low. Here's a suggestion to improve it:
diff --git a/chat/themes/conv.css b/chat/themes/conv.css
--- a/chat/themes/conv.css
+++ b/chat/themes/conv.css
@@ -37,6 +37,10 @@
   border-top: 1px dashed red;
 }
 
+.ib-nick, .ib-sender {
+  cursor: context-menu;
+}
+
 .ib-nick {
   font-weight: bold;
 }


I'm really looking forward to trying this in a nightly soon (and disappointed that I have to r- it again, sorry for not catching this earlier :-/).
Attachment #8354507 - Flags: review?(florian) → review-
(Assignee)

Comment 19

5 years ago
Created attachment 8354565 [details] [diff] [review]
Patch

*** Original post on bio 451 as attmnt 2795 at 2013-08-28 14:30:00 UTC ***

(In reply to comment #14)
> >+contextmenu.nickOpenConv=Start a Conversation
> 
> After trying it, I would suggest making this string "Private Conversation"

I changed it to 'Start a Private Conversation' to keep the analogy with the blist context menu, but if you think that's too long you can change it on checkin.

> >+    addAction("ShowLogs", this.onNick && !!this.getLogsForNick(nick));
> 
> This test doesn't work. this.getLogsForNick returns an enumerator, so you need
> to check the return value of hasMoreElements()

Thanks for catching this!

> that's being moved around, so I guess I shouldn't flag it... but it in new code
> it would be nice if we could be consistent.

Hopefully most of these files are consistent now.

> I find the discoverability of the context menus on nicks inside the
> conversation very low. Here's a suggestion to improve it:
> +.ib-nick, .ib-sender {
> +  cursor: context-menu;
> +}

Sadly cursor: context-menu is not supported at all on Windows (and looks non-native and ugly on Linux, at least for me). I would have suggested cursor:default (which at least then varies the pointer from its shape over the message text) but that is what we get when hovering outside a message altogether, so I wasn't sure it was a net win. Discoverability may not be great, but I originally added it as my expectation was that it would be there anyway, and to provide for users that shared that expectation ;)

I also moved the Cu.import of the tagmenu in blist.js from the header to where the tagmenu is actually created, in a probably fairly pointless gesture at saving memory.
Attachment #8354565 - Flags: review?(florian)
(Assignee)

Comment 20

5 years ago
Comment on attachment 8354507 [details] [diff] [review]
Patch

*** Original change on bio 451 attmnt 2738 at 2013-08-28 14:30:57 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354507 - Attachment is obsolete: true
Comment on attachment 8354565 [details] [diff] [review]
Patch

*** Original change on bio 451 attmnt 2795 at 2013-08-28 23:16:09 UTC ***

>diff --git a/instantbird/content/blist.js b/instantbird/content/blist.js

> function buddyListContextMenu(aXulMenu) {
>   this.target  = document.popupNode;
>   this.menu    = aXulMenu;
>+  Components.utils.import("resource:///modules/ibTagMenu.jsm");
>+  this.tagMenu = new TagMenu(this, window,
>+                             this.onBuddy ? this.target.contact : this.target);

There's no way this can work, it's using this.onBuddy...

>   let localName = this.target.localName;
>   let hasVisibleBuddies = !!document.getElementById("buddylistbox").firstChild;
> 
>   // Don't display a context menu on the headers.
>   this.shouldDisplay = localName != "label";
> 
>   this.onContact = localName == "contact";
>   this.onBuddy = localName == "buddy";

... which is only set here.

I'm fixing this at the checkin with a few other trivial changes.
Attachment #8354565 - Flags: review?(florian) → review+
Created attachment 8354569 [details] [diff] [review]
Interdiff of my checkin and attachment 2795 [details]

*** Original post on bio 451 as attmnt 2799 at 2013-08-28 23:17:00 UTC ***

Checked in as http://hg.instantbird.org/instantbird/rev/25646eb93852

Looking forward to having this in the next nightly, thanks! :-)
*** Original post on bio 451 at 2013-08-28 23:18:17 UTC ***

Fixed per comment 17.
Severity: normal → enhancement
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.5
(Assignee)

Updated

5 years ago
Depends on: 955578
(Assignee)

Updated

5 years ago
Depends on: 955580
*** Original post on bio 451 at 2013-09-14 09:27:29 UTC ***

Comment on attachment 8354565 [details] [diff] [review] (bio-attmnt 2795)
Patch

>--- a/chat/components/src/imContacts.js
>+++ b/chat/components/src/imContacts.js
>@@ -136,16 +136,21 @@ TagsService.prototype = {
>   // Get an array of all existing tags.
>   getTags: function(aTagCount) {
>+    if (Tags.length)
>+      Tags.sort(function(a, b) a.name.toLowerCase().localeCompare(b.name.toLowerCase()));

Shouldn't this have been toLocaleLowerCase [1] to be on the safe side? I can file a follow-up bug if someone agrees.

[1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/toLocaleLowerCase
(Assignee)

Comment 25

5 years ago
*** Original post on bio 451 at 2013-09-15 13:36:05 UTC ***

(In reply to comment #19)
> Comment on attachment 8354565 [details] [diff] [review] (bio-attmnt 2795) [details]
> Patch
> 
> >--- a/chat/components/src/imContacts.js
> >+++ b/chat/components/src/imContacts.js
> >@@ -136,16 +136,21 @@ TagsService.prototype = {
> >   // Get an array of all existing tags.
> >   getTags: function(aTagCount) {
> >+    if (Tags.length)
> >+      Tags.sort(function(a, b) a.name.toLowerCase().localeCompare(b.name.toLowerCase()));
> 
> Shouldn't this have been toLocaleLowerCase [1] to be on the safe side? I can
> file a follow-up bug if someone agrees.

It makes sense to fix this for consistency. (Afaik there are other places in the existing code where we use tolowercase instead of tolocalelowercase for performance reasons, but that doesn't apply here.)
*** Original post on bio 451 at 2013-09-16 08:58:10 UTC ***

(In reply to comment #20)

> It makes sense to fix this for consistency. (Afaik there are other places in
> the existing code where we use tolowercase instead of tolocalelowercase for
> performance reasons, but that doesn't apply here.)

Filed as bug 955618 (bio 2176).
(Assignee)

Updated

5 years ago
Depends on: 955553
(Assignee)

Comment 27

5 years ago
Created attachment 8354870 [details] [diff] [review]
Followup to fix getNormalizedName

*** Original post on bio 451 as attmnt 3088 at 2013-11-27 19:29:00 UTC ***

Use the normalize() method from bug 955553 (bio 2115) to replace the getNormalizedName placeholder.
Attachment #8354870 - Flags: review?(florian)
(Assignee)

Comment 28

5 years ago
Comment on attachment 8354870 [details] [diff] [review]
Followup to fix getNormalizedName

*** Original change on bio 451 attmnt 3088 at 2013-11-27 19:30:16 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354870 - Flags: review?(florian)
*** Original post on bio 451 at 2013-11-27 21:37:13 UTC ***

Comment on attachment 8354870 [details] [diff] [review] (bio-attmnt 3088)
Followup to fix getNormalizedName

I suspect you want conv.getNormalizedChatBuddyName in these 2 places.

And the getLogsForAccountAndName implementation likely wants to call account.normalize; rather than the caller.

getBuddyByNameAndProtocol would want too, but it doesn't have access to an account :-/.
(Assignee)

Comment 30

5 years ago
Comment on attachment 8354870 [details] [diff] [review]
Followup to fix getNormalizedName

*** Original change on bio 451 attmnt 3088 at 2013-11-28 14:35:50 UTC ***

(In reply to comment #23)
> I suspect you want conv.getNormalizedChatBuddyName in these 2 places.

That's what I wanted to double-check as well. But we do want normalize() here because we are trying to find out if there is a buddy associated to this name.

For the logs case I am not sure what the right thing to do is. The question is what the normalizedName will be *for a private conversation opened with a MUC chatbuddy*. (For IRC it makes no difference whether we use getNormalizedChatBuddyName or normalize in this case.)

> And the getLogsForAccountAndName implementation likely wants to call
> account.normalize; rather than the caller.

Since we have misgivings about MUC usernames, we should not rely on normalize() giving the desired result here. So I'm keeping it outside logger.js.
Attachment #8354870 - Flags: review?(florian)
(Assignee)

Comment 31

5 years ago
Created attachment 8354876 [details] [diff] [review]
Patch to replace getNormalizedName stub

*** Original post on bio 451 as attmnt 3093 at 2013-11-28 14:38:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354876 - Flags: review?(florian)
(Assignee)

Comment 32

5 years ago
Comment on attachment 8354870 [details] [diff] [review]
Followup to fix getNormalizedName

*** Original change on bio 451 attmnt 3088 at 2013-11-28 14:38:23 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354870 - Attachment is obsolete: true
Attachment #8354870 - Flags: review?(florian)
(Reporter)

Comment 33

5 years ago
Comment on attachment 8354876 [details] [diff] [review]
Patch to replace getNormalizedName stub

*** Original change on bio 451 attmnt 3093 at 2013-11-28 14:40:28 UTC ***

Looks good to me.
Attachment #8354876 - Flags: review?(florian) → review+
(Assignee)

Updated

5 years ago
Depends on: 955709
(Assignee)

Comment 34

5 years ago
*** Original post on bio 451 at 2013-11-28 14:52:29 UTC ***

Moved that discussion to bug 955709 (bio 2260).
(Assignee)

Comment 35

5 years ago
Comment on attachment 8354876 [details] [diff] [review]
Patch to replace getNormalizedName stub

*** Original change on bio 451 attmnt 3093 at 2013-11-28 14:52:43 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354876 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.