Closed Bug 628768 Opened 13 years ago Closed 13 years ago

mark as read toggle broken

Categories

(Thunderbird :: Mail Window Front End, defect)

x86
Windows 7
defect
Not set
major

Tracking

(blocking-thunderbird5.0 alpha3+)

RESOLVED FIXED
Thunderbird 3.3a3
Tracking Status
blocking-thunderbird5.0 --- alpha3+

People

(Reporter: asa, Assigned: squib)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

I use the "m" key to mark messages that I've just read as unread. That broke recently. Now it seems it can only unread messages as read which seems pretty useless to me. If I've got the message selected then it's mostly likely already marked read. 

Keyboard shortcuts are critical for an email client. We can't let them break, even for a few days. This is really really painful and caused me to revert from nightly builds to the ill-performant previous release.
This was changed in bug 575732 to be Shift+M for mark as unread.
That change is wrong. Optimizing for the multi-message-select case instead of the single-message-select is backwards.
It's not optimized for either case; it just changes the behavior from toggling (which is only useful insofar as it reduces the total number of keyboard shortcuts by one) to a specific shortcut for each action.
I suppose there's an argument to be made that the *keyboard shortcut* should change to be a toggle (i.e. the way that the "mark" toolbar button currently works), but the menu popups should certainly have separate "Mark as Read" and "Mark as Unread" items, since it doesn't hurt the single message case, and immensely helps the multiple message case. This would have the side effect of making the keyboard shortcut not actually visible anywhere, however.
Keywords: relnote
blocking-thunderbird5.0: --- → ?
(In reply to comment #4)
> I suppose there's an argument to be made that the *keyboard shortcut* should
> change to be a toggle (i.e. the way that the "mark" toolbar button currently
> works), but the menu popups should certainly have separate "Mark as Read" and
> "Mark as Unread" items, since it doesn't hurt the single message case, and
> immensely helps the multiple message case. This would have the side effect of
> making the keyboard shortcut not actually visible anywhere, however.

This is what I was thinking since the 'm' key has already been a toggle for many users (both the auto mark as read and the not) while the menu option has improved to be more precise.  I guess we'd just have to accept the lack of visibility of the keyboard shortcut.
blocking-thunderbird5.0: ? → ---
blocking-thunderbird5.0: --- → ?
Aside from tests (grumble) this is pretty easy, but it would be nice if we could come up with some way to document this in Thunderbird itself. Is there any kind of precedent for this?
blocking-thunderbird5.0: ? → alpha3+
Here's a patch. This *will* break tests, which I haven't gotten around to updating yet. I'm still thinking about ways to document this keyboard shortcut, but I suppose it's not the end of the world if it's hidden.
Assignee: nobody → squibblyflabbetydoo
Status: NEW → ASSIGNED
Attached patch Now with testsSplinter Review
Well, I guess the earlier patch didn't technically break tests, since they were disabled. Here are some hopefully-working tests for this. The test_toggle_* functions might fail on Tinderbox, since they're basically the same tests as what was failing before. Hopefully the others will work though.
Attachment #508539 - Attachment is obsolete: true
Attachment #508557 - Flags: review?(bwinton)
Comment on attachment 508557 [details] [diff] [review]
Now with tests

>+++ b/mail/test/mozmill/folder-display/test-message-commands.js
>@@ -105,91 +105,131 @@ function check_read_menuitems(index, can
>-// XXX Disabled due to issues with running these tests on tinderbox
>-/*

I think I’ld like to see a push-to-try run to make sure these work before we check them in, but it’s late-ish, so I’ll do that tomorrow.

(Having said that, they all passed for me on my OSX box.)

So, I’m going to give it a provisional-r=me, but please wait until we've got a tinderbox run before you ask for checkin.

(Also, I'm kind of missing the explicit mark-as-read/unread keys now, because of the read-unread-bounce that happens on my slow server.)

Thanks,
Blake.
Attachment #508557 - Flags: review?(bwinton) → review+
Comment on attachment 508557 [details] [diff] [review]
Now with tests

So the way I understood Bryan's comment was that the 'm' key by itself would perform the toggle, but shift-'m' would still be an additional shortcut for mark as unread.

Bryan, did I get that understanding wrong? Is this patch what you were wanting?
Attachment #508557 - Flags: ui-review?(clarkbw)
Comment on attachment 508557 [details] [diff] [review]
Now with tests

(In reply to comment #10)
> Comment on attachment 508557 [details] [diff] [review]
> Now with tests
> 
> So the way I understood Bryan's comment was that the 'm' key by itself would
> perform the toggle, but shift-'m' would still be an additional shortcut for
> mark as unread.
> 
> Bryan, did I get that understanding wrong? Is this patch what you were wanting?

Yeah, I had assumed that we would keep the shift-m shortcut for mark unread.  ui-r+ otherwise.
Attachment #508557 - Flags: ui-review?(clarkbw) → ui-review+
That seems strange to me since it's so asymmetric (and you'd have Shift+M listed in the menus but not M, which would confuse me if I weren't privy to these changes). If we had more modifier keys, this wouldn't matter, since we could have M as "toggle read", Whatever+M as "mark read", and Shift+M as "mark unread", but as it is, the existence of a keyboard shortcut for one operation and the lack of one for its opposite gives me a bad feeling...

I think "mark unread" and "mark read" have the same overall priority (users with auto-mark-as-read want the former, and users without that want the latter), so giving one a keyboard shortcut but not the other will necessarily make things worse by comparison for one of those groups of people.

I guess in quantifiable terms, the "toggle read" shortcut suffers from a lack of ux-discovery and the lack of a "mark read" shortcut suffers from a lack of ux-consistency with "mark unread".

All that said, I don't think this is a *huge* issue, so I will do my best to avoid bikeshedding here. :)
Blocks: 575732
(In reply to comment #12)
> That seems strange to me since it's so asymmetric (and you'd have Shift+M
> listed in the menus but not M, which would confuse me if I weren't privy to
> these changes). If we had more modifier keys, this wouldn't matter, since we
> could have M as "toggle read", Whatever+M as "mark read", and Shift+M as "mark
> unread", but as it is, the existence of a keyboard shortcut for one operation
> and the lack of one for its opposite gives me a bad feeling...

What I'm reading here is that you're suggesting we use, perhaps ctrl+m for 'mark read', to keep shift+m for 'mark unread' and keeping m as the 'toggle read'.  That sounds reasonable to me, though slightly more complicated it is at least thorough.
Unfortunately, Ctrl+M is "compose new message" (though Ctrl+N also works), and Ctrl+Shift+M is "move to ___ again". My original idea was that "it would be nice if we have more meta keys, but we don't, so we should only have a hotkey for 'toggle read'." I guess Ctrl+M could be moved to be "mark as read", but I also have no real problem with having "M" be the only keyboard shortcut. It seems like there's a downside to any choice here, unfortunately.

Good things about only having a hotkey for "toggle read":
* No need to change existing key combos
* No asymmetry if only one of "mark as read"/"mark as unread" had a hotkey
* Hitting "m" is fast, and so is hitting "m" twice

Bad things:
* The hotkeys for marking or toggling read state would be undocumented in the
  UI (though "toggle read" would be undocumented no matter what)
* Potential for the "light-switch problem" (i.e. you walk into a room, flip the
  switch in the wrong direction, and then switch back and forth a couple times
  until you get it right)
(in case it hasn't been mentioned already) you also have the bugger of Bug 353837 - Mark button label returns "Mark messages", does not indicate default action is mark message as read/unread
Comment on attachment 508557 [details] [diff] [review]
Now with tests

looking back over this issue I think we can leave the shift-m out of this and go ahead with the patch we have.

ui-r+ as is
All that remains now is a push to the try server and then checkin.
Hmmm, it looks like there are test failures in close_popup() from test-folder-display-helpers.js, which seems... strange.

http://tinderbox.mozilla.org/showlog.cgi?log=ThunderbirdTry/1299884994.1299885659.2103.gz
Ah ha, I got bitrotted by bug 633498. That should be easy enough to fix.
I fixed the bitrot and pushed it, hopefully it'll pass tests on all platforms:

http://hg.mozilla.org/comm-central/rev/6fbe19ade0d5
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.3a3
OK, so I have a major usability issue that was solved by having mark read and mark unread being separate actions, and I was quite disappointed when that got broke by this checkin.

I use the standalone message window to read messages.  The standalone window has no UI within the window to tell you if the message is currently marked as unread or not.  Quite likely this problem is better solved by exposing that condition within the UI of the standalone window, but without that UI, it was nice to be able to know for sure that I was marking the message as unread without having it need to be still visible in the folder window behind it to be able to tell.
(In reply to comment #22)
> Quite likely this problem is better solved by exposing that
> condition within the UI of the standalone window

which is bug 418024
Depends on: 649890
Blocks: 768025
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: