Closed
Bug 466019
Opened 16 years ago
Closed 15 years ago
allow switching to tabs in new tabbed interface (via ctrl+tab number)
Categories
(Thunderbird :: Mail Window Front End, defect)
Thunderbird
Mail Window Front End
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 3.0b3
People
(Reporter: maxxmozilla, Assigned: davida)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
Attachments
(2 files, 6 obsolete files)
5.86 KB,
patch
|
davida
:
review+
|
Details | Diff | Splinter Review |
6.08 KB,
patch
|
maxxmozilla
:
review+
|
Details | Diff | Splinter Review |
If you have calendar or task tab opened and you want to switch to the mail tab with accel+1 nothing happens (it worked when there were modes and not tabs).
Reporter | ||
Comment 1•16 years ago
|
||
Adds a check for tabmail, if exists then switches directly to the mail tab.
Attachment #349250 -
Flags: review?(mkmelin+mozilla)
Comment 2•16 years ago
|
||
Comment on attachment 349250 [details] [diff] [review] Proposed fix v1 shouldn't the third line use the tabmail var as well, instead of getting it again?
Reporter | ||
Comment 3•16 years ago
|
||
it should, my omission
Reporter | ||
Comment 4•16 years ago
|
||
changing review to David as he mostly already did it
Attachment #349250 -
Attachment is obsolete: true
Attachment #349262 -
Flags: review?(bienvenu)
Attachment #349250 -
Flags: review?(mkmelin+mozilla)
Comment 5•16 years ago
|
||
Comment on attachment 349262 [details] [diff] [review] Proposed fix v2 Taking back review, i bet David's got a lot of other work on his plate :p
Attachment #349262 -
Flags: review?(bienvenu) → review?(mkmelin+mozilla)
Comment 6•16 years ago
|
||
The indention is off, but i don't really see why switching to the first tab with ctrl+1 would be expected behavior. Ctrl+1 is "get me to the main 3pane window", that your current tab happens to be not the first tab seems to irrelevant.
Summary: fix switching to mail tab in new tabbed interface → fix switching to mail tab in new tabbed interface (via ctrl+1)
Updated•16 years ago
|
Attachment #349262 -
Flags: ui-review?(clarkbw)
Comment 7•16 years ago
|
||
Comment on attachment 349262 [details] [diff] [review] Proposed fix v2 Let's let Bryan weigh in here...
Reporter | ||
Comment 8•16 years ago
|
||
When there were no tabs ctrl+1 was switching only to the main 3pane window and even with earlier builds of lightning (with modes and not tabs) you could switch from calendar/tasks to the mail window. So now with tabs and with ctr+3,4 switching to the calendar, tasks tabs I think such behavior is expected. Please note that current implementation only affects when switching from tabs, from compose, AB, standalone msg windows pressing ctrl+1 gets you to the last view of the 3pane window and second ctrl+1 is needed to switch to the mail tab.
Comment 9•16 years ago
|
||
Thx, Magnus, plus, Lightning isn't working in my profile, which makes it hard to test this.
Comment 10•16 years ago
|
||
I didn't actually realize ctrl+3/ctrl+4 was used by lightning (that seems not to be indicated anywhere?), so it makes a bit more sense to me now.
Reporter | ||
Comment 11•16 years ago
|
||
> I didn't actually realize ctrl+3/ctrl+4 was used by lightning (that seems not
> to be indicated anywhere?)
The shortcuts are shown under Go menu (for example) - currently switching works only from menu (lightning relies on this shortcut).
Also Ctrl+1 would be partial parity with Firefox (select first tab :) (at least until re-arranging of tabs is not possible)
David: maybe it needs updating ? ;) / as a lighting tester I would be interested why it doesn't work on your profile or please file a bug in spare time
BTW lightning is not needed to test this, opening folder in new tab or a message is enough.
Comment 12•16 years ago
|
||
If we were to follow firefox wrt tab shortcuts then accel+X would map to the tab index. However currently AB is accel+2 and lightning has taken 3,4. I think it's best if we follow the firefox tab shortcut scheme instead of having our own hybrid system. Using half of their tab shortcuts and half of our own is going to confuse people, especially since we have such a cross over of users.
Reporter | ||
Comment 13•16 years ago
|
||
So, do you propose to change accel+2 and 3,4 (even in lightning) so they would map to the tab index ? (this would break some TB and lightning users habits) While the idea of following the firefox might be good in this case I'm not sure about it wrt lightning. What shortcuts could be used to switch to calendar/tasks (if they are not opened) ? (alt+1234 is used to switch the views in calendar; and what about shortcut to open AB) Acel+1 - main 3pane window Acel+2 - AB (Bug 457270 would be a nice option) Acel+3 - Calendar Acel+4 - Tasks Do they look like a hybrid ?, new added shortcuts seems to follow original idea of TB. I can add that I had plans to file a bug to allow switching from AB, compose, standalone msg windows to calendar and tasks tabs after fixing this bug.
Reporter | ||
Comment 14•16 years ago
|
||
W got a little OT. Currently mail tab is shown by default, it's the first one and it can't be re-arranged so I don't have objections to use the tab index but is there an easy way to select tab by index in TB implementation of tabs (I saw only by mode) ?
Updated•15 years ago
|
Attachment #349262 -
Flags: review?(mkmelin+mozilla) → review-
Comment 16•15 years ago
|
||
Comment on attachment 349262 [details] [diff] [review] Proposed fix v2 Cancelling per comment 12, in favor of mapping the tabs to tab indexes.
Comment 17•15 years ago
|
||
Mapping the numbers to tab indexes...
Assignee | ||
Comment 18•15 years ago
|
||
Przemyslaw, any chance you can update this patch to use indices? The patch should probably need to come up with new non-numeric shortcuts for addressbook and the lightning tabs, but it does feel like a more predictable model.
Assignee | ||
Updated•15 years ago
|
Flags: wanted-thunderbird3+
Reporter | ||
Comment 19•15 years ago
|
||
> Przemyslaw, any chance you can update this patch to use indices?
I will look into this (code hints are welcomed).
Reporter | ||
Comment 20•15 years ago
|
||
I have problems providing the requested patch, sorry. -> nobody
Assignee: firefox → nobody
Status: ASSIGNED → NEW
Reporter | ||
Updated•15 years ago
|
Attachment #349262 -
Flags: ui-review?(clarkbw)
Comment 21•15 years ago
|
||
I just looked up how FF does this. http://mxr.mozilla.org/comm-central/source/mozilla/browser/base/content/browser-sets.inc#337 They just insert the hard coded <key/> elements for 1-8, with 9 taking you to the last tab. accel-0 doesn't do anything, as it probably shouldn't. Przemyslaw: Perhaps you can just remove the current <key/> elements [1] from mailWindowOverlay.xul, messengercompose.xul, and addressbook.xul; and adding the tab index selectors to mailWindowOverlay.xul just like FF has. Magnus: Please comment on this approach if you have a chance. [1] http://mxr.mozilla.org/comm-central/search?string=toMessengerWindow&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central
Comment 22•15 years ago
|
||
(In reply to comment #21) > They just insert the hard coded <key/> elements for 1-8, with 9 taking you to > the last tab. accel-0 doesn't do anything, as it probably shouldn't. accel-0 sets page zoom to the default of no zoom.
Comment 23•15 years ago
|
||
Sounds good to me. Przemyslaw: wanna give that a shot?
Reporter | ||
Comment 24•15 years ago
|
||
(In reply to comment #21) > I just looked up how FF does this. > http://mxr.mozilla.org/comm-central/source/mozilla/browser/base/content/browser-sets.inc#337 > > They just insert the hard coded <key/> elements for 1-8, with 9 taking you to > the last tab. Actually I've tried to do this but tabmail.xml is missing selectTabAtIndex method, I've tried to adapt it from tabbrowser.xml but I couldn't get it to work.
Reporter | ||
Updated•15 years ago
|
Assignee: nobody → firefox
Status: NEW → ASSIGNED
Assignee | ||
Comment 25•15 years ago
|
||
I was bored, so i took this on. I hope that's ok, Przemyslaw. The attached patch works for me -- note that in particular it will bring back the main mail window from a minimized-to-dock state on the mac using any of the cmd-N keys. In that case, hitting the key again will select the Nth tab. (if the window is already focused, it does the second part right away). Pending issues: - file bugs against lightning re: 3 & 4. - decide if we want a keybinding for the addressbook to replace the 2 - decide if cmd-0 should do something interesting. I wouldn't mind stuffing accel-N in the Window menu if it means we can do accel-N and get a new message window w/o having to have the window focused.
Assignee: firefox → david.ascher
Attachment #349262 -
Attachment is obsolete: true
Attachment #359706 -
Flags: review?(mkmelin+mozilla)
Updated•15 years ago
|
Attachment #359706 -
Flags: review?(mkmelin+mozilla) → review-
Comment 27•15 years ago
|
||
Comment on attachment 359706 [details] [diff] [review] proposed patch Basically this works fine, however, I'd like us to do more of the stuff firefox does - especially as it would just be some more copy paste. (See the code clarkbw linked to earlier.) - should be "alt" on gnome - ctrl + 9 for last tab, (also see bug 443863 about extensibility, cancel the event) Other stuff - remove the uncommented code - style nits: o remove the space in if ( foo ) o if one branch in an if / else has braces, they all (of the same level) should Personally I think we don't need shortcuts to get to the addressbook - that feels like some leftover from suite times.
Comment 28•15 years ago
|
||
err, I *don't* think we need...
Assignee | ||
Comment 29•15 years ago
|
||
I think this addresses all of the review comments. Have at it =).
Attachment #359706 -
Attachment is obsolete: true
Attachment #359926 -
Flags: review?(mkmelin+mozilla)
Reporter | ||
Comment 30•15 years ago
|
||
(In reply to comment #25) > I was bored, so i took this on. I hope that's ok, Przemyslaw. not a problem (guess I will change my plans for sunday ;) > - decide if we want a keybinding for the addressbook to replace the 2 accel+B (address Book) is free so maybe it could be changed and not removed... ? (there are some use cases for it) > - decide if cmd-0 should do something interesting. it sets page zoom to default value
Comment 31•15 years ago
|
||
(In reply to comment #25) > - decide if we want a keybinding for the addressbook to replace the 2 Mail.app uses cmd+opt+A, Windows Live Mail uses ctrl+shift+C-for-contacts, Outlook Express uses ctrl+shift+B, Evolution uses ctrl+2. So I guess, no matter what the OS there's a keyboard-accessible app that people can use for mail :)
Updated•15 years ago
|
Summary: fix switching to mail tab in new tabbed interface (via ctrl+1) → fix switching to mail tab in new tabbed interface (via ctrl+tab number)
Updated•15 years ago
|
Attachment #359926 -
Flags: review?(mkmelin+mozilla) → review+
Comment 32•15 years ago
|
||
Comment on attachment 359926 [details] [diff] [review] rev 2 Sweet! r=mkmelin with a few nits. >+ var topWindow = windowManagerInterface.getMostRecentWindow( "mail:3pane" ); Remove the extra space inside the () >+ if (topWindow ) { Remove the extra space. You also have to define XP_GNOME, like http://mxr.mozilla.org/comm-central/source/mozilla/browser/base/content/browser-sets.inc#42
Assignee | ||
Comment 33•15 years ago
|
||
revised patch as per review comments. carrying forward r+. checkin at will.
Attachment #359926 -
Attachment is obsolete: true
Attachment #360385 -
Flags: review+
Assignee | ||
Updated•15 years ago
|
Keywords: checkin-needed
Comment 34•15 years ago
|
||
Checked in: http://hg.mozilla.org/comm-central/rev/6ea77b011cf5
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.0b2
Reporter | ||
Comment 35•15 years ago
|
||
David, you did forget about compose window (has old shortcut to address book.) http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/messengercompose.xul#236
Comment 36•15 years ago
|
||
Ah yes we forgot about the compose window.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [compose window showing old hotkey issue remaining]
Comment 37•15 years ago
|
||
I'm also seeing cmd-1 indicated on the Window menu for Mac builds (probably related to hiddenWindow.xul?).
Reporter | ||
Comment 38•15 years ago
|
||
Attachment #361586 -
Flags: review?(mkmelin+mozilla)
Reporter | ||
Comment 39•15 years ago
|
||
@Bryan: is keyboard shortcut for addressbook wanted ? If yes I can file a bug to add it back and take care of it.
Comment 40•15 years ago
|
||
(In reply to comment #39) > @Bryan: is keyboard shortcut for addressbook wanted ? > If yes I can file a bug to add it back and take care of it. Yes, can you file that with the suggestions from above listed? Thanks!
Comment 41•15 years ago
|
||
Comment on attachment 361586 [details] [diff] [review] rev3 - missing part While you're touching the row, please fold it to somthing of reasonable length. Bigger problem though, even with this patch addressBookCmd.commandkey is used, also check mailTasksOverlay.xul for the key_addressbook
Attachment #361586 -
Flags: review?(mkmelin+mozilla) → review-
Reporter | ||
Comment 42•15 years ago
|
||
Bryan has agreed for the shortcut to the AB so I propose to not remove code which would have to be re-added but to either fix the shortcut here or close this bug and file follow-up bug ?
Comment 43•15 years ago
|
||
Assuming we find any... I think it's clearer to add it back if we find one, especially as the keys were partly removed already. I mis-pasted the filename earlier it seems (and was wrong about addressBookCmd.commandkey) - but I think the occurrences I was thinking of is http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.xul#1308 http://mxr.mozilla.org/comm-central/source/mail/base/content/macMenuOverlay.xul#54
Reporter | ||
Comment 44•15 years ago
|
||
take 2 Thanks for the tips Magnus! (if I would re-add addressBookCmd.commandkey entity in other bug with different key, do I have to change it name e.g. addressBookCmd.commandkey2 ?)
Attachment #361586 -
Attachment is obsolete: true
Attachment #362058 -
Flags: review?(mkmelin+mozilla)
Reporter | ||
Comment 45•15 years ago
|
||
adjusting summary
Summary: fix switching to mail tab in new tabbed interface (via ctrl+tab number) → allow switching to tabs in new tabbed interface (via ctrl+tab number)
Comment 46•15 years ago
|
||
(In reply to comment #44) > (if I would re-add addressBookCmd.commandkey entity in other bug with different > key, do I have to change it name e.g. addressBookCmd.commandkey2 ?) The entity name should change, but not that way. It should end in .commandkey or .key, to ease localization tool automation tasks. I'd use .key for the new one.
Updated•15 years ago
|
Attachment #362058 -
Flags: review?(mkmelin+mozilla) → review+
Comment 47•15 years ago
|
||
Comment on attachment 362058 [details] [diff] [review] rev3.1 - missing part Nit: would be nice if the id was the first attribute (in the stuff you're touching).
Reporter | ||
Comment 48•15 years ago
|
||
Attachment #362058 -
Attachment is obsolete: true
Attachment #362266 -
Flags: review+
Reporter | ||
Updated•15 years ago
|
Keywords: checkin-needed
Reporter | ||
Updated•15 years ago
|
Attachment #360385 -
Attachment description: rev 3 → rev 3 [checked in]
Updated•15 years ago
|
Whiteboard: [compose window showing old hotkey issue remaining] → [checkin after tb3b2 string freeze]
Comment 49•15 years ago
|
||
Comment on attachment 362266 [details] [diff] [review] rev3.1 - missing part - for the checkin [Checkin: See comment 54] http://hg.mozilla.org/comm-central/rev/35537d8318a6
Attachment #362266 -
Attachment description: rev3.1 - missing part - for the checkin → rev3.1 - missing part - for the checkin
[Checkin: Comment 49]
Comment 50•15 years ago
|
||
(In reply to comment #49) > (From update of attachment 362266 [details] [diff] [review]) > > http://hg.mozilla.org/comm-central/rev/35537d8318a6 Oh ! I think this needed approval for 30b2 :-< (I saw no TB approval flag to request though :-/) Please, post-approve or let me know if you prefer that I back it out.
Status: REOPENED → RESOLVED
Closed: 15 years ago → 15 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [checkin after tb3b2 string freeze]
Comment 51•15 years ago
|
||
(In reply to comment #50) > Please, post-approve or let me know if you prefer that I back it out. Please back out ASAP. It affects strings and was not meant to be checked in
Comment 52•15 years ago
|
||
Backed out: http://hg.mozilla.org/comm-central/rev/16dd55a2901c
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Updated•15 years ago
|
Attachment #362266 -
Attachment description: rev3.1 - missing part - for the checkin
[Checkin: Comment 49] → rev3.1 - missing part - for the checkin
Updated•15 years ago
|
Reporter | ||
Updated•15 years ago
|
Keywords: checkin-needed
Comment 54•15 years ago
|
||
Comment on attachment 362266 [details] [diff] [review] rev3.1 - missing part - for the checkin [Checkin: See comment 54] http://hg.mozilla.org/comm-central/rev/4c52febd755b after fixing context for { patching file mail/base/content/mailWindowOverlay.xul Hunk #1 FAILED at 1299 1 out of 1 hunks FAILED }
Attachment #362266 -
Attachment description: rev3.1 - missing part - for the checkin → rev3.1 - missing part - for the checkin
[Checkin: See comment 54]
Updated•15 years ago
|
Status: REOPENED → RESOLVED
Closed: 15 years ago → 15 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Updated•15 years ago
|
Target Milestone: Thunderbird 3.0b2 → Thunderbird 3.0b3
Comment 55•12 years ago
|
||
(Magnus, pls assist, as requested below.) 3 years after the fact, these useful keyboard shortcuts also found their way into the main keyboard shortcuts documentation (1), with my latest revision (2) (1) https://support.mozillamessaging.com/en-US/kb/keyboard-shortcuts/ (2) https://support.mozillamessaging.com/en-US/kb/keyboard-shortcuts/revision/5824 Looking at Magnus comment 27, I think I got the Linux shortcuts wrong: (In reply to Magnus Melin from comment #27) > Comment on attachment 359706 [details] [diff] [review] > proposed patch > Basically this works fine, however, I'd like us to do more of the stuff > firefox does - especially as it would just be some more copy paste. (See the > code clarkbw linked to earlier.) > > - should be "alt" on gnome > - ctrl + 9 for last tab, (also see bug 443863 about extensibility, cancel > the event) Magnus, can you clarify which shortcuts eventually made it into the final patch for *LINUX*? I live in a windows world, so my idea of things like +#define XP_GNOME 1 is somewhat vague, with the added difficulty of multiple patches here, some of which were backed out, so it's not entirely clear to me what finally went in. I think this bug implemented *alt*+1-9 for *LINUX*, but I got confused when updating the documentation because old documentation claims that in *composition* window, we have *Ctrl+1* on *Linux* to get back to main mail window (mail & newsgroups 3pane), so it would be quite inconsistent if for practically the same purpose, it's *alt*+1 on Linux in the main 3pane. So Magnus, can you also comment which shortcut is used in *composition* window, on *Linux*, to show main mail window (mail & newsgroups 3pane)? I'm talking about "Tools > Mail & Newsgroup" from composition.
Blocks: tb-keyboard-tracker, tbkbd-doc-tracker
Comment 56•12 years ago
|
||
Thomas, On linux it's alt+number (1-8) to switch to the given tab number. alt+9 will give you the last tab. Yes, in the compose window it's ctrl+1 to go go to the 3pane window.
You need to log in
before you can comment on or make changes to this bug.
Description
•