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

Create new locationHelper module to facilitate easier conversion between rectangle and point types #7423

Closed
LeonarddeR opened this issue Jul 20, 2017 · 2 comments · Fixed by #7537
Milestone

Comments

@LeonarddeR
Copy link
Collaborator

LeonarddeR commented Jul 20, 2017

Currently, there are many different ways in which a location is presented in NVDA:

  • (Left, top, width, height)
  • (left, top, right, bottom)
  • textInfos.Rect, which has left, top, right and bottom properties
  • ctypes.wintypes.Rect, which is a struct and also has left, top, right and bottom properties
  • (x,y)
  • textInfos.Point, which has x and y properties
  • ctypes.wintypes.Point, which is a structure and also has x and y properties

Although there aren't many use cases for this at first sight, i propose a new locationHelper module to get rid of these inconsistencies and conversion code that must be duplicated several times. This includes:

  1. New namedtuple based classes with some extra enhancements

    • Location: a named tuple with the four properties above, including right and bottom properties which are automatically calculated. This takes away the need to convert locations to rectangles and back in most cases. Also, we could facilitate checks like self.location in self.parent.location to see whether one object is part of another from a spatial perspective.
    • Rect: also a named tuple with corresponding properties, including width and height properties. I also propose a RECT property which creates a ctypes.wintypes.RECT on the fly. Also corner and center points come to mind. This should be backwards compatible with textInfos.Rect (with one exception, see below)
    • Point: named tuple with x and y properties, with a POINT property to create a ctypes.wintypes.POINT. Also methods to convert the point to a logical or physical point, point addition and subtraction, etc. Replacement for textInfos.Point
  2. Helper functions to convert between the new locationHelper classes and ctypes ones. I've bee nthinking about monkeypatching ctypes to do conversion on the fly, but this seems complex and might not be something we should even consider.

One concern about changing the Point and Rect objects from textInfos into named tuples is that named tuples aren't mutaple. There are some places in NVDA Core where this would lead to issues, however they could easily be solved. Regarding backwards compatibility with add-ons, we could decide to keep textInfos.Point and textInfos.Rect and deprecate them.

Some examples from current code

From TouchHandler.notifyInteraction

Current code

		l, t, w, h = obj.location
		oledll.oleacc.AccNotifyTouchInteraction(gui.mainFrame.Handle, obj.windowHandle,
			POINT(l + (w / 2), t + (h / 2)))

new code

		oledll.oleacc.AccNotifyTouchInteraction(gui.mainFrame.Handle, obj.windowHandle, obj.location.center.POINT)

displayModel.DisplayModelTextInfo._get__storyFieldsAndRects

Note that location for DisplayModelTextInfo is a rectangle, and location for object is based on width and height. Named tuples would make this clear when inspecting from a console. It is quite confusing, and I initially did this wrong when writing down below code.

Current code

		if self._location:
			left, top, right, bottom = self._location
		else:
			try:
				left, top, width, height = self.obj.location
			except TypeError:
				# No location; nothing we can do.
				return [],[],[]
			right = left + width
			bottom = top + height
		bindingHandle=self.obj.appModule.helperLocalBindingHandle
		if not bindingHandle:
			log.debugWarning("AppModule does not have a binding handle")
			return [],[],[]
		left,top=windowUtils.physicalToLogicalPoint(self.obj.windowHandle,left,top)
		right,bottom=windowUtils.physicalToLogicalPoint(self.obj.windowHandle,right,bottom)

New code

		try:
			location = self.location or self.obj.location.toRectangle()
		except AttributeError:
			# No location; nothing we can do.
			return
		bindingHandle=self.obj.appModule.helperLocalBindingHandle
		if not bindingHandle:
			log.debugWarning("AppModule does not have a binding handle")
			return [],[],[]
		left, top, right, bottom=location.toLogical(self.obj.windowHandle)
@jcsteh
Copy link
Contributor

jcsteh commented Jul 20, 2017 via email

@LeonarddeR
Copy link
Collaborator Author

At least for the location part, I think a deprecation path could solve most of this. Note that the location[0] notation will still work, location.left won't. An ugly, but effective way to maintain backwards compatibility during the deprecation period would be a trick like location=Location(*location) before you request properties like left or top. More easy would be a two stage approach, implement the locationHelper part in the first stage, and a year after, assume that everyone has been able to update their code to support locationHelper.

For rectangles and points, I have less concern, as they are already property based, even though they aren't tuples. So, I belief the only problems there would be:

  1. mutability vs immutability, which can be solved by leaving the textInfos ones around during the deprecation stage
  2. methods like Point.toLogical, however, it is easy to implement those on textInfos.Point and textInfos.Rect as well during the deprecation stage.
@property
def _locationHelperPoint(self):
	"""Returns the locationHelper.Point for this textInfos.Point."""

def toLogical(self, hwnd):
	return self._locationHelperPoint.toLogical(hwnd)

@nvaccessAuto nvaccessAuto added this to the 2018.3 milestone Jul 30, 2018
feerrenrut pushed a commit that referenced this issue Jul 30, 2018
…gles and points (PR #7537)

Closes #7423

Previously there were many different ways in which a location is presented in NVDA:
- (left, top, width, height)
- (left, top, right, bottom)
- textInfos.Rect, which has left, top, right and bottom properties
- ctypes.wintypes.Rect, which is a struct and also has left, top, right and bottom properties
- (x,y)
- textInfos.Point, which has x and y properties
- ctypes.wintypes.Point, which is a structure and also has x and y properties

On the fly conversion between the different types is not possible. Also, conversion from screen coordinates to client coordinates, physical to logical coordinates, etc. is somewhat cumbersome.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants