Closed Bug 819652 Opened 12 years ago Closed 11 years ago

Restore wheel settings for horizontal direction

Categories

(SeaMonkey :: Preferences, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.17

People

(Reporter: emk, Assigned: neil)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

Bug 786120 will add the prefs very soon.
Version: unspecified → Trunk
Attached patch Draft patch (obsolete) — Splinter Review
Attachment #690242 - Flags: feedback?(philip.chee)
Attachment #690242 - Flags: feedback?(jh)
Attachment #690242 - Flags: feedback?(iann_bugzilla)
Attachment #690242 - Flags: feedback?(bugzilla)
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.
Attachment #690242 - Flags: feedback?(jh) → feedback+
Comment on attachment 690242 [details] [diff] [review]
Draft patch

Oops sorry!
Attachment #690242 - Flags: feedback?(philip.chee) → feedback?(jh)
Attached patch Proposed patchSplinter Review
Assignee: nobody → neil
Attachment #690242 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #690242 - Flags: feedback?(jh)
Attachment #690242 - Flags: feedback?(iann_bugzilla)
Attachment #690242 - Flags: feedback?(bugzilla)
Attachment #692671 - Flags: review?(philip.chee)
Attachment #692671 - Flags: review?(jh)
Attachment #692671 - Flags: review?(iann_bugzilla)
Attachment #692671 - Flags: review?(bugzilla)
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?]]
Attachment #692671 - Flags: review?(philip.chee) → review+
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.
Attachment #692671 - Flags: review?(jh)
Pushed comm-central changeset cf6e1013c175.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
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 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.
Attachment #692671 - Flags: review?(iann_bugzilla) → review+
(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.
(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
Blocks: 817979
Target Milestone: --- → seamonkey2.17
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.
Oh just saw Jens already commented in Comment 9 on this.
Comment on attachment 692671 [details] [diff] [review]
Proposed patch

Canceling obsolete review request.
Attachment #692671 - Flags: review?(bugzilla)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: