512169 - Port Bug 368177 - Add support for mouse Back and Forward buttons, and the Stop, Search and Bookmarks keys on media keyboards

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: General, enhancement)

Description

[Jens Hatlak (:InvisibleSmiley)]

Attachment: proposed patch

Direct port. I have a multimedia mouse and keyboard and checked that all advertised functions work (back/forward both on mouse and keyboard, the rest on the keyboard).

Comment 1

[[e-mail]]

Comment on attachment 396158 [details] [diff] [review]
proposed patch

>+    case "Stop":
>+      msgWindow.StopUrls();
Shouldn't this be goDoCommand('cmd_stop') for consistency?

Comment 2

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #1)
> (From update of attachment 396158 [details] [diff] [review])
> >+    case "Stop":
> >+      msgWindow.StopUrls();
> Shouldn't this be goDoCommand('cmd_stop') for consistency?

If that worked (which is doesn't for some reason), probably. Comparison:
<http://mxr.mozilla.org/comm-central/search?string=cmd_goForward&find=suite%2Fmailnews&findi=&filter=%5E%5B%5E%5C0%5D*%24&hitlimit=&tree=comm-central>
<http://mxr.mozilla.org/comm-central/search?string=cmd_stop&find=suite%2Fmailnews&findi=&filter=%5E%5B%5E%5C0%5D*%24&hitlimit=&tree=comm-central>

BTW:
- this is in conflict with bug 114853 which was suggested to be WONTFIX'd. Karsten, please advise (it's obvious from my point of view but probably not my decision).
- the browser part was implemented for FF in bug 360731. We should probably port that one as well eventually.

Comment 3

[Karsten Düsterloh]

Comment on attachment 396158 [details] [diff] [review]
proposed patch

I a severe problems here: I don't have a multimedia keyboard to test this (plus I somehow suppose it's Windows only).
I tried to test it in SeaMonkey 2b1 on someone else's WinXP box with a Logitech Ex110, but it didn't work out...

That said, some code nits:

>+++ b/suite/mailnews/msgMail3PaneWindow.js
>+  window.addEventListener("AppCommand", HandleAppCommandEvent, true);

You should free this on close.

>+function HandleAppCommandEvent(evt)
>+{
>+  evt.stopPropagation();
>+  switch (evt.command) {

Braces should go on their own line in SM MailNews land.
<https://wiki.mozilla.org/SeaMonkey:MailNews:CodingStyle>

>+    case "Reload":

You could wire this to the View->Reload command (cmd_reload).

Minussing for the technicalities above, but I'd need a second "WFM" confirmation that it works - adding two possible candidates. ;-)

Comment 4

[Karsten Düsterloh]

> adding two possible candidates. ;-)

Cedric, Frank, are you able to test this?

Comment 5

[Cédric "chewey" Menge]

I'll give it a try. I should at least be able to test the mouse part of it with XP - I haven't managed to assign all my mouse's buttons to something meaningful in Ubuntu yet. This might be a good reason to change that though :-)

Comment 6

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch v2

Addressed nits, made Reload and Home do something sensible. cmd_stop failed because DefaultController.supportsCommand() returned false (cmd_stop case was missing).

Comment 7

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 396546 [details] [diff] [review]
patch v2

Re-requesting sr for the cmd_stop supportsCommand addition (checked that it works of course)

Comment 8

[Karsten Düsterloh]

Comment on attachment 396546 [details] [diff] [review]
patch v2

Looks technically okay, but what about the standalone message window? Some of these should work there as well, like goBack/goForward etc. Do they?

r=me given that it works in the standalone message window as well and mcsmurf can confirm both. :)

Comment 9

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch v3 [Checkin: Comment 16]

(In reply to comment #8)
> (From update of attachment 396546 [details] [diff] [review])
> Looks technically okay, but what about the standalone message window? Some of
> these should work there as well, like goBack/goForward etc. Do they?

No. It's the same in TB. But there mails open in a new tab by default when using double click and the commands are available there, too.

The new patch handles the standalone message window as well (had to add cmd_stop), except Mail Start Page (Home key) which is not available there (and hardly worth adding).

mcsmurf, do you happen to have Tabmail in your build (I don't)? If yes, please also test whether the keys work in a new tab opened for a mail (I don't see why it should fail, but let's play safe here).

Comment 10

[Frank Wein [:mcsmurf]]

Comment on attachment 396743 [details] [diff] [review]
patch v3
[Checkin: Comment 16]

Works fine with the Back/Forward/Reload/Search keys of my keyboard.

Comment 11

[Cédric "chewey" Menge]

Confirmed for back/forward/stop on my mouse using XP - it's probably the same keycodes internally anyway.

Comment 12

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #2)
> - the browser part was implemented for FF in bug 360731. We should probably
> port that one as well eventually.

Strike that, it's all there (navigator.js)...

Comment 13

[[e-mail]]

Comment on attachment 396743 [details] [diff] [review]
patch v3
[Checkin: Comment 16]

It just occurred to me that we could move the code into mailWindow.js although CreateMailWindowGlobals isn't the most ideally named entry point...

Comment 14

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #13)
> (From update of attachment 396743 [details] [diff] [review])
> It just occurred to me that we could move the code into mailWindow.js although
> CreateMailWindowGlobals isn't the most ideally named entry point...

Well, I guess it's possible, its companion OnMailWindowUnload is also called in both unload functions so we could move both the addEventListener and removeEventListener call to mailWindow.js, together with HandleAppCommandEvent (Home would just do nothing in the standalone window then). Karsten, what's your take on this?

Comment 15

[Jens Hatlak (:InvisibleSmiley)]

With the b2 freeze directly ahead and given that the existing patch has all reviews and is tested by more people than just me I'd like to get what we have into the tree now. The suggested cleanup can happen in a second step or bug.

Comment 16

[Serge Gautherie (:sgautherie)]

Comment on attachment 396743 [details] [diff] [review]
patch v3
[Checkin: Comment 16]


http://hg.mozilla.org/comm-central/rev/53a8efc75603

Comment 17

[Robert Kaiser]

Adding fixed-seamonkey2.0 keyword so the queries for approved but not fixed bugs don't catch that bug.