718139 - From tags dropdown, no easy way to edit/rename/change tag caption or delete tag from list of tags (idea: add "Manage tags..." menu to tags dropdown)

Status: RESOLVED FIXED

(Thunderbird :: Toolbars and Tabs, defect)

Description

[Ron]

User Agent: Mozilla/5.0 (Windows NT 6.0; rv:9.0.1) Gecko/20100101 Firefox/9.0.1
Build ID: 20111220165912

Steps to reproduce:

A while back I made up some 'New' tags by clicking on "New Tag...". 


Actual results:

Now I just want to change the name of a tag. No way! You can only click on "New Tag..." or "Remove All Tags". This is crazy.


Expected results:

Should be able with one 'click' to change the names of one of the tags or remove one of the tags; just like adding a "New Tag...". It's not rocket science.  It's ridiculous that there wasn't these options set up in the first place.

Comment 1

[WADA:World Anti-bad-Duping Agency]

(In reply to Ron from comment #0)
> Now I just want to change the name of a tag. No way!

"change the name of a tag" in Tb usually means "change label name assigned to existent tag" and is a "definition change of already defined tag".
This corresponds to next;
  A tag is defined like next in prefs.js.
    mailnews.tags.$label1.tag = Important
    mailnews.tags.tag-003.tag = Tag-003
  When tag-003 is added to a mail, it's saved like next.
    when local mail folder : X-Mozilla-Keys: tag-003
    when IMAP folder       : IMAP flag(keyword) of "tag-003" is stored for mail,
                             if server supports user defined flag(keyword)
  At Tools/Options/Display/Tag, change label name for tag(s),
  for example change label name for tag-003 to User-Defined-Tag-No-3.
    mailnews.tags.tag-003.tag = User-Tag-No-3
  "X-Mozilla-Keys: tag-003" in mail source is not changed,
  or stored flag(keyword) name of "tag-003" is not changed, 
  but Tag name shown at thread pane is changed from Tag-003 to User-Tag-No-3.

It sounds for me what you want is "change tag added to a mail to different tag by single click". Is it right?
If it's right, it's currently impossible, because functionality for it is not provided yet. You need to do both "remove already added tag(s) from mail(s)" and "add new tag to the mail(s)". And, "remove already added tags" is consecutive "remove already added single tag" actions, unless "remove already added all tags"(0:Remove All Tags) is usable.

Current "Tag" of Tb is successor of former "Label".
 name of feature:
  former "Label"      : Important  Work     Personal  To Do    Later
  corresponding "Tag" : $label1    $label2  $label3   $label4  $label5
 characteristics:
  former "Label": Only one of five labels can be assigned to a mail.
                  So, select a "Label" == remove previous one + add selected one
  "Tag"         : any tag is added. any tag is removed.
                  label name added to tag(shown at thread pane) can be changed.
"Independednt add-tag and remove-tag operation" is because of "any tag is indepedent and can be added/removed independently".

> Expected results:
> Should be able with one 'click' to change the names of one of the tags (snip)

IIRC, feature like next is already requested.
  Set of tags in which only one tag can be selected.
  This is for tag change by single action and is similar one to former "Label".
  e.g.
  tag = setA.tag1, setA.tag2, ... setA.tagN, ... setA.tagP
  when setA.tagP is selected, remove already added setA.tagN, add setA.tagP.
Please search B.M.O for such request.

By the way, current Tag/"New Tag" of context menu of mail(s) does next two things at same time - (1) Create a new tag, (2) Add the created tag to selected mails.
It may produce user's confusion.
I recommend you to create all required tags via Tools/Options/Display/Tag before you start to add Tb's tag to mail.

Comment 2

[Hashem Masoud]

(In reply to Ron from comment #0)
Check this addon: https://addons.mozilla.org/en-US/thunderbird/addon/tag-dialog

Comment 3

[:aceman]

I do not understand this bug report. You can change the names of the tags in Tools->Options->Display->Tags ... Edit . What else is needed here?

Comment 4

[Ludovic Hirlimann [:Usul]]

Ron ?

Comment 5

[Thomas D. (:thomas8)]

I'll morph this into something useful.
Reporter is right: it's far too difficult to rename or delete tags.
Digging into Tools > Options blabla... is not very intuitive nor efficient given that tags are right in front of you on the tags dropdown.

Suggestion:

What if we add a menu for "Manage tags..." to the Tags dropdown, like this:

New Tag...
Manage Tags...
---------------------
0 Remove All Tags...
---------------------
1 Important
...

Clicking on "Manage Tags..." takes you straight into
Tools > Options > Display > Tags

Comment 6

[:aceman]

That could work. Bwinton?

Comment 7

[Thomas D. (:thomas8)]

Attachment: Suggestion: Add "Manage Tags..." Menu on Tags dropdown

Blake, adding a simple "Manage Tags..." Menu to Tags dropdown (as a shortcut to Tools > Options > Display > Tags) would make access to tag management significantly easier and more intuitive than it is now.
If you like it, ui-review+ is also welcome.

Comment 8

[Thomas D. (:thomas8)]

Low cost, high benefit:

Suggested UI (add "Manage tags..." menu) will...
- improve ux-discovery (currently very hard to find that)
- improve ux-efficiency (currently very many clicks for simple task like renaming tag, see comment 0)
- improve feeling of ux-control, e.g. to fix a spelling mistake after creating new tag from the same place where you added it, instead of feeling lost and angry like comment 0 that there is no way to rename/delete from tags dropdown, which is only primary UI for tags.

Comment 9

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 627504 [details]
Suggestion: Add "Manage Tags..." Menu on Tags dropdown

Yeah, this seems like a thing we can do, and it feels like it would fit in to the rest of the items in that menu.

Comment 10

[Thomas D. (:thomas8)]

Comment on attachment 627504 [details]
Suggestion: Add "Manage Tags..." Menu on Tags dropdown

Blake, I read your feedback+ on this one as ui-review+ for the new menu entry "Manage Tags" on tags dropdown, pls confirm so that we'll send the right vibes to folks who want to pick this little thing up.

Comment 11

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 627504 [details]
Suggestion: Add "Manage Tags..." Menu on Tags dropdown

Sure, although any given patch should also get ui-reviewed, to make sure we handle things like appropriate keyboard accellerators, using the correct "…" character, etc, etc…  (And I'm not totally sure whether I like the wording "Manage Tags…" or not.  Something about it feels kinda weird, but that might just be the coffee talking.)

Thanks,
Blake.

Comment 12

[:aceman]

OK, let's see if I can finally make openOptionsDialog() work :)

Comment 13

[:aceman]

Attachment: patch

This mostly works for me. It opens the Preferences window on the Display pane, but does not open the Tags tab. I don't know why, it seems the aTabID of openOptionsDialog() does not work (there doesn't seem to be any use in TB), or I did not add the tab ID properly.

It adds the Manage Tags item to all the tag menus I could find:
mail context menu
Messages -> Tags main menu
main toolbar Tag icon
message toolbar icon

Did I miss any?

Comment 14

[:aceman]

Is this applicable to Seamonkey in any way? Would SM want it?

Comment 15

[Ian Neal]

(In reply to :aceman from comment #14)
> Is this applicable to Seamonkey in any way? Would SM want it?

SM already have a "Customize…" menu item in the Tags dropdown, that takes the user to Tags Pref Pane ;)

Comment 16

[Thomas D. (:thomas8)]

Ludo, who could find out why calling openDialog with aTabID fails (opens pane, but does not open the right tab on the pane)? It's relevant for this bug and bug 525905, so already two places in our UI are affected. I don't think it's a good idea to keep adding links to option tabs that won't open reliably, so we really need someone to look into this.

This is what aceman correctly calls:
> +        <tab id="tagTab" label="&itemTags.label;"/>
> ...
> +  let dialog = window.openOptionsDialog("paneDisplay", "tagTab");

And this is where it goes:

> function openOptionsDialog(aPaneID, aTabID)
>  ...
>  openDialog("chrome://messenger/content/preferences
>              /preferences.xul","Preferences", features, aPaneID, aTabID);

From there, I get lost.
-------------------------------------------
(In reply to :aceman from comment #13)
> Created attachment 627789 [details] [diff] [review]
> patch

Thank you! :)

> This mostly works for me. It opens the Preferences window on the Display
> pane, but does not open the Tags tab. I don't know why, it seems the aTabID
> of openOptionsDialog() does not work (there doesn't seem to be any use in
> TB), or I did not add the tab ID properly.

I suspect you did everything right, but it doesn't work.
Bug 525905 is same problem for attachment keywords (only they don't even try to open the tab, but you do).

> It adds the Manage Tags item to all the tag menus I could find:
> Did I miss any?

I think not.

Comment 17

[Thomas D. (:thomas8)]

aceman, about tab not opening, can you test the following:

There are two cases of openDialogue (see below).
a) will never call aTabID (and failure is acknowledged in code), but for
b) in theory it should work.
Maybe we always end up in a) because there's a hidden instance of preference window always open or something like that? Can you test if you end up in a) or b), and if b) works if called directly?

a) when dialogue is already open, aTabID will not be called, and there's a comment in the code to acknoledge that failure:

