-
-
Notifications
You must be signed in to change notification settings - Fork 634
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
routing broken in notepad++ #4703
Comments
Comment 1 by jteh on 2014-12-18 02:00 I'm not sure whether we can support this or whether it is worth the effort, but either way, we probably shouldn't just click the centre of the window. :) |
Double clicking would be more useful than single clicking, but I'm not sure if NVDA supports that, i.e. by quickly pressing the same routing button twice. Of course it is of little use if the center of the window is clicked. Same for performing right clicks. I would rather that nothing happens, so the routing buttons don't do anything but route the cursor. I suppose NVDA could route by moving the mouse and faking a click, as some screen readers do, but this seems ugly. Maybe we can use |
The SCI_POINT*FROMPOSITION messages sound like they might do the trick.
The returned coordinates probably need to be tweaked for DPI unaware
apps though. See windowUtils.logicalToPhysicalPoint.
|
Actually, ScintillaTextInfo already implements _getPointFromOffset (and _getOffsetFromPoint). I can make a few educated guesses as to why it isn't working as expected:
|
For reference, the Scintilla doc says:
This sounds like it returns actual pixels, but the point being off by (exactly) five lines suggests there is a fixed strip of pixels not taken into account. Going to run some tests. |
Yeah, definitely client coordinates: they start at y=0. The resulting mouse click does indeed sometimes scroll to five times the line height higher than where the caret was. For example, my lines are 23px high and when I click at y=644 the caret moves to y=529. Another side-effect of the current implementation of |
Note that you've addressed client coordinates, but not DPI awareness
(logicalToPhysicalPoint). Unfortunately, the latter is annoying to
address for client coordinates because you first have to convert the
client rect to logical, add the logical client coordinates to get screen
coordinates, then convert to physical.
If activate is checking point.x and point.y, it should definitely be not
None; 0 should be acceptable, even if rare.
|
I’m not sure how to get screen pixels based on scintilla’s client coordinates. Is there a handy function for this? I’m thinking you could add the window’s x and y coordinates to the reported coords for a text offset, but that may be too simplistic. The only way to avoid clicking in the middle of the window seems to be to redefine TextInfo.activate. But even so, this coords thing should be addressed e.g. to fix mouse tracking. From: James Teh [mailto:[email protected]] Note that you've addressed client coordinates, but not DPI awareness If activate is checking point.x and point.y, it should definitely be not — |
Normally, you'd use the ClientToScreen function, but that doesn't
account for DPI issues. So, you have to convert the client top and left
to logical coordinates, add the text point and then convert back to
physical. DPI scaling is painful.
TextInfo.activate uses pointAtStart, so once _pointFromOffset is fixed,
it will behave as expected, barring the 0 problem which needs to be fixed.
|
Ah yes, provided winUser.getCursorPos() returns the right info. It seems the mouse is always in the center of the window in Notepad++. From: James Teh [mailto:[email protected]] Normally, you'd use the ClientToScreen function, but that doesn't TextInfo.activate uses pointAtStart, so once _pointFromOffset is fixed, — |
Oi, _getLineOffsets also needs to be tweaked (this would use ScreenToClient). The fix for the initial problem using ClientToScreen works as expected on my 1080p display, which evidently doesn't suffer from DPI badness. I'll get a working patch done for this first, then figure out DPI. |
…ation under the mouse. TODO: make this DPI-aware. re nvaccess#4703, nvaccess#5450
I'm very confused. TextInfo.activate has nothing to do with
getCursorPos. It uses TextInfo.pointAtStart, which is the coordinate at
which the text in this TextInfo starts.
|
Well it’s not the NVDA version: oldX,oldY=winUser.getCursorPos() This shouldn’t affect where the mouse is being clicked, just the place where it is then returned to. Either way, using ClientToScreen/ScreenToClient works. Can I test DPI by scaling my display, say to 75%? From: James Teh [mailto:[email protected]] I'm very confused. TextInfo.activate has nothing to do with — |
Scaling your display should allow you to test DPI, yes. I'm not sure if
Notepad++ is DPI aware or not, though. I'm guessing not, but I'm not
certain. It is DPI unaware apps that pose the problem. And you can't
assume that all Scintilla apps will be one way or the other.
|
So DPI-awareness doesn’t belong into the scintilla NVDAObject? Or are these DPI conversions ‘harmless’ to do if not needed? Because it was always exactly 5 lines that the cursor jumped when routing I’m reasonably sure that the use of client coordinates was the issue here. From: James Teh [mailto:[email protected]] Scaling your display should allow you to test DPI, yes. I'm not sure if — |
They're harmless if not needed. And yes, using client coordinates is absolutely a problem. It's just that on a high DPI system with a DPI unaware app, that's yet another problem to deal with that will also screw up your coordinates.
|
It looks like Scintilla is DPI-aware. That is, My intention is to stick with client coordinates in |
By the way, any thoughts on wrapping ClientToScreen into windowUtils? |
Actually, Maybe we can deal with the DPI stuff in a separate issue if/when it comes up in real use? |
Okay. This is getting confusing. Let's try to simplify.
The first thing to note is that if there's no DPI scaling (e.g. you
don't have a high DPI display), you'll get the same coordinates
(physical and logical) regardless of DPI awareness.
NVDA always wants to deal with physical (DPI aware) coordinates. Thus,
if your coordinates might be DPI unaware (logical), you must always
convert them with LogicalToPhysicalPoint. Similarly, if you're passing
coordinates from NVDA to something that might be DPI unaware, you must
convert them with PhysicalToLogicalPoint. If the app is already DPI
aware, conversion is a no-op.
Regarding _getLineOffsets, if the app isn't DPI aware, the width
calculated there will be wrong. It'll probably seem to work in most
cases because the coordinate you pass will be larger than the coordinate
for the end of the line, so Scintilla will probably just constrain it down.
|
So here is what I am getting so far:
If in the above scenario I add conversions from logical to physical coordinates the mouse click behavior breaks. So what do you do in such a situation, convert everything so it stays in that 1920×1080 space and avoid using |
I'm still not quite sure why this is relevant. Sure, it does call that, but only to get the original coordinates so it can return the mouse there after clicking. The important line is after that:
where
Not quite. If you get those client coordinates from a DPI unaware app, those client coordinates don't account for DPI scaling. Client coordinates are just screen coordinates relative to the top left of the client area of the window. That's why you can't just call ClientToScreen. You have to get the DPI unaware (logical) client top left, add the client coordinates you want and then convert them back to physical coordinates. This is why DPI scaling frustrates me to no end. As a final clarification, you don't need to mess with the coordinates returned by getCursorPos. You only need to mess with the clients you're returning from _getPointFromOffset (and accepting in _getffsetFromPoint). |
My question re GetCursorPos is simply: if it returns different coordinates depending on DPI, is that also what SetCursorPos expects, or does it always have to be the same coordinates regardless of DPI? I’m pretty sure it’s the first, since that is how GetCursorPos works, but MSDN isn’t very clear. Too bad Scintilla doesn’t have a ‘click at this offset’ message! From: James Teh [mailto:[email protected]]
I'm still not quite sure why this is relevant. Sure, it does call that, but only to get the original coordinates so it can return the mouse there after clicking. The important line is after that: winUser.setCursorPos(p.x,p.y) where p was retrieved earlier: p=self.pointAtStart
Not quite. If you get those client coordinates from a DPI unaware app, those client coordinates don't account for DPI scaling. Client coordinates are just screen coordinates relative to the top left of the client area of the window. That's why you can't just call ClientToScreen. You have to get the DPI unaware (logical) client top left, add the client coordinates you want and then convert them back to physical coordinates. This is why DPI scaling frustrates me to no end. As a final clarification, you don't need to mess with the coordinates returned by getCursorPos. You only need to mess with the clients you're returning from _getPointFromOffset (and accepting in _getffsetFromPoint). — |
In Windows 8 and earlier, the behaviour of GetCursorPos and SetCursorPos in a DPI aware app on a high DPI system is unclear at best. Sure, GetCursorPos could return DPI unaware coordinates, but SetCursorPos can't really do that because it doesn't take an HWND, so it doesn't know what process it's dealing with. This is why you're supposed to use (and why we use) Get/SetPhysicalCursorPos. In Windows 8.1 and later, for a DPI aware app, I believe Get/SetCursorPos is identical to Get/SetPhysicalCursorPos. So, this is why I say that the rule in NVDA is that you must always deal with physical coordinates. |
It looks like Long story short, what does 'physical' even mean? It sounds like the answer depends on who you ask. |
Well, they do in Windows 8.1 and later because DPI awareness is supposed to be more transparent there. Your app no longer has to care about whether you're dealing with DPI aware or DPI unaware coordinates. Thus, all of these functions just deal with coordinates based on whether or not your app is DPI aware. Unfortunately, we're not just dealing with coordinates reported by our own process like most apps do, which is why this is so difficult. obj.location should be physical, though there are some cases where this breaks. Suffice to say that in Notepad++, they will be physical. Physical means DPI aware screen coordinates. Logical means screen coordinates from the perspective of the app; i.e. if it's DPI aware, they'll be physical, but if it's not DPI aware, they'll be screen coordinates scaled to 96 DPI. |
I tried this in d38408b, but the resulting clicks are far less accurate than when simply using |
I'm pretty sure this is fixed in #9427. |
I can no longer reproduce this, thanks to #9427. Closing. |
Reported by driemer.riemer@... on 2014-12-17 04:58
In notepad++, double tapping the routing button, or accidentally clicking on the same routing button that was previously pressed will jump you 6 lines up from your current possition..
reproduction
log snippet.
DEBUG - (21:51:37):
starting snippet.
IO - speech.speak (21:51:37):
Speaking [- braille.BrailleBuffer.update (21:51:37):
Braille regions text: [u'NVDA Python Console ', u'>>> edt ', ' ']('>>>']
IO)
IO - braille.BrailleHandler.update (21:51:37):
Braille window dots: -
IO - braille.BrailleHandler.update (21:51:37):
Braille window dots: -
IO - inputCore.InputManager.executeGesture (21:51:39):
Input: kb(laptop):escape
IO - speech.speak (21:51:40):
Speaking Riemer\Google Drive\calc\practice\tues dec 16.txt - Notepad++'
IO - braille.BrailleBuffer.update (21:51:40):
Braille regions text: Riemer\Google Drive\calc\practice\tues dec 16.txt - Notepad++'
IO - braille.BrailleHandler.update (21:51:40):
Braille window dots: 16 147 156 12567 1367 234 15 1235 234 12567 1457 15 1235 15 13 - 12357 24 15 134 15 1235 12567 12457 135 135 1245 123 15 -
IO - speech.speak (21:51:40):
Speaking edit multi line'
IO - speech.speak (21:51:40):
Speaking -xcos x+sin x+c\r\n'
IO - braille.BrailleBuffer.update (21:51:40):
Braille regions text: -xcos x+sin x+c '
IO - braille.BrailleHandler.update (21:51:40):
Braille window dots: 123456 - 36 1346 14 135 234 - 1346 346 234 24 1345 - 1346 346 14 -
IO - braille.BrailleHandler.update (21:51:40):
Braille window dots: 123456 - 36 1346 14 135 234 - 1346 346 234 24 1345 - 1346 346 14 -
IO - inputCore.InputManager.executeGesture (21:51:45):
Input: br(freedomscientific):routing
IO - braille.BrailleBuffer.update (21:51:45):
Braille regions text: -xcos x+sin x+c '
IO - braille.BrailleHandler.update (21:51:45):
Braille window dots: 123456 - 36 1346 14 135 234 - 1346 346 234 24 1345 - 1346 346 14 -
IO - braille.BrailleHandler.update (21:51:45):
Braille window dots: 123456 - 36 1346 14 135 234 - 1346 346 234 24 1345 - 1346 346 14 -
IO - inputCore.InputManager.executeGesture (21:51:48):
Input: br(freedomscientific):routing
IO - braille.BrailleBuffer.update (21:51:48):
Braille regions text: |y|+c = !xsin xdx '
IO - braille.BrailleHandler.update (21:51:48):
Braille window dots: 123 1345 - 1256 13456 1256 346 14 - 123456 - 2346 1346 234 24 1345 - 1346 145 1346 -
IO - braille.BrailleHandler.update (21:51:48):
Braille window dots: 123 1345 - 1256 13456 1256 346 14 - 123456 - 2346 1346 234 24 1345 - 1346 145 1346 -
IO - inputCore.InputManager.executeGesture (21:51:52):
Input: kb(laptop):NVDA+f1
No error is triggered either, so this may be a pain to figure out. Let me know if I can help track it down.
Putting the numbers 1 through 100 each on their own line may be useful for this to see where the jumps take you.
The text was updated successfully, but these errors were encountered: