Last Comment Bug 819652 - Restore wheel settings for horizontal direction
: Restore wheel settings for horizontal direction
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Preferences (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.17
Assigned To: neil@parkwaycc.co.uk
:
:
Mentors:
: 819644 (view as bug list)
Depends on: 786120
Blocks: 817979 823647
  Show dependency treegraph
 
Reported: 2012-12-08 06:19 PST by Masatoshi Kimura [:emk]
Modified: 2013-03-22 16:13 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Draft patch (10.73 KB, patch)
2012-12-09 16:56 PST, neil@parkwaycc.co.uk
philip.chee: feedback+
Details | Diff | Splinter Review
Proposed patch (27.60 KB, patch)
2012-12-15 16:10 PST, neil@parkwaycc.co.uk
iann_bugzilla: review+
philip.chee: review+
Details | Diff | Splinter Review
Preference window screenshot (44.76 KB, image/png)
2013-03-22 16:11 PDT, Frank Wein [:mcsmurf]
no flags Details

Description Masatoshi Kimura [:emk] 2012-12-08 06:19:08 PST
Bug 786120 will add the prefs very soon.
Comment 1 neil@parkwaycc.co.uk 2012-12-09 16:56:52 PST
Created attachment 690242 [details] [diff] [review]
Draft patch
Comment 2 Philip Chee 2012-12-10 07:34:29 PST
*** Bug 819644 has been marked as a duplicate of this bug. ***
Comment 3 Philip Chee 2012-12-13 05:02:17 PST
Comment on attachment 690242 [details] [diff] [review]
Draft patch

Layout looks OK. Just a bit cramped. Wonder how this looks like in locales that are more verbose.

Not reviewing the code but some observations:

1. Error Console output:
> Thu Dec 13 2012 20:51:31
> Error: aElement is null
> Source file: chrome://communicator/content/pref/preferences.js
> Line: 16

2. Select the "no modifier key" tab.
In vertical scrolling select "Do nothing"
In horizontal scrolling select "Same as vertical"
Nothce that the mouse wheel speed settings are disabled for both horizontal and vertical directions.
In horizontal scrolling select some other option.
Horizonal speed setting is still disabled.
Comment 4 Philip Chee 2012-12-14 05:10:00 PST
Comment on attachment 690242 [details] [diff] [review]
Draft patch

Oops sorry!
Comment 5 neil@parkwaycc.co.uk 2012-12-15 16:10:01 PST
Created attachment 692671 [details] [diff] [review]
Proposed patch
Comment 6 Philip Chee 2012-12-18 00:46:18 PST
Comment on attachment 692671 [details] [diff] [review]
Proposed patch

Looks good. r=me

> -                  <label value="%"/>
> +              <label value="%"/>
[[Comment: I wonder if this needs to be localized?]]
Comment 7 Jens Hatlak (:InvisibleSmiley) 2012-12-18 01:07:35 PST
Comment on attachment 692671 [details] [diff] [review]
Proposed patch

Review of attachment 692671 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry I didn't have the time to review or even test this; thankfully, Ratty did. If you still want me to look at this, please request feedback and I'll put it on the top of my list (i.e. you shall have it within 24h).

I wonder whether you'd like to remove the stray whitespace while you're at it (see Splinter Review).

::: suite/common/pref/pref-mousewheel.xul
@@ +76,5 @@
>          <tab label="&usingWheelAndShft.label;"/>
>        </tabs>
>  
>        <tabpanels>
>          

Nit: Might want to remove this whitespace.

@@ +132,3 @@
>            </groupbox>
>          </vbox>
>              

Nit: Might want to remove this whitespace.

@@ +186,3 @@
>            </groupbox>
>          </vbox>
>      

Nit: Might want to remove this whitespace.

@@ +240,3 @@
>            </groupbox>
>          </vbox>
>      

Nit: Might want to remove this whitespace.

@@ +294,3 @@
>            </groupbox>
>          </vbox>
>        </tabpanels>     

Nit: Might want to remove this whitespace.

@@ +294,4 @@
>            </groupbox>
>          </vbox>
>        </tabpanels>     
>      </tabbox> 

Nit: Might want to remove this whitespace.
Comment 8 neil@parkwaycc.co.uk 2012-12-18 15:58:54 PST
Pushed comm-central changeset cf6e1013c175.
Comment 9 Jens Hatlak (:InvisibleSmiley) 2012-12-20 11:49:09 PST
Hmm, too bad I didn't come around to checking this before it landed. Now the Mouse Wheel pane is cut off at the bottom on Win7 with the default size of the Preferences window. I didn't spot anything obvious that could be done to reduce the height so I wonder whether we should adjust the default height of the window instead.

BTW I filed bug 823647 to have Help updated for the changes made here.
Comment 10 Ian Neal 2012-12-23 12:17:52 PST
Comment on attachment 692671 [details] [diff] [review]
Proposed patch

>+function doEnablingX(aElement)
Potentially could have passed prefix into the function rather than working it out again.
>+{
>+  var preference = document.getElementById(aElement.getAttribute("preference"));
>+  var prefix = aElement.id.replace(/action_x$/, "");
>+  var value = preference.value;
>+  if (value < 0) {
>+    var action = document.getElementById(prefix + "action");
>+    preference = document.getElementById(action.getAttribute("preference"));
>+    value = preference.value;
>+  }
>   var horizontal = document.getElementById(prefix + "delta_multiplier_x");
>   EnableElement(horizontal, preference.value);
I presume you wanted to change preference.value to value here, otherwise you would not have set value in the if statement.
Comment 11 neil@parkwaycc.co.uk 2012-12-24 16:49:49 PST
(In reply to Ian Neal from comment #10)
> (From update of attachment 692671 [details] [diff] [review])
> >+function doEnablingX(aElement)
> Potentially could have passed prefix into the function rather than working
> it out again.
No, because of the XUL callers.

> >+{
> >+  var preference = document.getElementById(aElement.getAttribute("preference"));
> >+  var prefix = aElement.id.replace(/action_x$/, "");
> >+  var value = preference.value;
> >+  if (value < 0) {
> >+    var action = document.getElementById(prefix + "action");
> >+    preference = document.getElementById(action.getAttribute("preference"));
> >+    value = preference.value;
> >+  }
> >   var horizontal = document.getElementById(prefix + "delta_multiplier_x");
> >   EnableElement(horizontal, preference.value);
> I presume you wanted to change preference.value to value here, otherwise you
> would not have set value in the if statement.
Good catch, I'll push a followup fix.
Comment 12 neil@parkwaycc.co.uk 2012-12-26 03:22:41 PST
(In reply to comment #11)
> (In reply to Ian Neal from comment #10)
> > I presume you wanted to change preference.value to value here, otherwise you
> > would not have set value in the if statement.
> Good catch, I'll push a followup fix.
http://hg.mozilla.org/comm-central/rev/a6aa9df9e3ea
Comment 13 Frank Wein [:mcsmurf] 2013-03-22 16:11:59 PDT
Created attachment 728497 [details]
Preference window screenshot

Although the review request for myself is obsolete, one question/comment about the wheel settings pref pane: Is it possible to automatically adjust the pref window height for this pref pane? Or would the general pref window height have to be changed? See the screenshot for how it looks for me (looks the same on Windows 7). The height of the pref window is a bit too small.
Comment 14 Frank Wein [:mcsmurf] 2013-03-22 16:13:19 PDT
Oh just saw Jens already commented in Comment 9 on this.
Comment 15 Frank Wein [:mcsmurf] 2013-03-22 16:13:42 PDT
Comment on attachment 692671 [details] [diff] [review]
Proposed patch

Canceling obsolete review request.

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