Closed Bug 660165 Opened 10 years ago Closed 10 years ago

Keyboard shortcuts for tags are always effective

Categories

(Thunderbird :: Mail Window Front End, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 7.0

People

(Reporter: protz, Assigned: squib)

Details

Attachments

(1 file)

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"?
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.
This should be pretty easy to do. Taking.
Assignee: nobody → squibblyflabbetydoo
Status: NEW → ASSIGNED
This should fix things. I also added some tests for it.
Attachment #540003 - Flags: review?(jonathan.protzenko)
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.
Attachment #540003 - Flags: review?(jonathan.protzenko) → review+
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. :)
Checked in: http://hg.mozilla.org/comm-central/rev/845cb720fef3
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 7.0
You need to log in before you can comment on or make changes to this bug.