Closed Bug 466019 Opened 11 years ago Closed 11 years ago

allow switching to tabs in new tabbed interface (via ctrl+tab number)

Categories

(Thunderbird :: Mail Window Front End, defect, minor)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b3

People

(Reporter: maxxmozilla, Assigned: davida)

References

(Depends on 1 open bug, Blocks 3 open bugs)

Details

Attachments

(2 files, 6 obsolete files)

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).
Attached patch Proposed fix v1 (obsolete) — Splinter Review
Adds a check for tabmail, if exists then switches directly to the mail tab.
Attachment #349250 - Flags: review?(mkmelin+mozilla)
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?
it should, my omission
Attached patch Proposed fix v2 (obsolete) — Splinter Review
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 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)
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)
Attachment #349262 - Flags: ui-review?(clarkbw)
Comment on attachment 349262 [details] [diff] [review]
Proposed fix v2

Let's let Bryan weigh in here...
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.
Thx, Magnus, plus, Lightning isn't working in my profile, which makes it hard to test this.
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.
> 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.
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.
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.
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) ?
Duplicate of this bug: 467359
Attachment #349262 - Flags: review?(mkmelin+mozilla) → review-
Comment on attachment 349262 [details] [diff] [review]
Proposed fix v2

Cancelling per comment 12, in favor of mapping the tabs to tab indexes.
Mapping the numbers to tab indexes...
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.
Flags: wanted-thunderbird3+
> Przemyslaw, any chance you can update this patch to use indices?
I will look into this (code hints are welcomed).
I have problems providing the requested patch, sorry.

-> nobody
Assignee: firefox → nobody
Status: ASSIGNED → NEW
Attachment #349262 - Flags: ui-review?(clarkbw)
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
(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.
Sounds good to me. Przemyslaw: wanna give that a shot?
(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.
Assignee: nobody → firefox
Status: NEW → ASSIGNED
Attached patch proposed patch (obsolete) — Splinter Review
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)
Duplicate of this bug: 464113
Attachment #359706 - Flags: review?(mkmelin+mozilla) → review-
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.
err, I *don't* think we need...
Attached patch rev 2 (obsolete) — Splinter Review
I think this addresses all of the review comments.  Have at it =).
Attachment #359706 - Attachment is obsolete: true
Attachment #359926 - Flags: review?(mkmelin+mozilla)
(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
(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 :)
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)
Attachment #359926 - Flags: review?(mkmelin+mozilla) → review+
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
revised patch as per review comments.  carrying forward r+.

checkin at will.
Attachment #359926 - Attachment is obsolete: true
Attachment #360385 - Flags: review+
Keywords: checkin-needed
Checked in: http://hg.mozilla.org/comm-central/rev/6ea77b011cf5
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.0b2
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
Blocks: 477091
Ah yes we forgot about the compose window.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [compose window showing old hotkey issue remaining]
I'm also seeing cmd-1 indicated on the Window menu for Mac builds (probably related to hiddenWindow.xul?).
Attached patch rev3 - missing part (obsolete) — Splinter Review
Attachment #361586 - Flags: review?(mkmelin+mozilla)
@Bryan: is keyboard shortcut for addressbook wanted ?
If yes I can file a bug to add it back and take care of it.
(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 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-
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 ?
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
Attached patch rev3.1 - missing part (obsolete) — Splinter Review
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)
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)
(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.
Attachment #362058 - Flags: review?(mkmelin+mozilla) → review+
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).
Attachment #362058 - Attachment is obsolete: true
Attachment #362266 - Flags: review+
Keywords: checkin-needed
Attachment #360385 - Attachment description: rev 3 → rev 3 [checked in]
Whiteboard: [compose window showing old hotkey issue remaining] → [checkin after tb3b2 string freeze]
Blocks: 478415
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]
(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: 11 years ago11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [checkin after tb3b2 string freeze]
(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
Backed out: http://hg.mozilla.org/comm-central/rev/16dd55a2901c
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #362266 - Attachment description: rev3.1 - missing part - for the checkin [Checkin: Comment 49] → rev3.1 - missing part - for the checkin
Duplicate of this bug: 478762
No longer blocks: 478415
Depends on: 478415
Keywords: checkin-needed
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]
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: Thunderbird 3.0b2 → Thunderbird 3.0b3
Depends on: 725469
(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.
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.