660165 - Keyboard shortcuts for tags are always effective

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Jonathan Protzenko [:protz]]

STR:
- select a thread in the thread pane,
- open a chrome tab (say, a conversation tab with Thunderbird Conversations©),
- hit 1,
- close the chrome tab.

Results:
- the thread has been tagged through the keyboard shortcut

Expected results:
- the thread has not been tagged

This is very confusing, and other keyboard shortcuts are not effective unless there's a currently focused message. If you hit the M key, for instance, it doesn't mark the thread as read/unread.

ToggleMessageTagKey and RemoveAllMessageTags from mailWindowOverlay.js should both check that the current tab is a mail:3pane one (glodaSearch, message, or folder tab types), and that it makes sense to change the selected message's tags.

As to why this command is not disabled, I think it doesn't go through the command mechanism that tells whether a given command is enabled in the current context, unlike cmd_markAsRead for instance.

Question:
- does anyone have an easy, simple way to check for "this is a mail:3pane default tab disguised as either a message tab, or a glodaSearch tab"?

Comment 1

[Mark Banner (:standard8)]

I would say that the preferred solution is to make the keys go from the command mechanism if that's possible. The reason is that extensions may add different tab types (e.g. to display messages in different ways) but still want to use the number keys for toggling tags. If we hard-code only certain tab types, then extensions can't do that easily.

Comment 2

[Jim Porter (:squib)]

This should be pretty easy to do. Taking.

Comment 3

[Jim Porter (:squib)]

Attachment: Use proper command dispatching

This should fix things. I also added some tests for it.

Comment 4

[Jonathan Protzenko [:protz]]

Comment on attachment 540003 [details] [diff] [review]
Use proper command dispatching

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

The test passes just fine, although I had a constantly reproducible python error with mozmill 1.4, when running the test. I had to upgrade mozmill to the latest version with sid0's excellent directions, and now the python error is gone. Which version are you running? I'd suggest a try-build or at least a careful monitoring of the tree after landing :-). Apart from that, a very fine patch, as usual. Thanks! r=me with the nits addressed.

::: mail/base/content/mail3PaneWindowCommands.js
@@ +817,5 @@
> +      case "cmd_tag6":
> +      case "cmd_tag7":
> +      case "cmd_tag8":
> +      case "cmd_tag9":
> +        var tagNumber = parseInt(command[7]);

Nice!

::: mail/base/content/mailWindowOverlay.js
@@ +560,2 @@
>  {
> +  let msgHdr = gFolderDisplay.selectedMessage;

Please add a comment explaining why msgHdr will always be non-null (because the command mechanism already checks for that).

@@ +560,3 @@
>  {
> +  let msgHdr = gFolderDisplay.selectedMessage;
> +  let tagService = Components.classes["@mozilla.org/messenger/tagservice;1"]

Please use MailServices.tags from from mailnews/base/util/mailServices.js

@@ -562,5 @@
> -  var tagArray = tagService.getAllTags({});
> -  for (var i = 0; i < tagArray.length; ++i)
> -  {
> -    var key = tagArray[i].key;
> -    if (!--index)

This was completely ridiculous. Thanks for rewriting that.

Comment 5

[Jim Porter (:squib)]

I'm on Mozmill 1.4.2b2. Here's a try server run, which will hopefully go well: http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=4a9f52325d20

I've addressed the nits in my local patch and everything still works fine, so once the try build passes, I'll push it.

(In reply to comment #4)
> ::: mail/base/content/mailWindowOverlay.js
> @@ +560,2 @@
> >  {
> > +  let msgHdr = gFolderDisplay.selectedMessage;
> 
> Please add a comment explaining why msgHdr will always be non-null (because
> the command mechanism already checks for that).

Since I just realized I had deleted the check for no selected messages from that function, I added it back on the off chance that an extension calls that function without a guarantee that there's a selected message.

> @@ +560,3 @@
> >  {
> > +  let msgHdr = gFolderDisplay.selectedMessage;
> > +  let tagService = Components.classes["@mozilla.org/messenger/tagservice;1"]
> 
> Please use MailServices.tags from from mailnews/base/util/mailServices.js

Fixed.

> @@ -562,5 @@
> > -  var tagArray = tagService.getAllTags({});
> > -  for (var i = 0; i < tagArray.length; ++i)
> > -  {
> > -    var key = tagArray[i].key;
> > -    if (!--index)
> 
> This was completely ridiculous. Thanks for rewriting that.

Yeah, that was some filthy code. I'm resisting the urge to clean up other parts of that file, but maybe we'll get to do that when some of this code gets moved to JSMs. :)

Comment 6

[Jim Porter (:squib)]

Checked in: http://hg.mozilla.org/comm-central/rev/845cb720fef3