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

Fix window point conversion functions not exposing failure #9269

Merged
merged 24 commits into from Jan 13, 2020

Conversation

@leonardder
Copy link
Collaborator

leonardder commented Feb 13, 2019

Link to issue number:

None

Summary of the issue:

There are 4 functions wrappers related to screen, client, logical and physical coordinate conversions.

  • windowUtils.logicalToPhysicalPoint
  • windowUtils.physicalToLogicalPoint
  • winUser.ClientToScreen
  • winUser.ScreenToClient

These functions take a window handle and a point structure and return 1 on success and 0 on failure. The user32 functions also set the last error when the functions fail. The point in the structure is used to calculate a new point, and on success, the coordinates in the structure are replaced. On failure, the coordinates stay the same.

However, the wrappers of these functions silently return whatever is in the structure, thereby silently ignoring whether the underlying function succeeded or failed.
As far as I've seen, for the user32 functions this is only a problem when giving it a wrong window handle. Converting coordinates falling out of the client area of the window works just fine. However, for physicalToLogicalPoint and logicalToPhysicalPoint, the functions also fail when the given coordinates fall outside the window. While normally, providing the wrong coordinates to these functions should not happen, not being able to see whether the underlying function succeeded or failed is problematic for debugging reasons.

Description of how this pull request fixes the issue:

  1. The winUser wrappers will now raise a WindowsError on failure. Note that this does not occur when providing it coordinates that fall outside the client window.
  2. the windowUtils functions raise a RuntimeError. Unfortunately, when these functions fail, they do not set the last error, so there's no way to see why the function failed.
  3. The corresponding calls in locationHelper do not catch errors, so calling screenToClient on a point or rectangle will raise a WindowsError on failure, etc.
  4. Places that call the mentioned functions, particularly displayModel, will now properly catch errors and log them.
  5. It removes Windows XP code from windowUtils

Testing performed:

This is pretty hard to test. I stumbled upon this bug when testing the highlighter in the vision framework (#9064) as that converts physical to logical rectangle coordinates. It silently seemed to fail the conversion of the top left coordinates where it succeeded in converting the bottom right coordinates, resulting in a malformed rectangle.

Known issues with pull request:

None

Change log entry:

  • Changes for developers
    • The following functions now raise an exception on failure instead of a silent return: (#9269)
      • RuntimeError: windowUtils.logicalToPhysicalPoint and windowUtils.physicalToLogicalPoint
      • WindowsError: winUser.ClientToScreen and winUser.ScreenToClient
      • Note that the wrapper methods in the locationHelper module will now also raise these exceptions on failure.
Copy link
Contributor

michaelDCurran left a comment

In some places you are catching exceptions and logging a debugWarning, but then it seems you're still going on and using the values you could not get. You'd either need to return some kind of safe value from the function or raise the original exception again.

try:
right,bottom=windowUtils.physicalToLogicalPoint(self.obj.windowHandle,right,bottom)
except RuntimeError:
log.debugWarning("physicalToLogicalPoint failed for bottom right", exc_info=True)

This comment has been minimized.

Copy link
@michaelDCurran

michaelDCurran Mar 28, 2019

Contributor

Should not this return from the function? if there is a runtimeError, then some of left,,top,right or button are going to not exist or be wrong...

This comment has been minimized.

Copy link
@leonardder

leonardder Mar 28, 2019

Author Collaborator

I'm not sure about this one. Returning from the function results in no text being returned at all. I guess your opinion is that it might be better to return no text at all rather than the wrong text. That makes sense, but then we really need to log something on the error level.

source/NVDAObjects/window/__init__.py Outdated Show resolved Hide resolved
source/NVDAObjects/window/__init__.py Outdated Show resolved Hide resolved
@leonardder

This comment has been minimized.

Copy link
Collaborator Author

leonardder commented Jul 30, 2019

@feerrenrut: This pull requests changes the behavior of some winUser and windowUtils helper functions. Therefore looking at this to be merged into 2019.3 might be a good idea, since it introduces backwards incompatible changes (i.e. these functions now raise exceptions where they didn't before).

@leonardder

This comment has been minimized.

Copy link
Collaborator Author

leonardder commented Sep 30, 2019

@michaelDCurran: I'd like to bring this under your attention again, as this introduces backwards incompatible changes for add-ons using these functions.

@AppVeyorBot

This comment has been minimized.

Copy link

AppVeyorBot commented Sep 30, 2019

PR introduces Flake8 errors 😲

See test results for Failed build of commit eb06de5eb2

Leonard de Ruijter
winUser.RECT(left, top, left + width, top + height), None,
winUser.RDW_INVALIDATE | winUser.RDW_UPDATENOW)
# Conversion to client coordinates may fail if the window handle of this object is incorrect.
# This will most likely be caused by a died window.

This comment has been minimized.

Copy link
@michaelDCurran

michaelDCurran Oct 4, 2019

Contributor

died -> dead

wcharToInt(next(cpBufIt)),
wcharToInt(next(cpBufIt))
)
if right < left:

This comment has been minimized.

Copy link
@michaelDCurran

michaelDCurran Oct 4, 2019

Contributor

Do you know under what circumstances this becomes reversed? can you put a comment here explaining this at least?

This comment has been minimized.

Copy link
@leonardder

leonardder Oct 9, 2019

Author Collaborator

I'm not 100% sure, but my theory is that this happens in case of left to right languages.

This comment has been minimized.

Copy link
@leonardder

leonardder Oct 9, 2019

Author Collaborator

Thinking more about this, I think it would be better to split this specific fix out into a new pr.

@feerrenrut feerrenrut added this to Review in progress in Babbage Work Oct 14, 2019
@feerrenrut feerrenrut added this to the 2019.3 milestone Oct 14, 2019
@michaelDCurran

This comment has been minimized.

Copy link
Contributor

michaelDCurran commented Oct 24, 2019

this pr has conflicts now.

@leonardder

This comment has been minimized.

Copy link
Collaborator Author

leonardder commented Oct 25, 2019

Fixed now.

@dingpengyu

This comment has been minimized.

Copy link
Contributor

dingpengyu commented Nov 19, 2019

hi
Is the current pr ready to merge?
thanks

@michaelDCurran

This comment has been minimized.

Copy link
Contributor

michaelDCurran commented Nov 19, 2019

Sorry, conflicts again.

@leonardder

This comment has been minimized.

Copy link
Collaborator Author

leonardder commented Nov 21, 2019

Conflict is now fixed.

@leonardder leonardder requested a review from michaelDCurran Nov 21, 2019
@dingpengyu

This comment has been minimized.

Copy link
Contributor

dingpengyu commented Nov 29, 2019

hi
Currently this is the last open pr for milestone 2019.3

Babbage Work automation moved this from Review in progress to Reviewer approved Jan 13, 2020
@michaelDCurran michaelDCurran modified the milestones: 2019.3, 2020.1 Jan 13, 2020
@michaelDCurran

This comment has been minimized.

Copy link
Contributor

michaelDCurran commented Jan 13, 2020

Moved this to the 2020.1 milestone. Needs a lot of testing.

@michaelDCurran michaelDCurran merged commit 669ae5a into nvaccess:master Jan 13, 2020
1 check passed
1 check passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Babbage Work automation moved this from Reviewer approved to Done Jan 13, 2020
@leonardder

This comment has been minimized.

Copy link
Collaborator Author

leonardder commented Jan 14, 2020

Note that this pr now causes errors like this:

Traceback (most recent call last):
  File "locationHelper.pyc", line 258, in toLogical
  File "locationHelper.pyc", line 170, in toLogical
  File "windowUtils.pyc", line 93, in physicalToLogicalPoint
RuntimeError: Couldn't convert point(x=8, y=1071) from physical to logical coordinates for window 1179716

This is no mistake in this pr, rather, it is exactly proving the point that this pr was necessary. Effort should be put into finding out why these errors occur in order to fix them properly.

@leonardder leonardder deleted the BabbageCom:fixConversionFunctions branch Jan 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Babbage Work
  
Done
5 participants
You can’t perform that action at this time.