341173 - Need hotkeys to assign label/tag to message - shortcut 0-5

Status: VERIFIED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Stephen Jennings]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.4) Gecko/20060508 Firefox/1.5.0.4
Build Identifier: version 3 alpha 1 (20060611)

With the transition from labels to tags, Thunderbird lost the 1-5 hotkeys for assigning labels.  I think many people rely on these hotkeys for the technique described at http://entropicprincipal.blogspot.com/2005/09/using-thunderbird-to-get-things-done.html

Reproducible: Always

Steps to Reproduce:
1. Select a message
2. Press a number key 1-5

Actual Results:  
None

Expected Results:  
Message should be tagged with a label/tag

Comment 1

[Wayne Mery (:wsmwk)]

*** Bug 341993 has been marked as a duplicate of this bug. ***

Comment 2

[Johannes Buchner]

Please also see the (now duped) Bug 341993, because it contains additional information.

Comment 3

[Wayne Mery (:wsmwk)]

Was the shortcut code removed as part of bug 114656? (Looks like it) Perhaps it was discussed somewhere other than in the bug. 

In restoring this capability, does there need to be a defined mapping for #<->label?  For portability could mapping be implicit through the label definition?
 0-blah0
 1-blah1
 etc.

I would greatly miss shortcuts and in fact, with the new label capability, would like to see this extended from 0-5 to 0-9.  For me and I expect others, it's usage is much broader than the example at http://entropicprincipal.blogspot.com/2005/09/using-thunderbird-to-get-things-done.html

Comment 4

[Alexander Kluss]

it's the same in

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/2006061910 SeaMonkey/1.5a

I couldn't set the labels with the keys 0-5.

Comment 5

[Karsten Düsterloh]

Attachment: SeaMonkey patch, v1

This SeaMonkey patch returns the old label shortcut keys 0-5 and extends them to 0-9, given that many labels exist...

@Scott/David: if you're interested in this solution, I could do a TB version as well...

Comment 6

[Scott MacGregor]

(In reply to comment #5)
> Created an attachment (id=226865) [edit]
> SeaMonkey patch, v1
> 
> This SeaMonkey patch returns the old label shortcut keys 0-5 and extends them
> to 0-9, given that many labels exist...
> 
> @Scott/David: if you're interested in this solution, I could do a TB version as
> well...

That would be most kind of you. I think we'd be interested in such a patch! Thanks.

Comment 7

[[e-mail]]

Comment on attachment 226865 [details] [diff] [review]
SeaMonkey patch, v1

>+        ToggleMessageTagCommand(Number(command.substr(-1, 1)));
Nit: Number() shouldn't be necessary, -- will convert for you.
Nit: substr(-1, 1) can be simplified to substr(-1)

>+  if (!gDBView)
>+    return;
>+  var msgHdr = gDBView.hdrForFirstSelectedMessage;
>+  if (!msgHdr)
>+    return;
Nit: hdrForFirstSelectedMessage throws if no message is selected although as the command is only enabled when at least one message is selected you shouldn't need any of these checks.

>+  var allKeys = tagService.keyEnumerator;
>+  // get the key at index
>+  var key = "";
>+  while ((--index >= 0) && allKeys.hasMore())
>+    key = allKeys.getNext();
>+  if (index < 0)
I think that this structure might be more readable:
while (allKeys.hasMore()) {
  var key = allKeys.getNext();
  if (!--index) {
    ...
    ToggleMessageTag(key, addKey);
    break;
  }
}

>+  menuseparator.previousSibling.setAttribute("hidden", !allTags.hasMore());
Nit: .hidden

>+  SetMessageTagLabel(menupopupNode.firstChild, tagLabelFormat, tagRemoveLabel);
Nit: Don't need this, label is static, put it in the .dtd instead.

>+    newMenuItem.setAttribute('oncommand', 'ToggleMessageTagMenu(event.target)');
Nit: as you're changing this, );');

