Add a context menu to the user icon to set and remove it

RESOLVED FIXED in 1.6

Status

Instantbird
Contacts window
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: clokep, Assigned: mayanktg)

Tracking

Dependency tree / graph

Details

Attachments

(2 attachments, 10 obsolete attachments)

19.89 KB, image/png
Details
8.01 KB, patch
florian
: review+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
*** Original post on bio 782 at 2011-05-14 13:16:00 UTC ***

You should be able to remove the buddy icon after it's set (apparently by setting it to null).

Suggested UI is: an "X" in the corner that appears during hover of the image.
Blocks: 953777
*** Original post on bio 782 at 2011-05-21 19:31:38 UTC ***

What about a context menu with "Set..." and "Remove" as options on the buddy icon?

At least I hope that many people have seen context menus already.
*** Original post on bio 782 at 2011-05-21 20:25:41 UTC ***

I like the context menu idea: it's easy to implement ([good first bug]-able!), and doesn't waste space in the always visible UI.

The usual downside of context menus is that they aren't very discoverable and so should not be the only way to access a feature. But in this case specific, I don't mind if a few users fail to understand how one can remove an icon, it's not a terrible loss for them anyway... And if someone is really upset by his buddy icon, it's more likely to click around and discover the context menu.
(Assignee)

Comment 3

4 years ago
Created attachment 8377139 [details] [diff] [review]
Changes to add the ability to remove buddy icon

This attachment contains minor changes to blist.js, blist.xul and imCore.js . Added menupopup to change&remove icon.
Attachment #8377139 - Flags: review?(florian)
Attachment #8377139 - Flags: review?(aleth)
(Assignee)

Comment 4

4 years ago
Created attachment 8377144 [details]
Menu screenshot

Comment 5

4 years ago
Comment on attachment 8377139 [details] [diff] [review]
Changes to add the ability to remove buddy icon

Review of attachment 8377139 [details] [diff] [review]:
-----------------------------------------------------------------

::: chat/components/src/imCore.js
@@ +163,5 @@
>      this._notifyObservers("status-changed", aMessage);
>    },
>  
> +  // Set the buddy icon for the user. If no parameter is passed then
> +  // icon is removed and set to default one.

The comments that should be improved are those in the interface (imIUserStatusInfo.idl), which is where you would go to find out how these functions are to be called. 

Please improve the comment for getUserIcon too.

Also, passing no parameter is not supported by the setUserIcon interface definition. You have to pass null.

::: instantbird/content/blist.js
@@ +592,5 @@
>            this.userIconClick();
>          break;
>      }
>    },
> +  // Choose Buddy Icon using file picker.

It's the user icon, not a buddy icon. ("Buddy" usually refers to a contact the user has added.)

@@ +604,5 @@
>      fp.appendFilters(nsIFilePicker.filterImages);
>      if (fp.show() == nsIFilePicker.returnOK)
>        Services.core.globalUserStatus.setUserIcon(fp.file);
>    },
> +  // Removes the already set Buddy Icon.

You don't need "already set" here.

If no icon is currently set, the menu item in the context menu should be grayed out.

