Pref Trackpoint hack to avoid breaking even more things

RESOLVED FIXED

Status

()

Core
Widget: Win32
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: sid0, Assigned: khuey)

Tracking

({regression})

Trunk
x86
Windows 7
regression
Points:
---
Bug Flags:
wanted1.9.2 ?

Firefox Tracking Flags

(status1.9.2 beta2-fixed)

Details

Attachments

(2 attachments, 4 obsolete attachments)

I use a utility called Katmouse <http://ehiti.de/katmouse/> to allow me to scroll the window the mouse is currently hovering on instead of the focused one. (This brings the Windows behaviour in line with Mac and Linux). Mouse wheel scrolling with Katmouse enabled seems to be completely broken on the latest Minefield nightly:

Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a1pre) Gecko/20090906 Minefield/3.7a1pre (.NET CLR 3.5.30729)

It works with the latest Namoroka nightly though:
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2a2pre) Gecko/20090905 Namoroka/3.6a2pre (.NET CLR 3.5.30729)
(Reporter)

Updated

8 years ago
OS: Mac System 7 → Windows 7
I installed it on Windows Vista and indeed it doesn't scroll.
WFM on Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a1pre) Gecko/20090902 Minefield/3.7a1pre

Broken on Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a1pre) Gecko/20090903 Minefield/3.7a1pre

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=75af94f85a98&tochange=3cab7a0c2c3d

That seems to make me the prime suspect :-D
Assignee: nobody → me
Keywords: regression
So the hack used in Bug 507222 breaks Katmouse ...  Not quite sure if we can work around this.
Blocks: 507222
With the hack in place Katmouse no longer forwards WM_MOUSEWHEEL messages to Mozilla.  

Amusingly the Trackpoint scroll still works, it's just actual wheel mice that break.
Uck, I'm really afraid of piling hacks on top of hacks here.

