525890 - Missing (context) menu: Message > "Show in conversation" (to view / open message in new thread pane view); add keyboard shortcut Ctrl+Shift+O for "Open in Conversation"

Status: RESOLVED FIXED

(Thunderbird :: Search, defect)

Description

[Thomas D. (:thomas8)]

TB3 introduces a new message-related command, "Show in conversation". Which is a really cool feature, but it's hidden in the akward depths of "other actions" button on header pane. We should really expose this valuable command in some main menu, too.

STR
1 look for a sane way of accessing the useful "Show in conversation" command via keyboard

Actual result

2 there is no sane way of accessing "Show in conversation" via keyboard
- other actions button is not really keyboard-accessible, only via tab
- there is no such context menu entry for the msg in the msg list
- there is no main menu entry for this command

Expected result

- there should be a context menu entry for this command (in context menu of msg in msg list):
Open Message in New Window
Open Message in New Tab
Show Message in Conversation

- there should be a main menu entry for this command (most likely in Message menu):
Message > Show in Conversation

Implementation Details:

- the main menu entry has [no l10n impact], because "Show in conversation" string already exists; this just takes a single line to fix

- the context menu entry has [l10n impact], because "Show Message in Conversation" string doesn't exist yet.

Comment 1

[Steve McMillen]

Actually, "Show in Conversation" is missing *Entirely* from latest builds. :( :(*

I really liked this feature.  I hope it comes back before release!!

Comment 2

[Steve McMillen]

Never mind.  I had the wrong version of Thunderbird installed.   Its there and working :)

Comment 3

[[e-mail]]

+1 for this bug. I've been using TB3 since the early betas, and only just discovered this feature because I realized that the search results are displaying the conversation, and that any sane programmer that made that feature would want to access it another way. 

It really, really needs to be surfaced since it's such a useful trick. Perhaps an entry in the right-click menu would help as well as an entry in the Message Menu.

Comment 4

[Ilja Sekler]

This would be IMHO the most useful and at the same time low-cost enhancement for Thunderbird on Linux as well.

Comment 5

[Thomas D. (:thomas8)]

This is a cool new feature of TB3, and as such should be exposed rather than hidden in "other actions". It's also a really low-hanging fruit.
--> blocking thunderbird 3.1?

Another strong use case (including mouse users) showing the need for this:

STR

want to show multiple unrelated mails in conversation

Actual

for every mail, need to do these steps:
1 left-click to select msg in msg list
2 move mouse to preview's "Other actions" button (that's if you already know that the command is in there, which isn't obvious, at all)
3 click "Other actions button"
4 from other actions menu, click "Show in conversation"
That's 3 clicks and a very unconfortable and completely un-intuitive mouse move in between.

Expected

for every mail, these steps:
1 right-click on msg in msg list
2 click "Show in conversation" from context menu
That's 2 clicks in-place, and completely intuitive.

Caveat for implementation:

a) "Show in conversation" should also be enabled for multiple selected msgs, both from Msg Context Menu and Message Main Menu (just iterate through selected msgs to open the conversation for each msg in a new tab)
(For a quick 'n easy fix, we could disable "Show in conversation" when multiple messages are selected, until we can handle this)

b) at a later stage (followup bug), consider developing an intelligent logic where multiple selected msgs from the same conversation will only be returned in a single conversation tab.
For example, this would enable user to select a bunch of recently received Mozilla bug messages from inbox, and "Show in conversation" will show everything neatly (including sent msgs) with just one conversation (one bug thread) per tab (even if there are multiple scattered msgs for the same bug).

Comment 6

[Wayne Mery (:wsmwk)]

this would be very helpful indeed to the keyboard-centric user.

marcoz, do you know if high % of accessibility users tend to use context menus? Or do they have better affordances?

Comment 7

[Marco Zehe (:MarcoZ)]

Context menus are definitely a common way to access options available for a selected item. The other much used method is shortcut keys if available (and memorized). So yes, keyboard-centric users (and those using screen readers are such users) definitely use context menus a lot!

Comment 8

[Nehal M]

Attachment: Patch that adds the requested menus.

I have attached a patch that adds the menus.
You can access 'Show in Conversation' from the Message menu and use the Ctrl+Shift+S keyboard shortcut.
You can also right-click in the message list and you will see 'Show Message in Conversation' in the context menu.