>+  <command id="cmd_tag1" oncommand="goDoCommand('cmd_tag1');" disabled="true"/>
>+  <command id="cmd_tag2" oncommand="goDoCommand('cmd_tag2');" disabled="true"/>
>+  <command id="cmd_tag3" oncommand="goDoCommand('cmd_tag3');" disabled="true"/>
>+  <command id="cmd_tag4" oncommand="goDoCommand('cmd_tag4');" disabled="true"/>
>+  <command id="cmd_tag5" oncommand="goDoCommand('cmd_tag5');" disabled="true"/>
>+  <command id="cmd_tag6" oncommand="goDoCommand('cmd_tag6');" disabled="true"/>
>+  <command id="cmd_tag7" oncommand="goDoCommand('cmd_tag7');" disabled="true"/>
>+  <command id="cmd_tag8" oncommand="goDoCommand('cmd_tag8');" disabled="true"/>
>+  <command id="cmd_tag9" oncommand="goDoCommand('cmd_tag9');" disabled="true"/>
You don't use these.

>+        <menuitem id="threadPaneContext-tagRemoveAll"
>+                  accesskey="&tagCmd0.accesskey;"
>+                  command="cmd_tag0"/>
I wonder whether you should use oncommand="RemoveAllMessageTags();" instead.

>+<!ENTITY tagCmd0.accesskey "0">
The problem here is that the accesskeys 1-9 are hardcoded to decimal digits.

>+mailnews.tags.format=%A %L
If you use %1$S %2$S then you can use formatStringFromName instead.

Comment 8

[Karsten Düsterloh]

Attachment: SeaMonkey patch, v2

Addressed of Neil's comments, but:
even though the label text "Remove All Tags" is static, the format isn't and thus the label can't be put into the .dtd (I don't think you meant to put "0 Remove All Tags" into there?)

Comment 9

[[e-mail]]

Comment on attachment 227320 [details] [diff] [review]
SeaMonkey patch, v2

>+  // remove all tags by removing all their keys at once
>+  // (using a msgHdr's setStringProperty won't notify the threadPane!)
>+  if (gDBView)
>+    // XXX need to do all selected messages
>+    gDBView.db.setStringProperty(gDBView.keyForFirstSelectedMessage, "keywords", "");
gDBView should exist at this point because you can only get here a) from a tag submenu, which can only be opened when a message is selected or b) from goDoCommand('cmd_tag0') which also first checks that a message is selected.
I'm not so sure about gDBView.db though, what about virtual folders?

>+  document.commandDispatcher.updateCommands('create-menu-tag');
You don't actually need this; the popup can only be opened if you have a selection so you know that all the menuitems should be enabled. This was also true for labels, so the command protection was never actually needed.

>+  <command id="cmd_tag0" oncommand="goDoCommand('cmd_tag0');" disabled="true"/>
>+  <key id="key_tag0" key="&tagCmd0.key;" command="cmd_tag0"/>
The problem here is that the commands are only updated when a menu opens.
This means that you can't use the shortcuts until then. As discussed on IRC, switch the keys back to using goDoCommand directly. I'll leave it to you whether to keep cmd_tag0 or not, but don't disable it as per the above.

sr=me with the commands fixed and the db question resolved.

Comment 10

[Karsten Düsterloh]

Attachment: SeaMonkey patch, v3

Changes to v2:
- gDBView.db doesn't work for virtual folders, thus changing and rerequesting sr
- dropped superfluous command handling

Comment 11

[[e-mail]]

Comment on attachment 227630 [details] [diff] [review]
SeaMonkey patch, v3

>+  var menuItem = document.getElementById(menuItemId);
>+  if(menuItem)
>+      menuItem.setAttribute('label', customLabel);
Nit: Rather than fixing the indent from 4 to 2 spaces you just outdented the lines by two spaces; the third line needs to be reduced from 8 to 4 spaces. Also the second line could do with a space between if and (.

Comment 12

[Ian Neal]

Comment on attachment 227630 [details] [diff] [review]
SeaMonkey patch, v3

As per neil's comment.

Comment 13

[Karsten Düsterloh]

Attachment: Thunderbird patch, v1

Thunderbird version, including some resyncing I had in my tree David just did, too, in bug 341354. ;-)

