Restore wheel settings for horizontal direction

RESOLVED FIXED in seamonkey2.17

Status

SeaMonkey
Preferences
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: emk, Assigned: neil@parkwaycc.co.uk)

Tracking

(Blocks: 1 bug)

Trunk
seamonkey2.17
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Bug 786120 will add the prefs very soon.

Updated

5 years ago
Version: unspecified → Trunk
(Assignee)

Comment 1

5 years ago
Created attachment 690242 [details] [diff] [review]
Draft patch
Attachment #690242 - Flags: feedback?(philip.chee)
Attachment #690242 - Flags: feedback?(jh)
Attachment #690242 - Flags: feedback?(iann_bugzilla)
Attachment #690242 - Flags: feedback?(bugzilla)

Updated

5 years ago
Duplicate of this bug: 819644

Comment 3

5 years ago
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 4

5 years ago
Comment on attachment 690242 [details] [diff] [review]
Draft patch

Oops sorry!
Attachment #690242 - Flags: feedback?(philip.chee) → feedback?(jh)
(Assignee)

Comment 5

5 years ago
Created attachment 692671 [details] [diff] [review]
Proposed patch
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 6

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

Comment 8

5 years ago
Pushed comm-central changeset cf6e1013c175.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Blocks: 823647
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

5 years ago
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+
(Assignee)

Comment 11

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

Comment 12

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

Updated

5 years ago
Blocks: 817979

Updated

5 years ago
Target Milestone: --- → seamonkey2.17
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.
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.