The existing string from the header can't be used because 'show in conversation' is in lowercase. Locale updates will be needed obviously.

Note: Currently this only works for a single selected message. Would this really be useful for more than one?

Comment 9

[David :Bienvenu]

doubt we'd block, but it sounds wanted.

Comment 10

[Thomas D. (:thomas8)]

Bryan, some aspects for UI-review further down this comment.

(In reply to comment #8)
> Created an attachment 424459 [details] [diff] [review]
> Patch that adds the requested menus.

Nehal, thanks for providing the patch!

> You can access 'Show in Conversation' from the Message menu and use the
> Ctrl+Shift+S keyboard shortcut.

Nehal, did you discuss the keyboard shortcut with main developers?
Keyboard shortcuts are sometimes difficult to get green light for.
If the keyboard shortcut should turn out a problem, please provide a patch without adding the shortcut, so that we can get the valuable menus in.

+<!ENTITY showConversationCmd.key "s">

This looks wrong to me. What you wanted was [Ctrl+*Shift*+S], but I think this will only add [Ctrl+S], which is already in use for File > Save msg as > File.

Bryan, some aspects for UI-review:

1) "Show in conversation" is an important everyday command (frequently used)
-> "Show in conversation" should therefore have a keyboard shortcut
3) "Show in conversation" is another variant of "opening" the message
-> Keyboard shortcuts for "open" and "open in conversation" should therefore be similar:
[Ctrl+O] -> open msg
[Ctrl+Shift+O] -> open msg in conversation

Pls consider that most other locales apart from English will never ever see the string "Show in conversation", so [Ctrl+Shift+S] will NOT be an intuitive shortcut for the non-English-speaking world (e.g. German will have something like "Gesprächsfaden anzeigen" which doesn't start with the letter "s").
On the other hand, [Ctrl+Shift+O]will be an internationally intuitive shortcut because its relative [Ctrl+O] is very well known and conceptually similar.

> The existing string from the header can't be used because 'show in
> conversation' is in lowercase.

lowercase is arguably a dubious design decision given the fact that all other buttons have uppercase, just like all other apps on my OS. I never understood why header buttons are an exception. Maybe it makes them special, in much the same way that junk and archive buttons are "special" because of their missing icons.

> Locale updates will be needed obviously.

David or Marco, do we need to set some flag like "has l10n impact"?

> Note: Currently this only works for a single selected message. Would this
> really be useful for more than one?

Definitely yes, as pointed out by comment 5 a) and b). As a rule of thumb, I think every msg-related command should work on multiple selected messages unless there are strong reasons against it. But for this bug, I think it's better to first try and get the single-selection version in, we can still do for multiple msgs lateron.

Nehal, would you know how to patch this for multiple selected messages? Is it difficult to code?

Comment 11

[Nehal M]

> Nehal, did you discuss the keyboard shortcut with main developers?
> Keyboard shortcuts are sometimes difficult to get green light for.
> If the keyboard shortcut should turn out a problem, please provide a patch
> without adding the shortcut, so that we can get the valuable menus in.

No, I was not sure how to deal with this so I picked one that seemed suitable.

> +<!ENTITY showConversationCmd.key "s">
> 
> This looks wrong to me. What you wanted was [Ctrl+*Shift*+S], but I think this
> will only add [Ctrl+S], which is already in use for File > Save msg as > File.

Correct me if I'm wrong, but because I have modifiers="accel, shift", it will work with Ctrl+Shift+S. I did test it and if I remember, Ctrl+Shift+S works.

> Pls consider that most other locales apart from English will never ever see the
> string "Show in conversation", so [Ctrl+Shift+S] will NOT be an intuitive
> shortcut for the non-English-speaking world (e.g. German will have something
> like "Gesprächsfaden anzeigen" which doesn't start with the letter "s").
> On the other hand, [Ctrl+Shift+O]will be an internationally intuitive shortcut
> because its relative [Ctrl+O] is very well known and conceptually similar.
> 

Yes, of course. This never occurred to me. Ctrl+Shift+O seems more suitable.

> Definitely yes, as pointed out by comment 5 a) and b). As a rule of thumb, I
> think every msg-related command should work on multiple selected messages
> unless there are strong reasons against it. But for this bug, I think it's
> better to first try and get the single-selection version in, we can still do
> for multiple msgs lateron.
> 
> Nehal, would you know how to patch this for multiple selected messages? Is it
> difficult to code?

