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

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Przemyslaw Bialik]

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).

Comment 1

[Przemyslaw Bialik]

Attachment: Proposed fix v1

Adds a check for tabmail, if exists then switches directly to the mail tab.

Comment 2

[David :Bienvenu]

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?

Comment 3

[Przemyslaw Bialik]

it should, my omission

Comment 4

[Przemyslaw Bialik]

Attachment: Proposed fix v2

changing review to David as he mostly already did it

Comment 5

[Magnus Melin [:mkmelin]]

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

Comment 6

[Magnus Melin [:mkmelin]]

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.

Comment 7

[Magnus Melin [:mkmelin]]

Comment on attachment 349262 [details] [diff] [review]
Proposed fix v2

Let's let Bryan weigh in here...

Comment 8

[Przemyslaw Bialik]

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

[David :Bienvenu]

Thx, Magnus, plus, Lightning isn't working in my profile, which makes it hard to test this.

Comment 10

[Magnus Melin [:mkmelin]]

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.

Comment 11

[Przemyslaw Bialik]

> 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

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

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.

Comment 13

[Przemyslaw Bialik]

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.

Comment 14

[Przemyslaw Bialik]

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) ?

Comment 16

[Magnus Melin [:mkmelin]]

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

[Magnus Melin [:mkmelin]]

Mapping the numbers to tab indexes...

Comment 18

[David Ascher (:davida)]

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.

Comment 19

[Przemyslaw Bialik]

> Przemyslaw, any chance you can update this patch to use indices?
I will look into this (code hints are welcomed).

Comment 20

[Przemyslaw Bialik]

I have problems providing the requested patch, sorry.

-> nobody

Comment 21

[Bryan Clark (DevTools PM) [: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

Comment 22

[Shawn Wilsher :sdwilsh]

(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

[Magnus Melin [:mkmelin]]

Sounds good to me. Przemyslaw: wanna give that a shot?

Comment 24

[Przemyslaw Bialik]

(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.

Comment 25

[David Ascher (:davida)]

Attachment: proposed patch

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.

Comment 27

[Magnus Melin [:mkmelin]]

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

[Magnus Melin [:mkmelin]]

err, I *don't* think we need...

Comment 29

[David Ascher (:davida)]

Attachment: rev 2

I think this addresses all of the review comments.  Have at it =).

Comment 30

[Przemyslaw Bialik]

(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

[Phil Ringnalda (:philor)]

(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 :)

Comment 32

[Magnus Melin [:mkmelin]]

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

Comment 33

[David Ascher (:davida)]

Attachment: rev 3 [checked in]

revised patch as per review comments.  carrying forward r+.

checkin at will.

Comment 34

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/6ea77b011cf5

Comment 35

[Przemyslaw Bialik]

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

[Magnus Melin [:mkmelin]]

Ah yes we forgot about the compose window.

Comment 37

[Mark Banner (:standard8)]

I'm also seeing cmd-1 indicated on the Window menu for Mac builds (probably related to hiddenWindow.xul?).

Comment 38

[Przemyslaw Bialik]

Attachment: rev3 - missing part

Comment 39

[Przemyslaw Bialik]

@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

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

(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

[Magnus Melin [:mkmelin]]

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

Comment 42

[Przemyslaw Bialik]

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

[Magnus Melin [:mkmelin]]

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

Comment 44

[Przemyslaw Bialik]

Attachment: rev3.1 - missing part

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 ?)

Comment 45

[Przemyslaw Bialik]

adjusting summary

Comment 46

[Magnus Melin [:mkmelin]]

(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.

Comment 47

[Magnus Melin [:mkmelin]]

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).

Comment 48

[Przemyslaw Bialik]

Attachment: rev3.1 - missing part - for the checkin [Checkin: See comment 54]

Comment 49

[Serge Gautherie (:sgautherie)]

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

Comment 50

[Serge Gautherie (:sgautherie)]

(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.

Comment 51

[Mark Banner (:standard8)]

(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

[Mark Banner (:standard8)]

Backed out: http://hg.mozilla.org/comm-central/rev/16dd55a2901c

Comment 54

[Serge Gautherie (:sgautherie)]

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
}

Comment 55

[Thomas D. (:thomas8)]

(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.

Comment 56

[Magnus Melin [:mkmelin]]

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.