The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in seamonkey2.0

Status

SeaMonkey
MailNews: General
--
enhancement
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: InvisibleSmiley, Assigned: InvisibleSmiley)

Tracking

({fixed-seamonkey2.0})

Trunk
seamonkey2.0
fixed-seamonkey2.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

5.94 KB, patch
Karsten Düsterloh
: review+
mcsmurf
: review+
neil@parkwaycc.co.uk
: superreview+
Robert Kaiser
: approval-seamonkey2.0+
Details | Diff | Splinter Review
(Assignee)

Description

8 years ago
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).
Attachment #396158 - Flags: superreview?(neil)
Attachment #396158 - Flags: review?(mnyromyr)

Comment 1

8 years ago
Comment on attachment 396158 [details] [diff] [review]
proposed patch

>+    case "Stop":
>+      msgWindow.StopUrls();
Shouldn't this be goDoCommand('cmd_stop') for consistency?
Attachment #396158 - Flags: superreview?(neil) → superreview+
(Assignee)

Comment 2

8 years ago
(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

8 years ago
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. ;-)
Attachment #396158 - Flags: review?(mnyromyr) → review-

Comment 4

8 years ago
> adding two possible candidates. ;-)

Cedric, Frank, are you able to test this?
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 :-)
(Assignee)

Comment 6

8 years ago
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).
Attachment #396158 - Attachment is obsolete: true
Attachment #396546 - Flags: superreview+
Attachment #396546 - Flags: review?(mnyromyr)
(Assignee)

Comment 7

8 years ago
Comment on attachment 396546 [details] [diff] [review]
patch v2

Re-requesting sr for the cmd_stop supportsCommand addition (checked that it works of course)
Attachment #396546 - Flags: superreview+ → superreview?(neil)

Comment 8

8 years ago
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. :)
Attachment #396546 - Flags: review?(mnyromyr)
Attachment #396546 - Flags: review?(bugzilla)
Attachment #396546 - Flags: review+
(Assignee)

Comment 9

8 years ago
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).
Attachment #396546 - Attachment is obsolete: true
Attachment #396743 - Flags: superreview?(neil)
Attachment #396743 - Flags: review?(mnyromyr)
Attachment #396546 - Flags: superreview?(neil)
Attachment #396546 - Flags: review?(bugzilla)
(Assignee)

Updated

8 years ago
Attachment #396743 - Flags: review?(bugzilla)
Comment on attachment 396743 [details] [diff] [review]
patch v3
[Checkin: Comment 16]

Works fine with the Back/Forward/Reload/Search keys of my keyboard.
Attachment #396743 - Flags: review?(bugzilla) → review+
Confirmed for back/forward/stop on my mouse using XP - it's probably the same keycodes internally anyway.
(Assignee)

Comment 12

8 years ago
(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)...

Updated

8 years ago
Attachment #396743 - Flags: review?(mnyromyr) → review+

Updated

8 years ago
Attachment #396743 - Flags: superreview?(neil) → superreview+
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...
(Assignee)

Comment 14

8 years ago
(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?
(Assignee)

Comment 15

8 years ago
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.
Keywords: checkin-needed
Attachment #396743 - Flags: approval-seamonkey2.0?

Updated

8 years ago
Attachment #396743 - Flags: approval-seamonkey2.0? → approval-seamonkey2.0+
Comment on attachment 396743 [details] [diff] [review]
patch v3
[Checkin: Comment 16]


http://hg.mozilla.org/comm-central/rev/53a8efc75603
Attachment #396743 - Attachment description: patch v3 → patch v3 [Checkin: Comment 16]
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.0

Comment 17

8 years ago
Adding fixed-seamonkey2.0 keyword so the queries for approved but not fixed bugs don't catch that bug.
Keywords: fixed-seamonkey2.0
You need to log in before you can comment on or make changes to this bug.