@@ +605,5 @@
>      if (fp.show() == nsIFilePicker.returnOK)
>        Services.core.globalUserStatus.setUserIcon(fp.file);
>    },
> +  // Removes the already set Buddy Icon.
> +  userIconRemove: function bl_userIconRemove(){

Nit: space after ()

::: instantbird/content/blist.xul
@@ +45,5 @@
>  
>  #endif
>    <popupset id="mainPopupSet">
>      <tooltip id="buddyTooltip" type="buddy"
>               onpopupshowing="return !('_droptarget' in window);"/>

Nit: blank line after here...

@@ +46,5 @@
>  #endif
>    <popupset id="mainPopupSet">
>      <tooltip id="buddyTooltip" type="buddy"
>               onpopupshowing="return !('_droptarget' in window);"/>
> +    <menupopup id="buddyIcon">

userIconContextMenu please, for consistency with the other context menu in the popupset.

@@ +47,5 @@
>    <popupset id="mainPopupSet">
>      <tooltip id="buddyTooltip" type="buddy"
>               onpopupshowing="return !('_droptarget' in window);"/>
> +    <menupopup id="buddyIcon">
> +      <menuitem label="Change"

These are user-visible strings, so they have to be localised. Look at the other context menu in this file for an example of how this is done.

@@ +51,5 @@
> +      <menuitem label="Change"
> +                onclick="buddyList.userIconClick();" default="true"/>
> +      <menuitem label="Remove"
> +                onclick="buddyList.userIconRemove();"/>
> +    </menupopup>

... and here.

@@ +129,1 @@
>                 aria-label="&userIcon.label;" tooltiptext="&userIcon.label;"

Why did you remove the onclick handler?
Attachment #8377139 - Flags: review?(florian)
Attachment #8377139 - Flags: review?(aleth)
Attachment #8377139 - Flags: review-

Updated

4 years ago
Summary: Ability to remove buddy icon → Add a context menu to the user icon to set and remove it
Comment on attachment 8377139 [details] [diff] [review]
Changes to add the ability to remove buddy icon

>   <popupset id="mainPopupSet">
>     <tooltip id="buddyTooltip" type="buddy"
>              onpopupshowing="return !('_droptarget' in window);"/>
>-
>+    <menupopup id="buddyIcon">
>+      <menuitem label="Change"
>+                onclick="buddyList.userIconClick();" default="true"/>
>+      <menuitem label="Remove"
>+                onclick="buddyList.userIconRemove();"/>
>+    </menupopup>

These need to be "oncommand" instead of "onclick" handlers. You'd lose keyboard-compatibility otherwise.
(Assignee)

Comment 8

4 years ago
Created attachment 8378480 [details] [diff] [review]
Fixed mentioned errors, added comments and localized strings

@@ +129,1 @@
>                 aria-label="&userIcon.label;" tooltiptext="&userIcon.label;"
> -               onclick="buddyList.userIconClick();

I removed onClick handler because due to it the menupopup was unable to show up. Even on right-clicking the File Picker turned up.
(Reporter)

Updated

4 years ago
Attachment #8378480 - Flags: review?(aleth)
(Reporter)

Updated

4 years ago
Attachment #8377139 - Attachment is obsolete: true
(In reply to Mayank Kumar [:mayanktg] from comment #8)
> Created attachment 8378480 [details] [diff] [review]
> Fixed mentioned errors, added comments and localized strings
> 
> @@ +129,1 @@
> >                 aria-label="&userIcon.label;" tooltiptext="&userIcon.label;"
> > -               onclick="buddyList.userIconClick();
> 
> I removed onClick handler because due to it the menupopup was unable to show
> up. Even on right-clicking the File Picker turned up.

Try something like

onclick="if (event.button == 0) buddyList.userIconClick();"
(Assignee)

Comment 10

4 years ago
Created attachment 8378893 [details] [diff] [review]
Fixed mentioned errors, added comments and localized strings.

Added onclick handler again to the userIcon div.
Attachment #8378480 - Attachment is obsolete: true
Attachment #8378480 - Flags: review?(aleth)
Attachment #8378893 - Flags: review?(aleth)
(Assignee)

Comment 11

4 years ago
Created attachment 8379668 [details] [diff] [review]
Fixed mentioned errors, added comments and localized strings

The remove-icon from the menu item now disables if no user icon is set.
Attachment #8378893 - Attachment is obsolete: true
Attachment #8378893 - Flags: review?(aleth)
Attachment #8379668 - Flags: review?(aleth)
(Reporter)

Comment 12

4 years ago
Comment on attachment 8379668 [details] [diff] [review]
Fixed mentioned errors, added comments and localized strings

Review of attachment 8379668 [details] [diff] [review]:
-----------------------------------------------------------------

Drive by review of some nits. I'll let aleth handle the "real" review. :)

::: instantbird/content/blist.js
@@ +592,5 @@
>            this.userIconClick();
>          break;
>      }
>    },
> +  // Choose user icon using file picker.