http://mxr.mozilla.org/comm-central/source/mail/base/content/mailCore.js#412
> // I don't know how to support aTabID for an arbitrary panel when the dialog is
> already open. This is complicated because showPane is asynchronous (it could
> trigger a dynamic overlay) so our tab element may not be accessible right
> away...

b) when dialogue is not yet open, it's

> openDialog("chrome://messenger/content/preferences
>             /preferences.xul","Preferences", features, aPaneID, aTabID);

So that at least should work, unless perhaps there's similar problem like a) lateron in the code. But it's not easy for me to follow that code...

Comment 18

[:aceman]

Yes, I could try to play with that a little.
Correct function of opening specific tab is also needed for bug 360891 and I think I've seen one more (concerning a link from the Account manager).

(In reply to Ian Neal from comment #15)
> SM already have a "Customize…" menu item in the Tags dropdown, that takes
> the user to Tags Pref Pane ;)
Thanks, I see now. Maybe SM's goPreferences() does work correctly :)

Comment 19

[:aceman]

Attachment: patch v2

So for me the second branch is always taken (no hidden pref window open):
openDialog("chrome://messenger/content/preferences/preferences.xul", "Preferences", features, aPaneID, aTabID);
But not even the proper pane is selected.

It seems the selection function is completely unimplemented.
I looked into preferences.js and the other pref files and I can't find anything looking like code to select the proper pane (passed pane ID).

There is "showPane" in http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/content/widgets/preferences.xml#724. There is code in preferences.js to call it for some Chat stuff so calling showPane is intended to work.

So I try to implement the general pane selection in this patch like this:
1. I think there should only be 1 argument after "features" in the openDialog call. (see other calls in the tree)
2. I put those 2 IDs into a passed object.
3. Then in preferences.js I can see this object and select the wanted pane via showPane.
4. Inside display.js I also see the object and select the proper tab via .selectedIndex.

It seems to work for me. Patch to be applied on top of bug 758911.

Comment 20

[:aceman]

Yes, if this is accepted I need to fix some callers. But there are few:
http://mxr.mozilla.org/comm-central/search?string=openOptionsDialog&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central

Comment 21

[:aceman]

And with this implementation the other panes need the tab selection code too. I can do it generally in some new bug or in the bugs that depend on this.

Comment 22

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 628101 [details] [diff] [review]
patch v2

I like this addition.  ui-r=me!

Thanks,
Blake.

Comment 23

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 628101 [details] [diff] [review]
patch v2

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

aceman:

This looks like the right idea. There are some cleanup-y things I could recommend, but I can take care of that during r?.

-Mike

::: mail/components/preferences/display.js
@@ +50,5 @@
> +      if (preference.value)
> +        document.getElementById("displayPrefs").selectedIndex = preference.value;
> +    } else {
> +      // select the specified tab
> +      document.getElementById("displayPrefs").selectedIndex =

You can probably use selectedPanel here instead of selectedIndex, and just pass the panel node as opposed to finding / passing the node's index.

Comment 24

[:aceman]

Attachment: patch v3

It seems the other callers are OK, API of openOptionsDialog() is not changed.
.selectedPanel did not work correctly, but .selectedTab works.

Comment 25

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 630316 [details] [diff] [review]
patch v3

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

::: mail/base/content/mailCore.js
@@ +349,5 @@
>    window.openDialog("chrome://messenger/content/importDialog.xul", "importDialog",
>                      "chrome, modal, titlebar, centerscreen");
>  }
>  
> +/*

Nit: Function header documentation should start like this:

/**

::: mail/components/preferences/preferences.js
@@ +25,5 @@
>    }
> +
> +  // select the specified preferences pane
> +  if (windowArgs.paneID) {
> +    prefWindow.showPane(document.getElementById(windowArgs.paneID));

It looks like you're deferring the handling of tabID to the individual prefpanes, right?

So I guess this is broken for prefpanes that are NOT display.xul/.js?

I'm not wild about that. What if you take care of loading the tabID in here as well, by attaching an event listener for paneload?  https://developer.mozilla.org/en/XUL/prefpane#Events

Comment 26

[:aceman]

Yes, the tab loading only works for display.xul for now. I did it because display.xul would set the tab itself according to the pref.
But yes, I can try to set the tab here and then the individual panels just do not set the tab if tabID was sent (they see the arguments).
But I do not yet know how to use an event listener...
Also, each pane has a different ID on it's tabbox element. But maybe I can just grab the first tabbox regardless of ID.

Comment 27

[:aceman]

This needs more work, there is a caller at http://mxr.mozilla.org/comm-central/source/mail/components/nsMailDefaultHandler.js#274 that does not use openOptionsDialog() but opens preferences.xul as a window. Maybe that one should be migrated too.

And some of the pref panes already use window.arguments[0] (http://mxr.mozilla.org/comm-central/search?string=arguments&find=%2Fmail%2Fcomponents%2Fpreferences&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central) to accept special arguments. I do not know if that ever works, but I can update mu patch to preserve this (just up it to arguments[1] and make openOptionsDialog() to pass a 3rd argument).

Comment 28

[:aceman]

Attachment: patch v4

This seems to work better, also handles the case the dialog is already open.

Comment 29

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 631575 [details] [diff] [review]
patch v4

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

This looks mostly good - see my notes below.

Also - you've prepped a good number of the panes in the preferences dialog so that we can open by pane and tab - that's great. But what about the attachment (applications) pane?

-Mike

::: mail/base/content/mailCore.js
@@ +356,5 @@
> + * @param aPaneID    ID of prefpane to select automatically.
> + * @param aTabID     ID of tab to select on the prefpane.
> + * @param otherArgs  other prefpane specific arguments
> + */
> +function openOptionsDialog(aPaneID, aTabID, otherArgs)