Comment 14

[Karsten Düsterloh]

Attachment: SeaMonkey patch, v4

Slight updates for TB sync, but I can't reproduce comment #11...

Comment 15

[David :Bienvenu]

Comment on attachment 229532 [details] [diff] [review]
Thunderbird patch, v1

Karsten, can you submit an updated patch that doesn't conflict with my checkin for  bug 341354 ? thx!

Comment 16

[Karsten Düsterloh]

Attachment: Thunderbird patch, v2

Unbitrotted TB patch.

Comment 17

[Scott MacGregor]

Comment on attachment 229862 [details] [diff] [review]
Thunderbird patch, v2

Thanks for putting this patch together Karsten. It looks good! 

A couple comments / questions:

1) Do we need a dtd format string for the key + the label? Would we ever expect a localization to move the key after the label? Didn't the key always come first when we had labels?

2) In InitMessageTags:

+    curKeys += " $label" + msgHdr.label;

 I'm having a hard time visualizing when we would access $labelN in the curKeys string while we iterate over the list of known tags from the tag service. I'm not sure how the code before this change was doing that either now that I look at it :), but it must work?

3) The menu drop down from the tags toolbar button doesn't fill itself with values anymore. 
JavaScript error: chrome://messenger/content/mailWindowOverlay.js, line 642: men
useparator.previousSibling has no properties

I think it needs the new separator added to it here:

http://lxr.mozilla.org/mozilla/source/mail/base/content/mailWindowOverlay.xul#1873

4) Remove All Tags doesn't actually work for me if I have a message with several tags. We always leave one tag behind. 

5) I think the Remove Tags menu item should be right above the New Tag menu item in the popup, so there's only one separator in the menupopup and Add / Remove are grouped together.

Comment 18

[Karsten Düsterloh]

Attachment: SeaMonkey patch, v5

Updated to do key tagging for multiple selected messages.

Comment 19

[Karsten Düsterloh]

Attachment: Thunderbird patch, v3

Do RemoveAllTags for multiple messages, fix virtual folder issues in the tag toggle code.

Comment 20

[Karsten Düsterloh]

I forgot a |var| keyword in ToggleMessageTag in both SM v5 and TB v3; I'll fix this at check-in or if other fixes are necessary.

Comment 21

[Karsten Düsterloh]

> 1) Do we need a dtd format string for the key + the label? Would we ever
> expect a localization to move the key after the label? Didn't the key always
> come first when we had labels?

Each label had its own format string, but those were all the same. The format string is necessary for bidi/rtl environments.

> 2) In InitMessageTags:
> 
> +    curKeys += " $label" + msgHdr.label;
> 
>  I'm having a hard time visualizing when we would access $labelN in the
> curKeys string while we iterate over the list of known tags from the tag 
> service. I'm not sure how the code before this change was doing that either 
> now that I look at it :), but it must work?

My understanding was that this should migrate not-yet-migrated labels to tags?

> 3) The menu drop down from the tags toolbar button doesn't fill itself with
> values anymore.

I'll look into this.
 
> 4) Remove All Tags doesn't actually work for me if I have a message with
> several tags. We always leave one tag behind. 

I didn't see this yet; I wonder if this would be the msgHdr.label?

> 5) I think the Remove Tags menu item should be right above the New Tag menu
> item in the popup, so there's only one separator in the menupopup and Add /
> Remove are grouped together. 

I disagree:
- 'Add' adds just one tag, but 'Remove All' removes all tags. Grouping them suggests more commonality than there is, imo.
- When having more than nine tags, the '0 Remove All Labels' will look quite out of place if at the end.

But i can do that if you want me to...

Comment 22

[David :Bienvenu]

>My understanding was that this should migrate not-yet-migrated labels to tags?

Yes - we don't really migrate the message store's labels to tags explicitly (though we may do if we have to grow the tag storage in a message).

Should I wait until the other issues are investigated before reviewing, in particular 3) and perhaps 4)?

Comment 23

