514927 - Pref Trackpoint hack to avoid breaking even more things

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Rain [:rain1] (inactive)]

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)

Comment 1

[Ria Klaassen (not reading all bugmail)]

I installed it on Windows Vista and indeed it doesn't scroll.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

So the hack used in Bug 507222 breaks Katmouse ...  Not quite sure if we can work around this.

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+#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.

Comment 9

[Rain [:rain1] (inactive)]

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?

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

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

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Kyle, are you intending to finish this off or should we get someone else to take it over?

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

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

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

> +// 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.

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

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

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 19

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Thanks for the comment everyone.  New patch later tonight.

Comment 20

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

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.

Comment 22

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch V2.1.1

Relocated the pref in all.js per Masayuki's instructions.  Sorry not understanding the previous instructions.

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

great, thanks.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 404623 [details] [diff] [review]
Patch V2.1.1

great, thanks!

Comment 25

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

Comment 26

[Joe Drew (not getting mail)]

http://hg.mozilla.org/mozilla-central/rev/7c9e123dbb32

Comment 27

[Joe Drew (not getting mail)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/5f2a6c970a96

Comment 28

[Barry Marshall]

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.

Comment 29

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

[Barry Marshall]

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

[Barry Marshall]

Also...

HKEY_CURRENT_USER\Software\Lenovo\PRDCTR\Client\UltraNav
HKEY_CURRENT_USER\Software\Synaptics\SynTPEnh\UltraNavPS2

Comment 32

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #31)
> HKEY_CURRENT_USER\Software\Synaptics\SynTPEnh\

Is what we need to add.

Comment 33

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(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

[Barry Marshall]

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

Comment 35

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Ok thanks, SynTPEnh is probably what we want then.  I'll work up a patch later today.

Comment 36

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: 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).

Comment 37

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

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.

Comment 38

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

Comment 39

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Reopening this to track adding registry keys to the detection code.

Comment 40

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch to add SynTPEnh V2

Masayuki is right, we can use NS_ARRAY_LENGTH to make this simpler.

Comment 41

[Barry Marshall]

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.

Comment 42

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 43

[Barry Marshall]

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

Comment 45

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Changing status so this shows up on queries.

Comment 46

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/03b2895110e0

Comment 47

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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