It should be easy.

So to confirm, I will implement this feature for multiple messages.
Should I change the ctrl+shift+S shortcut to ctrl+shift+O or remove it for now?
What about the accessKey entries? I picked 's' for the menu and 'n' for the popup ('s' is already taken by 'save as' in the popup); btw, 'o' is also unused for the popup. This is locale-dependent, so I think this should be OK.

Comment 12

[Thomas D. (:thomas8)]

Bryan, this needs your UI advice: How do we handle multiple selected msgs when user clicks "show in conversation"? I'm exploring some UI options for that situation below. Your comment will be appreciated (also on the keyboard shortcut Ctrl+Shift+O).

(In reply to comment #11)
> Correct me if I'm wrong, but because I have modifiers="accel, shift", it will
> work with Ctrl+Shift+S. I did test it and if I remember, Ctrl+Shift+S works.

Yes, you are right, thanks for helping me learn.

> Yes, of course. This never occurred to me. Ctrl+Shift+O seems more suitable.
> So to confirm, I will implement this feature for multiple messages.

I'm not sure about the desired behaviour. Probably different people will have different needs. There are at least three ways of doing "Show in conversation" for multiple selected messages:

a) for each selected msg, open a new tab and show it in conversation
b) group selected msgs that belong to the same conversation in a single new
   tab, otherwise open a new tab for each msg in conversation
c) show all conversations for all selected messages in a single new tab (like 
   in gloda search results, after "open as list")

Bryan and Nehal, maybe it's best and easiest to implement a) first, and discuss and implement more intelligent/complex behaviour later (Details and benefits below). I assume enabling the "Show in conversation" for multiple messages using simple behaviour a) is better than disabling it altogether. Bryan, what do you think?

> Should I change the ctrl+shift+S shortcut to ctrl+shift+O or remove it for 
> now?

Nehal, please try Ctrl+Shift+O and let's try to get ui-review from Bryan for that (comment 10 explains the benefits and the need for this shortcut).

> What about the accessKey entries? I picked 's' for the menu and 'n' for the
> popup ('s' is already taken by 'save as' in the popup); btw, 'o' is also 
> unused for the popup. This is locale-dependent, so I think this should be OK.

's' for main menu and 'n' for context menu should be ok for EN if they are free.

-----------------------------------------

Details - Behaviour options for multiple selected msgs > Show in conversation

a) new tab for every selected message in conversation

- this will be easiest to code,
- and probably acceptable as a default choice, because
- we actually do strictly as we're told: SHOW every msg in conversation. Whereas in b), we're not opening every msg for viewing, only one per conversation, with all other msg of that conversation just listed in the thread list
- use case for a) is in the feature: we're just enabling the same command for multiple selected msgs.

b) group selected msgs with one tab per conversation

- this will be more complex to code,
- intelligently and efficiently reduces the number of new tabs needed
- while still keeping different conversations apart in separate tabs
- could arguably be the more intelligent default choice (compromise between separating and grouping msgs)
- use case from comment 5 b):
> For example, this would enable user to select a bunch of recently received
> Mozilla bug messages from inbox, and "Show in conversation" will show
> everything neatly (including sent msgs) with just one conversation per tab
> (one bug thread per tab, even if selection has multiple scattered msgs for 
> the same bug).

c) all conversations for all selected msgs in single tab

- this will also be more complex to code (it's not the same as in gloda search results in list mode)
- radically reduces number of needed tabs to one (avoiding multiple tabs clutter)
- but conversations are less distinguishable
- IMO this should not be the default choice
- use case:
E.g. ctrl-select three single bug messages that relate to a similar topic like attachment handling, but from different threads. Click Show in Conversation. Get 3 related conversations in full in a single handy tab, and start navigating without clutter of dozens of tabs.

Comment 13

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to comment #12)
> Bryan, this needs your UI advice: How do we handle multiple selected msgs when
> user clicks "show in conversation"? I'm exploring some UI options for that
> situation below. Your comment will be appreciated (also on the keyboard
> shortcut Ctrl+Shift+O).

The ctrl+shift+o seems like it could work.  We might want to think about renaming the label to "Open in Conversation" though as it works more like "Open in tab" does.  The label "show in conversation" was our first pass at the feature and I'm not sure it should continue as is.

> (In reply to comment #11)
> > Yes, of course. This never occurred to me. Ctrl+Shift+O seems more suitable.
> > So to confirm, I will implement this feature for multiple messages.
> 
> I'm not sure about the desired behaviour. Probably different people will have
> different needs. There are at least three ways of doing "Show in conversation"
> for multiple selected messages:
> 
> a) for each selected msg, open a new tab and show it in conversation
> b) group selected msgs that belong to the same conversation in a single new
>    tab, otherwise open a new tab for each msg in conversation
> c) show all conversations for all selected messages in a single new tab (like 
>    in gloda search results, after "open as list")
> 
> Bryan and Nehal, maybe it's best and easiest to implement a) first, and discuss
> and implement more intelligent/complex behaviour later (Details and benefits
> below). I assume enabling the "Show in conversation" for multiple messages
> using simple behaviour a) is better than disabling it altogether. Bryan, what
> do you think?