Can we go back to the original patch and do some direct detection of the Thinkpad driver and only add the scrollbar in that case?
(In reply to comment #5)
> Uck, I'm really afraid of piling hacks on top of hacks here.
> 
> Can we go back to the original patch and do some direct detection of the
> Thinkpad driver and only add the scrollbar in that case?

Yeah I agree, that leads down a road that probably does not end well.  We could easily switch the existing hack on/off.  (If we're going to do driver detection making it a pref is probably best so if detection fails users can override it.)  I'll have to think a bit on the best way to detect the driver though.
Created attachment 399004 [details] [diff] [review]
Patch to pref trackpoint hack

Pref the trackpoint hack to "win.trackpoint" (paralleling the existing OS/2 hack at os2.trackpoint) and add some simple autodetect code.  Autodetection is done by looking for the driver file in places it is likely to live.  The Lenovo path is where trackpoint drivers are installed on new machines, the SynTP path is where they're installed on older machines, and on really old machines they're installed in System32.  There should be no false positives (although there will be some false negatives, but that's what about:config is for)
Attachment #399004 - Flags: review?(roc)
+#define MOZ_TP_DRIVER_CHECK(x)                                                \
+          driver=::CreateFileW(x,                                             \
+                               0, FILE_SHARE_WRITE || FILE_SHARE_DELETE, NULL,\
+                               OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);   \
+          if(driver != INVALID_HANDLE_VALUE)                                  \
+          {                                                                   \
+            prefs->SetBoolPref("win.trackpoint", true);                       \
+            nsWindow::sTrackPointHack = true;                                 \
+            CloseHandle(driver);                                              \
+          }
+  MOZ_TP_DRIVER_CHECK(L"C:\\Program Files\\Lenovo\\TrackPoint\\tp4serv.exe");
+  MOZ_TP_DRIVER_CHECK(L"C:\\Program Files\\Synaptics\\SynTP\\tp4serv.exe");
+  MOZ_TP_DRIVER_CHECK(L"C:\\Windows\\System32\\tp4serv.exe");

Instead of this macro, I'd prefer an array of 3 strings with a loop over them.

You shouldn't really be setting a pref in there. We should only set prefs based on user actions. Maybe we need a 3-state pref --- enable, disable, auto? We'd normally do that with an int pref, say -1 is auto, 0 is disable, 1 is enable. You'll want to add the default value to all.js though.

+    ::CreateWindowW(L"SCROLLBAR", L"FAKETRACKPOINTSCROLLBAR", 
+                    WS_CHILD | WS_VISIBLE, 0,0,0,0, mWnd, NULL,
+                    nsToolkit::mDllInstance, NULL);

{} around this.
(Reporter)

Comment 9

8 years ago
I don't think hardcoding paths to C:\Program Files or C:\Windows is a good idea at all. People might have their Program Files or Windows in different places, plus there's 64-bit Windows to consider. Can't you just look at the registry instead and see whether a Lenovo key is present?
(In reply to comment #9)
> I don't think hardcoding paths to C:\Program Files or C:\Windows is a good idea
> at all. People might have their Program Files or Windows in different places,
> plus there's 64-bit Windows to consider. Can't you just look at the registry
> instead and see whether a Lenovo key is present?

right, we should check the registry. however, please note that the registry key might remain after you uninstall the driver. you need to find what registry key can be used for the check.
It appears from my system that we can check HKCU/Lenovo/TrackPoint to determine if the software is installed.  I don't have access to an older Thinkpad though to see what the pre-Lenovo key is.
Attachment #399004 - Attachment is obsolete: true
Attachment #399004 - Flags: review?(roc)
Kyle, are you intending to finish this off or should we get someone else to take it over?
Flags: wanted1.9.2?
I will finish it off, I really need some more input from other Trackpoint users on older systems though before I can write a patch that covers older systems.
(In reply to comment #11)
> It appears from my system that we can check HKCU/Lenovo/TrackPoint to determine
> if the software is installed.  I don't have access to an older Thinkpad though
> to see what the pre-Lenovo key is.

I was noticed by a Japanese contributor. Some thinkpad models use UltraNav instead of TrackPoint.
Created attachment 404503 [details] [diff] [review]
Patch V2

Pref the TrackPoint hack.  This version looks at the registry instead of at the filesystem.  It also uses the three setting pref that roc suggested.
Attachment #404503 - Flags: review?(roc)
> +// TrackPoint Hack (Bug 514927)
> +#ifndef WINCE
> +pref("win.trackpoint", -1);
> +#endif
> +

I think that "ui.trackpoint_hack.enabled" is better, and you should describe the meaning of the value here. It might help the community members who support other users. (all.js can be watched by the users.)

> +  if(NS_SUCCEEDED(rv) && prefs)
> +  {

The '{' should be the end of the previous line.
https://developer.mozilla.org/en/Mozilla_Coding_Style_Guide#Control_Structures
Other for loop and if statement are same.

And I worry that this patch might make damage to the ts score (ts test is the name of the starting-up time) because it accesses the registry at starting up. I think that the accessing time to the registry isn't fast.
(In reply to comment #16)
> > +// TrackPoint Hack (Bug 514927)
> > +#ifndef WINCE
> > +pref("win.trackpoint", -1);
> > +#endif
> > +
> 
> I think that "ui.trackpoint_hack.enabled" is better, and you should describe
> the meaning of the value here. It might help the community members who support
> other users. (all.js can be watched by the users.)

Oh, and looks like you add the new pref to all non-WinCE platforms (i.e., including Linux and Mac). It's bad, of course.

You should insert the pref after pref("mousewheel.system_scroll_override_on_root_content.enabled", true);
http://mxr.mozilla.org/mozilla-central/source/modules/libpref/src/init/all.js#1626
The line is in the #ifdef XP_WIN block.
(In reply to comment #16)
> And I worry that this patch might make damage to the ts score (ts test is the
> name of the starting-up time) because it accesses the registry at starting up.
> I think that the accessing time to the registry isn't fast.

I don't think we should worry about that. We're only reading two keys.

I think the "Init Trackpoint Hack" code should be broken out into its own static function though.
Thanks for the comment everyone.  New patch later tonight.
Created attachment 404587 [details] [diff] [review]
Patch V2.1

Incorporates comments from Robert and Masayuki.  No substantive functionality changes.

The way the pref ui.trackpoint_hack.enabled works is that it takes one of three values:
0:    Off: The trackpoint hack is disabled.
1:     On: The trackpoint hack is enabled.
-1:  Auto: The trackpoint hack is turned on only if certain registry keys belonging to the TrackPoint driver are detected.
Any other value is treated as 0 and the hack is disabled.
Attachment #404503 - Attachment is obsolete: true
Attachment #404587 - Flags: review?(roc)
Attachment #404503 - Flags: review?(roc)
Comment on attachment 404587 [details] [diff] [review]
Patch V2.1

> +// TrackPoint Hack (Bug 514927)
> +#ifdef XP_WIN
> +#ifndef WINCE
> +pref("ui.trackpoint_hack.enabled", -1);
> +#endif
> +#endif
> +

You should move this to the large #ifdef XP_WIN block as I said in the previous comment. The platform specific pref should be there.

Please imagine that there are very many #ifdefs in the all.js. We shouldn't scatter the #ifdefs.

And also please describe the meaning of the value at the comment in all.js. Not here.
Created attachment 404623 [details] [diff] [review]
Patch V2.1.1

Relocated the pref in all.js per Masayuki's instructions.  Sorry not understanding the previous instructions.
Attachment #404587 - Attachment is obsolete: true
Attachment #404623 - Flags: review?(roc)
Attachment #404587 - Flags: review?(roc)
great, thanks.
Comment on attachment 404623 [details] [diff] [review]
Patch V2.1.1

great, thanks!
Attachment #404623 - Flags: review?(roc) → review+
Keywords: checkin-needed
Whiteboard: [needs landing]
(In reply to comment #24)
> (From update of attachment 404623 [details] [diff] [review])
> great, thanks!

(In reply to comment #23)
> great, thanks.

No problem at all.  Thanks for the link to the Coding Style document Masayuki, that will be very useful to me in the future.
Attachment #404623 - Flags: approval1.9.2+
http://hg.mozilla.org/mozilla-central/rev/7c9e123dbb32
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Keywords: checkin-needed
Whiteboard: [needs landing]
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/5f2a6c970a96
status1.9.2: --- → beta1-fixed

Comment 28

8 years ago
Can you define where in the Registry you're searching for this key?

Whatever it is, I don't seem to have it and my Thinkpad T61's Trackpoint is not registering.
This looks for HKCU/Software/Lenovo/Trackpoint and HKCU/Software/Lenovo/Ultranav.  If you have additional registry keys that we should use, please let me know.  I couldn't find anyone with an older Thinkpad to look at.

Comment 30

8 years ago
Some more alternatives from my T61...

HKEY_LOCAL_MACHINE\SOFTWARE\Lenovo\ThinkPad UltraNav Utility
HKEY_LOCAL_MACHINE\SOFTWARE\Lenovo\ProdCenter\Client\UltraNav
HKEY_LOCAL_MACHINE\SOFTWARE\Lenovo\ThinkPad FnF8

Comment 31

8 years ago
Also...

HKEY_CURRENT_USER\Software\Lenovo\PRDCTR\Client\UltraNav
HKEY_CURRENT_USER\Software\Synaptics\SynTPEnh\UltraNavPS2
(In reply to comment #31)
> HKEY_CURRENT_USER\Software\Synaptics\SynTPEnh\

Is what we need to add.
(In reply to comment #32)
> (In reply to comment #31)
> > HKEY_CURRENT_USER\Software\Synaptics\SynTPEnh\
> 
> Is what we need to add.

Actually that may not be the one we want.  If you search your registry for "TrackPoint" do you find anything?

Comment 34

8 years ago
Only as a substring of value string names.   There are no Registry Keys with just TrackPoint.

For example in HKEY_CURRENT_USER\Software\Synaptics\SynTPEnh\UltraNavPS2 there is:

[HKEY_CURRENT_USER\Software\Synaptics\SynTPEnh\UltraNavPS2]
"TrackPointMode"=dword:00001214
"TrackPointModeFunction"=dword:00000011
Ok thanks, SynTPEnh is probably what we want then.  I'll work up a patch later today.
Created attachment 405482 [details] [diff] [review]
Patch to add SynTPEnh to the detection

Should make the detection work for Barry (and all the other people who have the same drivers he does).
Attachment #405482 - Flags: review?(roc)
Attachment #405482 - Flags: approval1.9.2?
Comment on attachment 405482 [details] [diff] [review]
Patch to add SynTPEnh to the detection

> -        for(int i = 0; i < 2; i++) {
> +        for(int i = 0; i < 3; i++) {

Cannot you use |NS_ARRAY_LENGTH|?
# I'm not sure it can be used for the two dimensional array.
(In reply to comment #37)
> (From update of attachment 405482 [details] [diff] [review])
> > -        for(int i = 0; i < 2; i++) {
> > +        for(int i = 0; i < 3; i++) {
> 
> Cannot you use |NS_ARRAY_LENGTH|?
> # I'm not sure it can be used for the two dimensional array.
Judging by the macro definition at http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsMemory.h#156 it looks like it can be.  I'll test it.  Thanks for the tip.
Reopening this to track adding registry keys to the detection code.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 405693 [details] [diff] [review]
Patch to add SynTPEnh V2

Masayuki is right, we can use NS_ARRAY_LENGTH to make this simpler.
Attachment #405482 - Attachment is obsolete: true
Attachment #405693 - Flags: review?(roc)
Attachment #405693 - Flags: approval1.9.2?
Attachment #405482 - Flags: review?(roc)
Attachment #405482 - Flags: approval1.9.2?
Attachment #405693 - Flags: review?(roc)
Attachment #405693 - Flags: review+
Attachment #405693 - Flags: approval1.9.2?
Attachment #405693 - Flags: approval1.9.2+
Keywords: checkin-needed

Comment 41

8 years ago
Is it worth modifying the summary of this bug to mention the Trackpoint?  The code being changed really is due to it, not a Katmouse bug.
Thanks roc!

http://hg.mozilla.org/mozilla-central/rev/f7484ca1d5c6

(In reply to comment #41)
> Is it worth modifying the summary of this bug to mention the Trackpoint?  The
> code being changed really is due to it, not a Katmouse bug.

I agree.  Done.
Summary: Mouse wheel scrolling broken with Katmouse enabled → Pref Trackpoint hack to avoid breaking even more things

Comment 43

8 years ago
WFM in Minefield nightly now.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20091015 Minefield/3.7a1pre (.NET CLR 3.5.30729) ID:20091015051159
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago8 years ago
Resolution: --- → FIXED
Whiteboard: [needs 192 landing]
Duplicate of this bug: 522474
Changing status so this shows up on queries.
status1.9.2: beta1-fixed → ---
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/03b2895110e0
status1.9.2: --- → final-fixed
Keywords: checkin-needed
Whiteboard: [needs 192 landing]
(In reply to comment #46)
> http://hg.mozilla.org/releases/mozilla-1.9.2/rev/03b2895110e0

Thanks Dao and everyone else!

If we find that we need to add any more keys lets file separate bugs blocking bug 507222.
You need to log in before you can comment on or make changes to this bug.