Skip to content
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

Closed
nvaccessAuto opened this issue Dec 17, 2014 · 30 comments
Closed

routing broken in notepad++ #4703

nvaccessAuto opened this issue Dec 17, 2014 · 30 comments

Comments

@nvaccessAuto
Copy link

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

  1. In notepad++ press a routing key.
  2. Press the same routing key again.
  3. You land somewhere completely different in the document.

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.

@nvaccessAuto
Copy link
Author

Comment 1 by jteh on 2014-12-18 02:00
Pressing a routing button over the cell where the caret is located "activates" instead of routing. In most cases (including here), activate here means click. We don't support retrieval of location information for Scintilla, so it just uses the centre of the window.

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

@dkager
Copy link
Collaborator

dkager commented Jan 28, 2016

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 SCI_POINTXFROMPOSITION and SCI_POINTYFROMPOSITION to click in the appropriate place.

@jcsteh
Copy link
Contributor

jcsteh commented Jan 28, 2016 via email

@jcsteh
Copy link
Contributor

jcsteh commented Feb 24, 2016

Actually, ScintillaTextInfo already implements _getPointFromOffset (and _getOffsetFromPoint). I can make a few educated guesses as to why it isn't working as expected:

  1. The points are probably client coordinates (not screen coordinates), but it doesn't look like we convert them. We'd need ot check the API.
  2. As per my last comment, if the app isn't DPI aware, we'd have to take that into account too. See windowUtils.logicalToPhysicalPoint.

@dkager
Copy link
Collaborator

dkager commented Mar 6, 2016

For reference, the Scintilla doc says:

SCI_POINTXFROMPOSITION(<unused>, int pos)
SCI_POINTYFROMPOSITION(<unused>, int pos)
      These messages return the x and y display pixel location of text at position pos in the document.

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.

@dkager
Copy link
Collaborator

dkager commented Mar 6, 2016

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 _getPointFromOffset is that you can't 'activate' at y=0, because the code checks if point.x and point.y which returns False if y=0. This should be fixed if we use physical coordinates, though maybe using not None is better?

@jcsteh
Copy link
Contributor

jcsteh commented Mar 6, 2016 via email

@dkager
Copy link
Collaborator

dkager commented Mar 7, 2016

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:notifications@github.com]
Sent: Sunday, March 6, 2016 23:16
To: nvaccess/nvda nvda@noreply.github.com
Cc: Davy Kager mail@davykager.nl
Subject: Re: [nvda] routing broken in notepad++ (#4703)

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.


Reply to this email directly or view it on GitHub #4703 (comment) . https://github.com/notifications/beacon/AC9y8ctwFutl6xpcQ565pXvinEAWSYjAks5pq1IMgaJpZM4HOcL8.gif

@jcsteh
Copy link
Contributor

jcsteh commented Mar 7, 2016 via email

@dkager
Copy link
Collaborator

dkager commented Mar 7, 2016

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:notifications@github.com]
Sent: Monday, March 7, 2016 07:38
To: nvaccess/nvda nvda@noreply.github.com
Cc: Davy Kager mail@davykager.nl
Subject: Re: [nvda] routing broken in notepad++ (#4703)

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.


Reply to this email directly or view it on GitHub #4703 (comment) . https://github.com/notifications/beacon/AC9y8ab5BHGqfffnsodklMcPxqCI6k3pks5pq8fhgaJpZM4HOcL8.gif

@dkager
Copy link
Collaborator

dkager commented Mar 7, 2016

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.

dkager added a commit to dkager/nvda that referenced this issue Mar 7, 2016
@jcsteh
Copy link
Contributor

jcsteh commented Mar 7, 2016 via email

@dkager
Copy link
Collaborator

dkager commented Mar 8, 2016

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:notifications@github.com]
Sent: Monday, March 7, 2016 23:27
To: nvaccess/nvda nvda@noreply.github.com
Cc: Davy Kager mail@davykager.nl
Subject: Re: [nvda] routing broken in notepad++ (#4703)

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.


Reply to this email directly or view it on GitHub #4703 (comment) . https://github.com/notifications/beacon/AC9y8UJhkv6zGWGxG4sng0u3kMtyFDiQks5prKYpgaJpZM4HOcL8.gif

@jcsteh
Copy link
Contributor

jcsteh commented Mar 11, 2016 via email

@dkager
Copy link
Collaborator

dkager commented Mar 11, 2016

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:notifications@github.com]
Sent: Friday, March 11, 2016 07:04
To: nvaccess/nvda nvda@noreply.github.com
Cc: Davy Kager mail@davykager.nl
Subject: Re: [nvda] routing broken in notepad++ (#4703)

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.


Reply to this email directly or view it on GitHub #4703 (comment) . https://github.com/notifications/beacon/AC9y8W5jo0spuizvBuuT1LlN8SwbWxMHks5psQXcgaJpZM4HOcL8.gif

@jcsteh
Copy link
Contributor

jcsteh commented Mar 11, 2016 via email

@dkager
Copy link
Collaborator

dkager commented Mar 13, 2016

It looks like Scintilla is DPI-aware. That is, _getLineOffsets works regardless of DPI. But when performing the activate action in Notepad++ it often scrolls to the wrong line, so I think Notepad++ isn't fully DPI-aware.

My intention is to stick with client coordinates in _getLineOffsets because this avoids conversion to/from screen coordinates (see my commit referencing this issue). Hopefully this makes sense.

@dkager
Copy link
Collaborator

dkager commented Mar 13, 2016

By the way, any thoughts on wrapping ClientToScreen into windowUtils?

@dkager
Copy link
Collaborator

dkager commented Mar 13, 2016

Actually, TextInfo.activate uses SetPhysicalCursorPos on Windows Vista and up. Therefore ScintillaTextInfo._getPointFromOffset should not compensate for DPI in this particular case. But what about ther use cases? And what should _getOffsetFromPoint do? I'm tempted to say returning physical coordinates is enough and handling DPI should be done globally, but evidently I am slightly confused by now. :)

Maybe we can deal with the DPI stuff in a separate issue if/when it comes up in real use?

@jcsteh
Copy link
Contributor

jcsteh commented Mar 13, 2016 via email

@dkager
Copy link
Collaborator

dkager commented Mar 14, 2016

So here is what I am getting so far:

  • I route braille to a specific location in Notepad++.
  • I press the routing button again on that location to cause a mouse click.
  • This leads to TextInfo.activate being called.
  • In turn this calls winUser.getCursorPos, on Windows 10 this resolves to GetPhysicalCursorPos.
  • In Notepad++ the mouse is normally centered, so the returned coordinates on a 1920×1080 display are (960, 540).
  • Then I scale my display down from 150% to 100%. Now the returned mouse coordinates are (1440, 810). Again this is the center of the display.
  • Finally, without calling windowUtils.logicalToPhysicalPoint, Scintilla already returns coordinates that change depending on DPI, provided you convert them to screen coordinates with ClientToScreen. They are passed to winUser.setCursorPos.

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 Get/SetPhysicalCursorPos, or leave things as they are and don't worry about DPI?

@jcsteh
Copy link
Contributor

jcsteh commented Mar 15, 2016

  • This leads to TextInfo.activate being called.
  • In turn this calls winUser.getCursorPos, on Windows 10 this resolves to GetPhysicalCursorPos.

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

  • Finally, without calling windowUtils.logicalToPhysicalPoint, Scintilla already returns coordinates that change depending on DPI, provided you convert them to screen coordinates with ClientToScreen. They are passed to winUser.setCursorPos.

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

@dkager
Copy link
Collaborator

dkager commented Mar 15, 2016

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:notifications@github.com]
Sent: Tuesday, March 15, 2016 03:50
To: nvaccess/nvda nvda@noreply.github.com
Cc: Davy Kager mail@davykager.nl
Subject: Re: [nvda] routing broken in notepad++ (#4703)

  • This leads to TextInfo.activate being called.
  • In turn this calls winUser.getCursorPos, on Windows 10 this resolves to GetPhysicalCursorPos.

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

  • Finally, without calling windowUtils.logicalToPhysicalPoint, Scintilla already returns coordinates that change depending on DPI, provided you convert them to screen coordinates with ClientToScreen. They are passed to winUser.setCursorPos.

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


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
#4703 (comment) https://github.com/notifications/beacon/AC9y8b5duow9j0HjrTso3AK_XmNZ9tC5ks5pth5igaJpZM4HOcL8.gif

@jcsteh
Copy link
Contributor

jcsteh commented Mar 16, 2016

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.

@dkager
Copy link
Collaborator

dkager commented Mar 16, 2016

It looks like Get/SetPhysicalCursorPos behaves differently depending on the calling process, i.e. by the call to SetProcessDPIAware() that NVDA makes on startup. This is on Windows 10, but on previous versions of Windows that use Get/SetCursorPos this may be different still. I'm not sure who thought this would be a good idea, but I understand why people avoid dealing with this stuff now.
Similarly, the top/left coordinates of the Notepad++ window don't change when I scale my display, but the width/height do. Are the top/left coordinates NVDA stores in obj.location logical or physical?

Long story short, what does 'physical' even mean? It sounds like the answer depends on who you ask.

@jcsteh
Copy link
Contributor

jcsteh commented Mar 16, 2016

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.

@dkager
Copy link
Collaborator

dkager commented Mar 18, 2016

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.

I tried this in d38408b, but the resulting clicks are far less accurate than when simply using ClientToScreen. That is, sometimes you click and the caret jumps up a few lines. You can also not consistently double-click to select entire words. I added some log output to see what's going on. In particular, the top/left stored in obj.location don't change regardless of DPI whereas the width/height do.

@LeonarddeR
Copy link
Collaborator

I'm pretty sure this is fixed in #9427.

@LeonarddeR
Copy link
Collaborator

@dkager: Could you test this try build?

@LeonarddeR
Copy link
Collaborator

I can no longer reproduce this, thanks to #9427. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants