Last Comment Bug 512169 - Port Bug 368177 - Add support for mouse Back and Forward buttons, and the Stop, Search and Bookmarks keys on media keyboards
: Port Bug 368177 - Add support for mouse Back and Forward buttons, and the Sto...
Status: RESOLVED FIXED
: fixed-seamonkey2.0
Product: SeaMonkey
Classification: Client Software
Component: MailNews: General (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: seamonkey2.0
Assigned To: Jens Hatlak (:InvisibleSmiley)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-08-23 13:10 PDT by Jens Hatlak (:InvisibleSmiley)
Modified: 2009-09-08 11:13 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
proposed patch (1.42 KB, patch)
2009-08-23 13:10 PDT, Jens Hatlak (:InvisibleSmiley)
mnyromyr: review-
neil: superreview+
Details | Diff | Splinter Review
patch v2 (2.46 KB, patch)
2009-08-25 14:32 PDT, Jens Hatlak (:InvisibleSmiley)
mnyromyr: review+
Details | Diff | Splinter Review
patch v3 [Checkin: Comment 16] (5.94 KB, patch)
2009-08-26 10:00 PDT, Jens Hatlak (:InvisibleSmiley)
mnyromyr: review+
bugzilla: review+
neil: superreview+
kairo: approval‑seamonkey2.0+
Details | Diff | Splinter Review

Description Jens Hatlak (:InvisibleSmiley) 2009-08-23 13:10:40 PDT
Created attachment 396158 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2009-08-24 02:36:19 PDT
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) 2009-08-24 09:51:43 PDT
(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 2009-08-24 13:47:11 PDT
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 2009-08-24 13:48:33 PDT
> adding two possible candidates. ;-)

Cedric, Frank, are you able to test this?
Comment 5 Cédric "chewey" Menge 2009-08-24 16:48:24 PDT
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) 2009-08-25 14:32:53 PDT
Created attachment 396546 [details] [diff] [review]
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) 2009-08-25 14:54:50 PDT
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 2009-08-25 15:51:01 PDT
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) 2009-08-26 10:00:11 PDT
Created attachment 396743 [details] [diff] [review]
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] 2009-08-26 12:46:51 PDT
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 2009-08-26 13:31:53 PDT
Confirmed for back/forward/stop on my mouse using XP - it's probably the same keycodes internally anyway.
Comment 12 Jens Hatlak (:InvisibleSmiley) 2009-08-26 14:28:30 PDT
(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 neil@parkwaycc.co.uk 2009-08-31 16:48:08 PDT
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) 2009-09-01 00:42:00 PDT
(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) 2009-09-01 16:50:46 PDT
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) 2009-09-05 07:27:10 PDT
Comment on attachment 396743 [details] [diff] [review]
patch v3
[Checkin: Comment 16]


http://hg.mozilla.org/comm-central/rev/53a8efc75603
Comment 17 Robert Kaiser 2009-09-08 11:13:03 PDT
Adding fixed-seamonkey2.0 keyword so the queries for approved but not fixed bugs don't catch that bug.

Note You need to log in before you can comment on or make changes to this bug.