819652 - Restore wheel settings for horizontal direction

Status: RESOLVED FIXED

(SeaMonkey :: Preferences, defect)

Description

[Masatoshi Kimura [:emk]]

Bug 786120 will add the prefs very soon.

Comment 1

[[e-mail]]

Attachment: Draft patch

Comment 3

[Philip Chee]

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]

Comment on attachment 690242 [details] [diff] [review]
Draft patch

Oops sorry!

Comment 5

[[e-mail]]

Attachment: Proposed patch

Comment 6

[Philip Chee]

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

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

[[e-mail]]

Pushed comm-central changeset cf6e1013c175.

Comment 9

[Jens Hatlak (:InvisibleSmiley)]

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]

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

[[e-mail]]

(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

[[e-mail]]

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

Attachment: 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]]

Oh just saw Jens already commented in Comment 9 on this.

Comment 15

[Frank Wein [:mcsmurf]]

Comment on attachment 692671 [details] [diff] [review]
Proposed patch

Canceling obsolete review request.