Nit: "Choose the user icon"...

@@ +606,2 @@
>        Services.core.globalUserStatus.setUserIcon(fp.file);
> +      //enable the remove icon menu item if user icon is set.

Nit: Add a space after // and write in full sentences (e.g. "Enable the remove icon menu item if the user icon is set.")

@@ +610,4 @@
>    },
>        Services.core.globalUserStatus.setUserIcon(fp.file);
>    },
> +  // Removes the Buddy Icon.

Nit: Everywhere else this is called a "user icon" (in lowercase). Please be consistent!

@@ +615,2 @@
>      Services.core.globalUserStatus.setUserIcon(null);
> +    //disable the remove icon menu item if user icon is not set.

Nit: Again, please use a full sentence with a space after //.

::: instantbird/content/blist.xul
@@ -50,5 @@
> -    <menupopup id="buddyIcon">
> -      <menuitem label="Change"
> -                onclick="buddyList.userIconClick();" default="true"/>
> -      <menuitem label="Remove"
> -                onclick="buddyList.userIconRemove();"/>

Is this a diff against one of your previous revisions? I'm confused at this...

Comment 13

4 years ago
Comment on attachment 8379668 [details] [diff] [review]
Fixed mentioned errors, added comments and localized strings

Review of attachment 8379668 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think this patch will have to correct behaviour after a restart: Is the menu item correctly enabled/disabled?

Please don't forget yahoo - you can feedback?qheaden to discuss how best to do it.

::: chat/components/public/imIUserStatusInfo.idl
@@ +25,5 @@
>    void setStatus(in short aStatus, in AUTF8String aMessage);
>  
> +  /* Will fire an user-icon-changed notificaton.
> +     * If called with a parameter the function sets an icon for the user.
> +     * if null is passed as a parameter the user icon is removed.

How about "Sets the user icon, or removes it if null is passed as a parameter. Will fire a user-icon-changed notification."?

Please make sure the stars align in the comment ;) (I realize you were using them as bullet points, but see the last comment in this file -- it looks odd together with that convention.)

@@ +31,3 @@
>    void setUserIcon(in nsIFile aIconFile);
>  
> +  /* Get the location of the icon set by the user. */

"...of the current user icon" is shorter.

::: chat/components/src/imCore.js
@@ +163,5 @@
>      this._notifyObservers("status-changed", aMessage);
>    },
>  
> +  // Set icon for the user.
> +  // * Set the filename currently picked from filepicker

Please remove this line - this is a core file, it knows nothing about file pickers ;)

@@ +164,5 @@
>    },
>  
> +  // Set icon for the user.
> +  // * Set the filename currently picked from filepicker
> +  // * After copying the new file (newName) replace it with the oldFileName

This should be an inline comment -- if you want to comment on how something is done, it goes above the corresponding code where it happens.

@@ +166,5 @@
> +  // Set icon for the user.
> +  // * Set the filename currently picked from filepicker
> +  // * After copying the new file (newName) replace it with the oldFileName
> +  // * If null is passed as a parameter to setUserIcon() then icon is
> +  //   removed and set to default one.

You can remove this (it's in the IDL now).

@@ +204,5 @@
>      this._notifyObservers("user-icon-changed", newName);
>    },
> +
> +  // Get the curent user icon.
> +  // * If icon  has not been set, return null

You don't need this - there's already an inline comment next to the return null.
If you want to explain the expected behaviour, this comment should be in the IDL.

@@ +207,5 @@
> +  // Get the curent user icon.
> +  // * If icon  has not been set, return null
> +  // * If file is not found,  return null
> +  // * else
> +  //     return the filename

I don't think these comments add anything that isn't self-evident from the code.

::: instantbird/content/blist.js
@@ +606,2 @@
>        Services.core.globalUserStatus.setUserIcon(fp.file);
> +      //enable the remove icon menu item if user icon is set.

Nit: Space after // and (usually) start with a capitalized letter.

Do you need to check whether the user icon was indeed set correctly (i.e. what happens when you select an invalid file in the file picker?)

@@ +610,4 @@
>    },
>        Services.core.globalUserStatus.setUserIcon(fp.file);
>    },
> +  // Removes the Buddy Icon.

user icon

@@ +615,2 @@
>      Services.core.globalUserStatus.setUserIcon(null);
> +    //disable the remove icon menu item if user icon is not set.

Drop the "if..." (after the previous line is run, it certainly isn't set ;) )

::: instantbird/content/blist.xul
@@ +134,2 @@
>                 aria-label="&userIcon.label;" tooltiptext="&userIcon.label;"
> +               onclick="if (event.button == 0) { buddyList.userIconClick(); }"

Nit: You don't need the {} around a single line.

::: instantbird/locales/en-US/chrome/instantbird/instantbird.dtd
@@ +128,5 @@
>  
>  <!ENTITY hideGroupTooltip              "Hide">
>  
> +<!-- Localization of Context Menu to add/remove user icon -->
> +<!ENTITY openChangeIconCmd.label      "Change">

Can we drop the "open" from all of these? I'm not sure what it is supposed to mean.

"Change" should be "Change..." as selecting the item opens a dialog. Note there is a special character for such an ellipsis.

Does "Change" make sense if no user icon is currently set? Maybe "Set" would be clearer.

::: purple/purplexpcom/src/purpleAccount.cpp
@@ +979,5 @@
>    // If should resize
>    //   Ask libpurple for the desired new size
>    // Get the image data
>    // Set the icon
> +  // * If null is passed instead of fileURL, the user icon is removed.

This is confusing. ApplyCurrentUserIcon has no parameters.
Attachment #8379668 - Flags: review?(aleth) → review-
(Assignee)

Comment 14

4 years ago
In the code at 
http://mxr.mozilla.org/comm-central/source/chat/protocols/yahoo/yahoo-session.jsm#312
I'm adding a remove user icon feature I want to know whether this supports user icon removal in any way?
Flags: needinfo?(qheaden)
(Reporter)

Comment 15

4 years ago
(In reply to Mayank Kumar [:mayanktg] from comment #14)
> In the code at 
> http://mxr.mozilla.org/comm-central/source/chat/protocols/yahoo/yahoo-
> session.jsm#312
> I'm adding a remove user icon feature I want to know whether this supports
> user icon removal in any way?

For reference: http://lxr.instantbird.org/instantbird/source/purple/libpurple/protocols/yahoo/yahoo_picture.c#542
(Assignee)

Comment 16

4 years ago
Created attachment 8380189 [details] [diff] [review]
Fixed mentioned errors, added comments and localized strings.
Attachment #8379668 - Attachment is obsolete: true
Attachment #8380189 - Flags: review?(aleth)

Comment 17

4 years ago
Comment on attachment 8380189 [details] [diff] [review]
Fixed mentioned errors, added comments and localized strings.

Review of attachment 8380189 [details] [diff] [review]:
-----------------------------------------------------------------

You didn't address "I don't think this patch will have to correct behaviour after a restart: Is the menu item correctly enabled/disabled?". The best way to fix this is to set the disabled flag in an onpopupshowing handler.

::: instantbird/content/blist.js
@@ +592,5 @@
>            this.userIconClick();
>          break;
>      }
>    },
> +  // Choose user icon using.

I don't think you wanted to delete the end of this sentence.

@@ +610,4 @@
>    },
>        Services.core.globalUserStatus.setUserIcon(fp.file);
>    },
> +  // Removes the user Icon.

Nit: don't capitalize icon ;)

@@ +615,2 @@
>      Services.core.globalUserStatus.setUserIcon(null);
> +    //disable the remove icon menu item if user icon is not set.

See previous review comments on commenting style.

::: instantbird/content/blist.xul
@@ +134,3 @@
>                 aria-label="&userIcon.label;" tooltiptext="&userIcon.label;"
> +               onclick="if (event.button == 0)
> +                          buddyList.userIconClick();"

Please keep this on a single line. If this was in JS code block, what you did would be absolutely correct, but because we are here including a short fragment of code as a string, we prefer to keep it compact.

::: instantbird/locales/en-US/chrome/instantbird/instantbird.dtd
@@ +128,5 @@
>  
>  <!ENTITY hideGroupTooltip              "Hide">
>  
> +<!-- Localization of Context Menu to add/remove user icon -->
> +<!ENTITY setIconCmd.label             "Set..">

Please ask if you didn't understand what I meant about the ellipsis ;)

::: purple/purplexpcom/src/purpleAccount.cpp
@@ -984,1 @@
>  

Why did you remove this line?
Attachment #8380189 - Flags: review?(aleth) → review-
(Assignee)

Updated

4 years ago
Depends on: 976177
(Assignee)

Comment 18

4 years ago
Created attachment 8382264 [details] [diff] [review]
Fixed mentioned errors, added comments and localized strings.

::: purple/purplexpcom/src/purpleAccount.cpp
@@ -984,1 @@
>  
I removed the comment of this line with reference to this comment of flo 
http://log.bezut.info/instantbird/140219/#m234 as hi mentioned that this line has no preference which relates to the code. So I should rather remove it.
Attachment #8380189 - Attachment is obsolete: true
Attachment #8382264 - Flags: review?(aleth)
(Assignee)

Comment 19

4 years ago
Created attachment 8382703 [details] [diff] [review]
Changed according to the new c-c repo

I have changed it according to the new comm-central repository.
The purpleAccount.cpp file is no longer present.
Attachment #8382264 - Attachment is obsolete: true
Attachment #8382264 - Flags: review?(aleth)
Attachment #8382703 - Flags: review?(aleth)
Comment on attachment 8382703 [details] [diff] [review]
Changed according to the new c-c repo

Review of attachment 8382703 [details] [diff] [review]:
-----------------------------------------------------------------

::: chat/components/src/imCore.js
@@ +162,5 @@
>        this._statusText = aMessage;
>      this._notifyObservers("status-changed", aMessage);
>    },
>  
> +  // Set icon for the user.

I don't think this comment adds any value here. Thanks for the comments in imIUserStatusInfo.idl thought! :-)

@@ +199,5 @@
>  
>      this._notifyObservers("user-icon-changed", newName);
>    },
> +
> +  // Get the curent user icon.

Same here.

::: im/content/blist.js
@@ +592,5 @@
>            this.userIconClick();
>          break;
>      }
>    },
> +  // Choose user icon.

I don't see the value of this comment.

@@ +606,3 @@
>        Services.core.globalUserStatus.setUserIcon(fp.file);
> +      // Enable the remove icon menu item if user icon is set.
> +      document.getElementById("removeIconMenu…Item").removeAttribute("disabled");

I think the … here is a typo.

Also, I think aleth told you to handle disabling the menuitem in an onpopupshown handler (comment 17).

::: im/locales/en-US/chrome/instantbird/instantbird.dtd
@@ +128,5 @@
>  
>  <!ENTITY hideGroupTooltip              "Hide">
>  
> +<!-- Localization of Context Menu to add/remove user icon -->
> +<!ENTITY setIconCmd.label             "Set…">

As a user, I don't understand what "Set…" means. Why not "Set User Icon…"?

@@ +131,5 @@
> +<!-- Localization of Context Menu to add/remove user icon -->
> +<!ENTITY setIconCmd.label             "Set…">
> +<!ENTITY setIconCmd.accesskey         "S">
> +<!ENTITY RemoveIconCmd.label          "Remove">
> +<!ENTITY RemoveIconCmd.accesskey      "R">

removeIconCmd, not RemoveIconCmd.
Attachment #8382703 - Flags: review?(aleth) → review-

Comment 21

4 years ago
(In reply to Mayank Kumar [:mayanktg] from comment #19)
> I have changed it according to the new comm-central repository.
> The purpleAccount.cpp file is no longer present.

It will be back after bug 955009 lands, but in a separate repository holding libpurple.
Comment on attachment 8382703 [details] [diff] [review]
Changed according to the new c-c repo

Review of attachment 8382703 [details] [diff] [review]:
-----------------------------------------------------------------

I only have a nit regarding the labels (I hope it's constructive and not just bike-shedding).

I r-'ed the patch only for the reason that one part of the patch didn't apply correctly and that the line numbers at this place were off again. I think we need to find out what's going wrong here. Please contact me if you need help with that.

::: im/locales/en-US/chrome/instantbird/instantbird.dtd
@@ +128,5 @@
>  
>  <!ENTITY hideGroupTooltip              "Hide">
>  
> +<!-- Localization of Context Menu to add/remove user icon -->
> +<!ENTITY setIconCmd.label             "Set…">

Wouldn't "Choose image..." be less technical and therefore maybe more user friendly?
Attachment #8382703 - Flags: review-
(Assignee)

Comment 23

4 years ago
Created attachment 8383578 [details] [diff] [review]
Changed according to the new c-c repo part2

- Fixed the typos.

+++ b/im/content/menus.js
@@ -122,16 +122,22 @@ var menus = {
   },
 
   updateFileMenuitems: function menu_updateFileMenuitems() {
     goUpdateCommand("cmd_joinchat");
     goUpdateCommand("cmd_addbuddy");
     goUpdateCommand("cmd_newtab");
   },
 
+  updateuserIconContextMenuitems: function menu_updateuserIconContextMenuitems() {
+    if (Services.core.globalUserStatus.getUserIcon() == null) {
+      document.getElementById("removeIconMenuItem").setAttribute("disabled", "true");
+    };
+  },
+
I have added the onclick event handler to make the menupopup work correctly. Please look into it. It's working as expected after restart.
Attachment #8382703 - Attachment is obsolete: true
Attachment #8383578 - Flags: review?(aleth)
Comment on attachment 8383578 [details] [diff] [review]
Changed according to the new c-c repo part2

Review of attachment 8383578 [details] [diff] [review]:
-----------------------------------------------------------------

The patch is working fine already, let's try to fix the remaining coding style and naming issues. This might be tedious and less exciting than writing something new but I hope you understand that this is a necessary step when creating code that should be maintainable in the future :)
Be assured that you're doing a good job and seem to pick up the conventions quickly. I'm glad we figured out your repository problems already :)

::: im/content/blist.js
@@ +593,5 @@
>          break;
>      }
>    },
>  
> +  // Choose a user icon.

"I don't see the value of this comment." as flo said ;)

@@ +598,1 @@
>    userIconClick: function bl_userIconClick() {

This name might no longer be appropriate. |chooseUserIcon| or |selectUserIcon| maybe? Make sure to find and change all references to this method when renaming it. MXR will help you with that :)

@@ +607,5 @@
>        Services.core.globalUserStatus.setUserIcon(fp.file);
> +      // Enable the remove icon menu item if user icon is set.
> +      document.getElementById("removeIconMenuItem").removeAttribute("disabled");
> +    }
> +  },

You don't want to do this anymore. All enabling and disabling of the menu items should be done from within your "onpopshowing" handler. More on that in menus.js...