I would recommend opening a new bug for "open multiple in conversations" as it's a new feature and this is just a change to an existing one.  We can easily get the current change pushed through as it's a simple modification but if we combine it with a new feature we'll complicate matters.

So for now just disable the command for multiple selection and I can easily ui-review something like that.  Copy your comments to a new bug that explores the possibilities of the new feature.

Comment 14

[Thomas D. (:thomas8)]

Nehal, could you provide a new patch in view of Bryan's comment 13?
- Ctrl+Shift+O as keyboard shortcut
- 's' as access key for main menu [EN]
- 'n' as access key for context menu [EN]
- disable "Open in conversation" in both menus for multiple messages
- context menu label: "Open Menu in Conversation"
- main menu label: "Open in Conversation"
- you might want to rename the new entities you introduce for cleaner code:
   +<!ENTITY openConversationCmd. ... (instead of showConversationCmd.)
   +<!ENTITY contextOpenConversation. ... (instead of contextShowConversation.)
- could you include a new label "open in conversation" for the same command in "Other actions" menu, and swap label references in the code accordingly? You'll need a new name for that label even though we're just changing the value (they say it's a bug that forces us do do such nonsense). Sidenote: I'll never understand why header pane commands have small initials, different from the rest of the program, but that's how it is right now.

- for ease and speed of ui-review, you might want to include screenshots of new patch applied

- Ideally, pls provide new patch asap. Ultimate String Freeze for 3.1 is currently 2010-03-23, and before that we'll still need final reviews.

Thanks for you contributions.

Comment 15

[Nehal M]

> - context menu label: "Open Menu in Conversation"
I'm guessing you meant "Open Message in Conversation" :)

I'll probably have time to get it done by the 7th.

Comment 16

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 424459 [details] [diff] [review]
Patch that adds the requested menus.

just my removing the review for now.  I tried to make my comments clear in comment 13 and I'll just try to keep an eye on this as Thomas drives it further.

Comment 17

[Thomas D. (:thomas8)]

As advised in Bryan's comment 13, I've created this followup bug:
Bug 550775 - Explore UI for implementing "Open in Conversation" for multiple selected messages

Nehal, please have an eye on Bug 525987 where some changes affecting the Message menu are up for review (including fix for bug 449383). This might interact with your patch, depending on which patch lands first.

Comment 18

[Nehal M]

Attachment: Patch-2

Here is a patch that implements comment 14.

Some notes:
- I changed 'ConversationShower' to 'ConversationOpener' as this made sense.
- There is 'showConversationInTab' in glodaFacetView.js and glodaFacetBindings.xml but I left that alone as there is also 'showMessageInTab' there, so it seemed consistent the way it is.

Thomas: The lower case menu items do look a bit out of place. Maybe open a new bug for that?

Comment 19

[Nehal M]

Attachment: Screenshot of the new menus from Patch-2.

I've attached an image composed of 3 screenshots: the Message Menu, the Context Menu, and the Message Header Pane.

Comment 20

[Thomas D. (:thomas8)]

Thanks Nehal, that was quick! Looks good.

(In reply to comment #18)
> Thomas: The lower case menu items do look a bit out of place. Maybe open a new
> bug for that?

Bug 550804 -  Reconsider capitalization in header pane command captions (should not be lower case, should be upper case)

Comment 21

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 430997 [details] [diff] [review]
Patch-2

looks good to me, thanks for the screenshot as well!

Comment 22

[Thomas D. (:thomas8)]

Thanks Bryan for quick ui-review! Looking at the screenshot from attachment 430997 [details] [diff] [review], it's also obvious that the command sequence in Message Menu has become pretty unstructured and unsorted over time, making it unnecessarily hard to find the right command. Bug 525987 addresses that problem with one of my first patches, and is waiting for your follow-up UI-review of what you have already reviewed  (screenshots provided for added comfort). It would be really cool to land these together.

Comment 23

[Bryan Clark (DevTools PM) [:clarkbw]]

This should get a code review as well before getting checked in.

Comment 24

[Thomas D. (:thomas8)]

Comment on attachment 430997 [details] [diff] [review]
Patch-2

Mark, can you do the code review for this little treat?

Comment 25

[Max Spicer]

I'd like to suggest 'Open Whole Conversation' as better wording than 'Open In Conversation'.  I don't think it's very clear what 'Open In Conversation' means.

Comment 26

[Mark Banner (:standard8)]

Comment on attachment 430997 [details] [diff] [review]
Patch-2

Sorry for the delay. In general, the patch is looking good. There's a few tweaks needed to make it work a bit better though:

>@@ -297,16 +298,20 @@ var DefaultController =

>+      case "cmd_openConversation":
>+      {
>+        return gFolderDisplay.selectedMessages.length == 1;
>+      }

The problem here is that this doesn't take account of two things:

- Firstly, if the pref mailnews.database.global.indexer.enabled is set to true.
- Secondly, if the message has actually been indexed or not.

I would suggest that you take the checks from onShowOtherActionsPopup, and put them into a function in ConversationOpener and then call them from there.

>+function MsgOpenConversationSelectedMessages()
>+{
>+  gConversationOpener.openConversationForMessages(gFolderDisplay.selectedMessages);
>+}
>+

I'm not sure we need this intermediate function, I think you could just go straight to gConversationOpener from where this is called.

>+			<menuitem id="openConversationMenuitem" label="&openConversationCmd.label;"
>+                      command="cmd_openConversation"
>+                      accesskey="&openConversationCmd.accesskey;"
>+                      key="key_openConversation"/>

nit: There's tabs here, please convert these to spaces.

>diff --git a/mail/base/content/nsContextMenu.js b/mail/base/content/nsContextMenu.js

>+    this.showItem("mailContext-openConversation",
>+                  this.numSelectedMessages == 1 && this.inThreadPane);
>+

You need to add mailContext-openConversation to the const array called messageTabSpecificItems otherwise you can get the menu item appearing at wrong times (e.g. right-clicking on the what's new tab).

Comment 27

[Nehal M]

Attachment: Patch 3

The last 3 issues you mentioned were straightforward. I was unsure about the first though. I added a isSelectedMessageIndexed function to ConversationOpener and called it from nsContextMenu.js and mail3PaneWindowCommands.js. Is this what you wanted? Please look over this patch.

Also, I moved case "cmd_openConversation": as I had carelessly put it right after a fall-through.

Comment 28

[Mark Banner (:standard8)]

Comment on attachment 436900 [details] [diff] [review]
Patch 3

This is good.

The only functional issue I've found is that the menu options don't work in the standalone message window - they are always disabled. However, this is already in part covered by Bug 531319, and I've had a chat to Bryan and we're prepared to let this patch go in as-is as the benefits are definitely good.

If you'd like to work on fixing bug 531319 as well, we'd appreciate it, but its not required to get this bug in.

For this patch, my only remaining comments are here:

>+  isSelectedMessageIndexed: function() {
...
>+      let isMessageIndexed = Gloda.isMessageIndexed(message);
>+      return isMessageIndexed;
>+    } else {
>+      return false;
>+    }

Firstly, you don't need the isMessageIndexed variable, you can just return the function value straight away. Secondly, because you've got the return statement, you then don't need the else block. So this bit becomes:

>+      return Gloda.isMessageIndexed(message);
>+    }
>+    return false;

As these are small, I've made these changes locally and committed the patch for you:

http://hg.mozilla.org/comm-central/rev/28597c753c92

Thanks for all your work on this.

Comment 29

[Thomas D. (:thomas8)]

For better retrievability, tweaking summary to better reflect all changes of this bug (before, couldn't find it when looking for ctrl+shift+o, or "Open in Conversation").