Closed Bug 525987 Opened 12 years ago Closed 12 years ago
Message > Archive and New Tag should be repositioned within their menus
31.06 KB, image/png
44.31 KB, image/png
21.67 KB, patch
|Details | Diff | Splinter Review|
This bug proposes to reposition "Archive" command within the "Message" menu. If this gets ui-review+, I'll provide the patch. Message > Archive command is currently in the following position: Actual layout: New Message Archive Reply Reply to all ... -------------- Move to > Copy to > Copy to... again Tag > Mark > -------------- ... A better position for Archive command would be the following: Expected layout: New Message Reply Reply to all ... -------------- Archive Move to > Copy to > Copy to... again Tag > Mark > -------------- ... Reasons for repositioning: 1) Archive is a move operation and thus should be grouped with the other move and copy commands (this is the most important reason) 2) All other commands above the first separator (from "New msg" to "Open") are commands that don't change the original mail in any way. Only archive command changes the original mail by relocating it. Which is inconsistent and confusing for users. 3) All commands below the first separator (after "Open") are commands that change the message (location/properties) in some way. So does "Archive" (relocation). 4) "Archive" will usually be the /last/ action on a msg (after reading, replying, forwarding etc.), so it shouldn't be listed above these other msg action commands, but underneath.
Attachment #409776 - Flags: ui-review?(clarkbw)
Comment on attachment 409776 [details] Screenshot: Message > Archive command in new position (as proposed here) sure, that makes sense to me.
Attachment #409776 - Flags: ui-review?(clarkbw) → ui-review+
Assignee: nobody → bugzilla2007
Version: 3.0 → Trunk
There is a similar arrangement in the context menu, which should changed then as well to make it the same.
Thomas, I assigned this to you to get a clear proposal but if you want to assign it back to someone else (not me!) for the patch feel free to do so.
(In reply to comment #2) > There is a similar arrangement in the context menu, which should changed then > as well to make it the same. Thanks, rsx11m. I included that in the patch. (In reply to comment #3) > Thomas, I assigned this to you to get a clear proposal but if you want to > assign it back to someone else (not me!) for the patch feel free to do so. Here's a first patch which repositions archive menu as proposed. I've also added a new separator in the MessageMenu before TagMenu, because we also have one in the context menu, and clearly tagging and marking are quite different from moving and copying. I have some more thoughts about reordering the different chunks of the menu, like putting "open msg" to the top of message menu (like in the context menu), and moving tagMenu and markMenu further up, either before the new position of "Archive", or even further up, after "open msg". I'm thinking of the most common sequence in which we can expect those commands to be done - open, mark, tag, new msg./reply etc., then archive/move around. Another sorting criterion might be frequency, where replying etc. might be more frequently used by more users than marking and tagging. Anyway, I'll put this up as ui-reviewed by Bryan so far, can't promise to get back to this for more. The current patch is absolutely no-risk (just moving the archive menu down a little, plus the added separator with a new ID), so we could probably take this for tb3 after review.
nitfix the patch header
Attachment #410605 - Flags: review?(bugzilla) → review+
Attachment #410605 - Flags: ui-review?(clarkbw) → ui-review+
Thomas, you know that you have to flag your patch for checkin to get it into the code? Once you have all required reviews (which is the case for trunk), make sure that the patch still applies and set "checkin-needed" as keyword.
I know. Unfortunately, it's currently bitrotted, but thanks for the reminder.
Are you sure about the bitrot? There have been changes in that file recently, but the patch still applies for me with a small offset.
Updated patch: 1) fix bitrot/offset for existing patch attachment 410605 [details] [diff] [review] (fwd ui-review+) 2) include patch for bug 449383 ("New Tag" command should be at the top of tag list; fwd ui-review+) 3) two minor enhancements (needs ui-review from Bryan; pls find screenshots in next comment): a) Message Menu: Open Command now at the top (consistent with Msg Context Menu) b) Message Menu + Msg Context Menu: "Tag >" and "Mark >" Menus now moved up a little: - mirror user's idealized sequence of actions (Open > Reply/Fwd > Tag/Mark > Archive, where Archive is usually the last operation on the msg) - functional benefit: as tag's list submenu now pops up further up, it can contain more tags and still pop up correctly at the submenu's node; furthermore: shorter mouse path to your favourite tag
Bryan, to ease your ui-review, here's a screenshot showing all the menus with Patch2 applied. As explained in comment 9, there's two minor changes in the command sequence of Message Menu and msg Context Menu, in addition to what you have given ui-review+ here and in bug 449383 already. If you like it, please set ui-review+ flags for both patch2 and its screenshot.
Mark, patch 2 (attachment 430938 [details] [diff] [review]) would like your review. Patch2 builds on the existing patch that you have already reviewed+ here; there's some chunks of moved xul code as explained in comment 9. Especially this now includes a patch for bug 449383 where I had to tweak the tagpopup Menu creation a little. I tried to change the existing hacks as little as possible, and looking at the screenshots, it seems to work out :)
If this got ui-review and review for the small remainder of added changes quickly, it would be a nice menu cleanup to land in 3.1, easing migration223 by creating order from the current chaos of commands in Message Menu ("ordo ab chao": cf. Dan Brown, The lost Symbol...). From what I can see, this is "NO l10n impact" and "no risk".
Comment on attachment 430940 [details] Screenshot of patch2: MessageMenu, MsgContextMenu, TagMenu looks good, thanks for the screenshot
Attachment #430940 - Flags: ui-review?(clarkbw) → ui-review+
Comment on attachment 430938 [details] [diff] [review] Patch2: reposition Archive, Tag+Mark, NewTag (bug 449383) I'm not sure about the open menu item moving to the top. It's consistent with the context popup menu but I don't think that makes it better for the main menu. I think that "New" is still likely what we want for the top action in that menu.
Attachment #430938 - Flags: ui-review?(clarkbw) → ui-review-
(In reply to comment #14) > (From update of attachment 430938 [details] [diff] [review]) > I'm not sure about the open menu item moving to the top. It's consistent with > the context popup menu but I don't think that makes it better for the main > menu. I think that "New" is still likely what we want for the top action in > that menu. Hmmm. Bryan, due to bug 525890 there's now 2 open commands (Open + Open in Conversation), and we might need more to enable opening conversations in tabs vs. windows (Context menu already has 3 open commands). We have Bug 546621: 'Open in conversation' feature is too hidden. Therefore, I'd think that there will probably be a larger number of users who will use "Open in conversation" from message menu (both mouse and keyboard users). Maybe that's a good excuse to have both open commands at the top? At first sight, "New message" & friends certainly look as if they should be top commands. On the other hand, who will use "New message" from Message Menu, and how? - Keyboard users will most likely use Ctrl+N for such a frequent action (so they are not using "New Message" from Message menu) - if they use the menu, they will most likely use the accelerator keys (alt+m, n), so they won't care much about the order of commands - Mouse users will most likely use "Write" button as it's less clicks (so they are not using "New Message" from Message menu) Well, you could probably say the same for open commands... Having "New msg" at the top would better mirror the file menu, where "new file" is always before "open file" (btw, we also have "new > message" in file menu). Ok, if you think open commands shouldn't be at the top, there's some variants for having them further down (see screenshot with all variants): 1) Open and Open in Conversation in their own section at the top (exactly as in context menu, followed by separator) 2) Open and Open in Conversation after "new msg" & friends, but still separate in their own section (similar to context menu; two separators). Reason: Open commands are different from "New", "Reply" etc. in that they don't create a new message 3) Open and Open in Conversation separated from "new msg" etc, but grouped with "tag" and "mark": all of these four do not create a new msg, but act on the existing msg. We also have a shorter, easier to parse list for "new msg" etc. 4) Open and Open in Conversation grouped with/after "new msg" etc., without separator. Rationale: all of these involve the msg as a whole (whereas tags & mark only change properties), and "new" and "open" can be seen as a logical pair. I don't have strong feelings about this, they all have their advantages and odds. Pls let me know which of the 4 variants you'd prefer.
Attachment #432920 - Flags: ui-review?(clarkbw)
Attachment #430938 - Flags: review?(bugzilla) → review-
Comment on attachment 430938 [details] [diff] [review] Patch2: reposition Archive, Tag+Mark, NewTag (bug 449383) >- <menuitem id="mailContext-tagRemoveAll" >- oncommand="RemoveAllMessageTags();"/> >- <menuseparator id="mailContext-sep-afterTagRemoveAll"/> >- <menuseparator id="mailContext-sep-beforeAddNewTag"/> ... >+ <menuseparator id="mailContext-sep-afterTagAddNew"/> >+ <menuitem id="mailContext-tagRemoveAll" >+ oncommand="RemoveAllMessageTags();"/> >+ <menuseparator id="mailContext-sep-afterTagRemoveAll"/> If I'm reading this right, then you're adding a separator of id "mailContext-sep-afterTagAddNew" and removing "mailContext-sep-beforeAddNewTag", in which case please update initSeparators in nsContextMenu.js. Although now I look at it again, some seem to be missing from the list in initSeparators, so please update that anyway. It would also be good to provide a comment at the start of the context menu xul code along the lines of "Please ensure all items and separators are kept up to date in nsContextMenu.js when making changes here".
Comment on attachment 432920 [details] Screenshot: Message Menu variants: "Open" + "Open in conversation" in different positions Either 2 or 4 is ok with me. No strong feeling about the separator. I want to move things around as little as possible. Menu item churn should be avoided at all costs even it's good to clean house; we're more likely breaking peoples habits for somewhat little gain.
Attachment #432920 - Flags: ui-review?(clarkbw) → ui-review+
(In reply to comment #17) > (From update of attachment 432920 [details]) > Either 2 or 4 is ok with me. No strong feeling about the separator. The new patch is modeled exactly on Variant 2 (screenshot attachment 432920 [details]), as ui-reviewed+ by Bryan's comment 17. (In reply to comment #16) This patch addresses everything of Mark's comment 16. Patch for trunk. Probably works for 3.1, too. Could someone please compile this and test, I can't. Screenshots of the following with patch applied would be great: - message menu, with tag menu popped open - toolbar tag button, with tag menu popped open - msg context menu, with tag menu popped open
Sorry, I keep missing this review, I'll get to it soon.
Comment on attachment 456618 [details] [diff] [review] Patch 3: reposition "Archive" and "NewTag" (bug 449383) Thanks for the patch, this looks good and works well. Sorry for the delay in getting to the review.
Attachment #456618 - Flags: review?(bugzilla) → review+
I've checked this in for you as well: http://hg.mozilla.org/comm-central/rev/b60327000654
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Summary: Message > Archive command should be repositioned within the menu → Message > Archive and New Tag should be repositioned within their menus
Target Milestone: --- → Thunderbird 3.2a1
You need to log in before you can comment on or make changes to this bug.