@@ +613,5 @@
> +  // Removes the user icon.
> +  userIconRemove: function bl_userIconRemove() {
> +    Services.core.globalUserStatus.setUserIcon(null);
> +    // Disable the remove icon menu item.
> +    document.getElementById("removeIconMenuItem").setAttribute("disabled", "true");

Same here.

::: im/content/blist.xul
@@ +48,5 @@
>      <tooltip id="buddyTooltip" type="buddy"
>               onpopupshowing="return !('_droptarget' in window);"/>
>  
> +    <menupopup id="userIconContextMenu"
> +               onpopupshowing="menus.updateuserIconContextMenuitems();">

We use camel case consistently, i.e. this needs to be "updateUserIconContextMenuItems"

@@ +50,5 @@
>  
> +    <menupopup id="userIconContextMenu"
> +               onpopupshowing="menus.updateuserIconContextMenuitems();">
> +      <menuitem id="changeIconMenuItem" label="&setIconCmd.label;"
> +                accesskey="&setIconCmd.accesskey;"

Please choose a consistent naming pattern here.

changeIconMenuItem,
changeIconCmd.label,
changeIconCmd.accesskey

setIconMenuItem,
setIconCmd.label,
... you get the idea?

@@ +51,5 @@
> +    <menupopup id="userIconContextMenu"
> +               onpopupshowing="menus.updateuserIconContextMenuitems();">
> +      <menuitem id="changeIconMenuItem" label="&setIconCmd.label;"
> +                accesskey="&setIconCmd.accesskey;"
> +                oncommand="buddyList.userIconClick();" default="true"/>

Or maybe something like "choose*" for the id's if you go for "chooseUserIcon" here?

@@ +55,5 @@
> +                oncommand="buddyList.userIconClick();" default="true"/>
> +      <menuitem id="removeIconMenuItem" label="&removeIconCmd.label;"
> +                accesskey="&removeIconCmd.accesskey;"
> +                oncommand="buddyList.userIconRemove();"/>
> +    </menupopup>

That's consistent! :)

::: im/content/menus.js
@@ +128,5 @@
>    },
>  
> +  updateuserIconContextMenuitems: function menu_updateuserIconContextMenuitems() {
> +    if (Services.core.globalUserStatus.getUserIcon() == null) {
> +      document.getElementById("removeIconMenuItem").setAttribute("disabled", "true");

else
  document.getElementById("removeIconMenuItem").removeAttribute("disabled");

Don't worry, trying to remove a non-existant attribute won't throw an error. That's why this code is sufficient to update your menu correctly all the time. Please insert line breaks as necessary to obey the 80-character line length limit. You can ask on the channel if you're unsure how we usally place them. 
No brackets around single-line if/else-blocks by the way!

@@ +129,5 @@
>  
> +  updateuserIconContextMenuitems: function menu_updateuserIconContextMenuitems() {
> +    if (Services.core.globalUserStatus.getUserIcon() == null) {
> +      document.getElementById("removeIconMenuItem").setAttribute("disabled", "true");
> +    };

The bracket is going to go away anyways but there's never a semi-colon after if-blocks by the way.

::: im/locales/en-US/chrome/instantbird/instantbird.dtd
@@ +129,5 @@
>  <!ENTITY hideGroupTooltip              "Hide">
>  
> +<!-- Localization of Context Menu to add/remove user icon -->
> +<!ENTITY setIconCmd.label             "Choose Image…">
> +<!ENTITY setIconCmd.accesskey         "S">

If you're referring to the "s" in "Choose", then please make this a lower-case "s". You could as well use "C" (upper-case) for example if you want to change that.
Attachment #8383578 - Flags: review?(aleth) → review-
(Assignee)

Comment 25

4 years ago
Created attachment 8384103 [details] [diff] [review]
Changed according to the new c-c repo
Attachment #8383578 - Attachment is obsolete: true
Attachment #8384103 - Flags: review?(aleth)
Comment on attachment 8384103 [details] [diff] [review]
Changed according to the new c-c repo

Review of attachment 8384103 [details] [diff] [review]:
-----------------------------------------------------------------

The next iteration (including the suggested changes) will be r+ :)

::: im/content/blist.js
@@ +593,5 @@
>          break;
>      }
>    },
>  
> +  chooseUserIcon: function bl_chooseUserIcon() {

Thanks!

@@ +606,5 @@
>        Services.core.globalUserStatus.setUserIcon(fp.file);
>    },
>  
> +  // Removes the user icon.
> +  userIconRemove: function bl_userIconRemove() {

Sorry that this slipped through the review last time: the comment here isn't needed either (the function name is self-explaining).

After the above rename to |chooseUserIcon|, this function name should be |removeUserIcon| now (like you already matched it before with |userIconClick| & |userIconRemove|).

::: im/content/menus.js
@@ +135,5 @@
> +    else
> +      // Enable the remove user icon menuitem.
> +      document.getElementById("removeIconMenuItem").
> +               removeAttribute("disabled");
> +  },