[Karsten Düsterloh]

Attachment: Thunderbird patch, v4

Changes:
- fixed tag toolbar button issue
- fixed issues caused by old, yet unmigrated labels

Comment 24

[Karsten Düsterloh]

Attachment: SeaMonkey patch, v6

Fixed issues with yet unmigrated labels.

Comment 25

[Scott MacGregor]

Comment on attachment 230370 [details] [diff] [review]
Thunderbird patch, v4

Thanks again for doing this Karsten. 

I may want to experiment / try out having the Remove All Tags at the bottom, but you shouldn't have to do that work, and it shouldn't stop us from landing what you've done already, so go ahead and check this in. I've also approved it for the 1.8.1 branch. 

Thanks again!

Comment 26

[[e-mail]]

Comment on attachment 230371 [details] [diff] [review]
SeaMonkey patch, v6

>+  var menuItem = document.getElementById(menuItemId);
>+  if (menuItem)
>+      menuItem.setAttribute('label', customLabel);
Nit: wrong indent on this line, should be 2*2 spaces, not 6

>+    msgHdr.folder.getMsgDatabase(msgWindow)
>+          .setStringProperty(msgHdr.messageKey, "keywords", "");
>+    msgHdr.label = 0; // remove legacy label
Would you mind removing the label first, so that it's removed before the notification for the tags? (How does this work if the message has a label and no real tags? Does the property notify anyway?)

>+    if (prevHdrFolder && prevHdrFolder != msgHdr.folder)
>+    {
>+      prevHdrFolder[toggler](messages, key);
>+      messages.Clear();
>+    }
>+    messages.AppendElement(msgHdr);
>+    prevHdrFolder = msgHdr.folder;
No need to check that prevHdrFolder is null because msgHdr.folder never is.
Also, you only need to update prevHdrFolder if it is different.

Comment 27

[Robert Kaiser]

Comment on attachment 230371 [details] [diff] [review]
SeaMonkey patch, v6

Looks like we want this for SeaMonkey 1.1, a=me for that.

Comment 28

[Karsten Düsterloh]

Both patrocles (TB) and hoshi (SM) are burning right now :-/; I'll check this all in tomorrow night. 

Comment 29

[Karsten Düsterloh]

Attachment: SeaMonkey patch, v7 [checked in on trunk and MOZILLA_1_8_BRANCH]

Updated to Neil's comments.

Comment 30

[Karsten Düsterloh]

Attachment: Thunderbird patch, v5 [checked in on trunk and MOZILLA_1_8_BRANCH]

Updated to Neil's comments.

Comment 31

[Adam Guthrie]

*** Bug 347013 has been marked as a duplicate of this bug. ***

Comment 32

[Mike Cowperthwaite]

It's a little offputting to have '1' not be "Important"; I don't care so much about preserving the rest of the ordering.  I suppose once the full UI for tag colors/labels is in place, I can just change tag#1 to be Important in Red.

It's not clear to me how the ordering is implemented (it's not alphabetical); 
on my system, the tags in order are: Personal, To Do, HTML, Important, Blimey, Work.  ("Blimey" was added just to have a new test tag; "HTML" was a string change I made to whatever the original fourth-position label was.  I expected 
it to be in the sixth position.)

Further, the precedence of which tag's color is used seems to be based on the most recent tag added, rather than which tag might be most important.  That's a different bug, I suppose, or an RFE.

That said, the number keys work as advertised.

Comment 33

[Karsten Düsterloh]

> It's not clear to me how the ordering is implemented (it's not alphabetical); 
> on my system, the tags in order are: Personal, To Do, HTML, Important, Blimey,
> Work.  ("Blimey" was added just to have a new test tag; "HTML" was a string
> change I made to whatever the original fourth-position label was.  I expected 
> it to be in the sixth position.)

The tag ordering is a system and profile specific, almost arbitrary result of the order the tag preferences are stored in the internal hash. See bug 342065.

Comment 34

[Mike Cowperthwaite]

*** Bug 361706 has been marked as a duplicate of this bug. ***