nit: that should be aOtherArgs

::: mail/base/content/mailWindowOverlay.xul
@@ +678,5 @@
>                    label="&addNewTag.label;"
>                    accesskey="&addNewTag.accesskey;"
>                    oncommand="AddTag();"/>
> +        <menuitem id="manageTags" label="&manageTags.label;" accesskey="&manageTags.accesskey;"
> +                  oncommand="ManageTags();"/>

We're side-stepping the command pattern here by using oncommand.

So, one thing we'll want to do in the future is a Test Pilot study to determine what things people click on and how often. In order to make that easier, it's necessary for us to use the command pattern where possible.

See https://developer.mozilla.org/en/XUL_Tutorial/Commands

::: mail/components/preferences/display.js
@@ +181,5 @@
>    },
>  
>    buildTagList: function()
>    {
> +    var tagArray = MailServices.tags.getAllTags({});

let instead of var

@@ +238,2 @@
>  
> +  var item = gDisplayPane.appendTagItem(aName, MailServices.tags.getKeyForTag(aName), aColor);

let instead of var

::: mail/components/preferences/preferences.js
@@ +47,5 @@
> +    // automatically. But let's check it and if the prefs window was already
> +    // open, the current prefpane may not be the wanted one.
> +    if (prefWindow.currentPane.id != prefPane.id) {
> +      if (aTabID && !prefPane.loaded) {
> +        prefPane.addEventListener("paneload", function() { showTab(prefPane, aTabID); });

You'll want to remove the listener once the event gets fired once.

function() {
  prefPane.removeEventListener("paneload", arguments.callee);
  showTab(prefPane, aTabID);
}

Comment 30

[:aceman]

Attachment: patch v5

So hopefully I got the 'command' stuff right. It added about 10KB of patch :) But looks like nice de-duplication of function calls.

Comment 31

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 634159 [details] [diff] [review]
patch v5

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

Great patch, and thanks for doing the command stuff. Looks good.

With the small nits I found fixed, r=me.

-Mike

::: mail/base/content/mailWindowOverlay.js
@@ +633,5 @@
>  }
>  
> +function ManageTags()
> +{
> +  window.openOptionsDialog("paneDisplay", "tagTab");

I think "window." is implied if we call openOptionsDialog on its own, and can be omitted.

::: mail/base/content/mailWindowOverlay.xul
@@ +297,5 @@
>      <command id="cmd_viewNormalHeader" oncommand="goDoCommand('cmd_viewNormalHeader');" disabled="true"/>
>  </commandset>
>  
> +<commandset id="mailTagMenuItems"
> +              commandupdater="true"

Lines 301-303 should be lined up with the id=... in line 300.

::: mail/components/preferences/preferences.js
@@ +8,2 @@
>  window.addEventListener("load", function () {
> +  /*

Nit - should be /**

Comment 32

[:aceman]

Attachment: patch v6

Thanks, done.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/52f7c3820faa