The comments here aren't needed either (the code is short and self-explanatory).

What about shortening the rest and getting around the line wraps with this?

let menuItem = document.getElementById("removeIconMenuItem");
if (Services.core.globalUserStatus.getUserIcon() == null)
  menuItem.setAttribute("disabled", "true");
else
  menuItem.removeAttribute("disabled");
Attachment #8384103 - Flags: review?(aleth) → review-
The NEEDINFO-part of this bug was spun off into bug 976177.
Flags: needinfo?(qheaden)
(Assignee)

Comment 28

4 years ago
Created attachment 8384131 [details] [diff] [review]
Changed according to the new c-c repo

Did the changes you mentioned :)
Attachment #8384103 - Attachment is obsolete: true
Attachment #8384131 - Flags: review?(benediktp)
Attachment #8384131 - Flags: review?(aleth)
Comment on attachment 8384131 [details] [diff] [review]
Changed according to the new c-c repo

Review of attachment 8384131 [details] [diff] [review]:
-----------------------------------------------------------------

I've got 3 trivial comments, but the patch looks really good otherwise! :-)

::: chat/components/public/imIUserStatusInfo.idl
@@ +29,3 @@
>    void setUserIcon(in nsIFile aIconFile);
>  
> +  // Get the location of the current user icon.

This needs to specify what the return value is when there's no icon.
Suggestion:
"Returns the location of the current user icon, or null if no icon is set."

::: im/content/menus.js
@@ +128,5 @@
>    },
>  
> +  updateUserIconContextMenuitems: function menu_updateUserIconContextMenuitems() {
> +    let menuItem = document.getElementById("removeIconMenuItem");
> +    if (Services.core.globalUserStatus.getUserIcon() == null)

Nit: We usually don't do |if (foo == null)| tests, we would instead do |if (!foo)|. In this specific case, you don't even need the ! because you can just remove the attribute if there's a value, and set it in the 'else' statement.

::: im/locales/en-US/chrome/instantbird/instantbird.dtd
@@ +127,5 @@
>  <!ENTITY contactDropTarget2            "Drop another contact here to merge them.">
>  
>  <!ENTITY hideGroupTooltip              "Hide">
>  
> +<!-- Localization of Context Menu to add/remove user icon -->

Nit: Remove the words "Localization of ". Everything in this file is to localize UI ;-).
Attachment #8384131 - Flags: review-
Attachment #8384131 - Flags: feedback+
Comment on attachment 8384131 [details] [diff] [review]
Changed according to the new c-c repo

Thanks for adressing my comments!
Attachment #8384131 - Flags: review?(benediktp) → review+
(Assignee)

Comment 31

4 years ago
Created attachment 8384133 [details] [diff] [review]
Changed according to the new c-c repo
Attachment #8384131 - Attachment is obsolete: true
Attachment #8384131 - Flags: review?(aleth)
Attachment #8384133 - Flags: review?(benediktp)
(In reply to aleth from comment #13)
> Do you need to check whether the user icon was indeed set correctly (i.e.
> what happens when you select an invalid file in the file picker?)

It's possible to select a wrong file on Windows (entering "*.*" in the file picker dialog will show all files in a folder) or a corrupted image file in general. The icon will then be "set" but not working of course (i.e. user icon placeholder no longer shown but no other image either).
This is an existing bug and not in the scope of this one in my opinion. We should make sure there's a bug report for it.
Comment on attachment 8384133 [details] [diff] [review]
Changed according to the new c-c repo

The remaining changes were in response to flo's comments. You can "carry forward" my r+ though (i.e. it is still valid on this patch).
Attachment #8384133 - Flags: review?(benediktp) → review?(florian)
Comment on attachment 8384133 [details] [diff] [review]
Changed according to the new c-c repo

Thanks!
Attachment #8384133 - Flags: review?(florian) → review+
Blocks: 955673

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/51cb8ddcfa18
Assignee: nobody → mayanktg
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
(Reporter)

Updated

4 years ago
Target Milestone: --- → 1.6
You need to log in before you can comment on or make changes to this bug.