-
-
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
Provide a locationHelper module for code related to locations, rectangles and points #7537
Conversation
* Added the ClientToScreen function from user32.dll to the winUser module * Use locationHelper for object locations
source/locationHelper.py
Outdated
|
||
def __add__(self,other): | ||
if not isinstance(other,_POINT_CLASSES): | ||
return NotImplemented |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you not throw a NotImplementedError?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See this on Stack Overflow
source/locationHelper.py
Outdated
class Point(namedtuple("Point",("x","y"))): | ||
"""Represents a point on the screen.""" | ||
|
||
def __add__(self,other): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a quick line of PyDoc here to summarize what this function does.
source/locationHelper.py
Outdated
return NotImplemented | ||
return Point((self.x+other.x),(self.y+other.y)) | ||
|
||
def __radd__(self,other): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a quick line of PyDoc here to summarize what this function does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit reluctant to add PyDoc to every operator overloader, I'd say and would be enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So what does (r)add do in this case?
source/locationHelper.py
Outdated
def __radd__(self,other): | ||
return self.__add__(other) | ||
|
||
def __lt__(self, other): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now you suddenly put spaces after commas. :)
source/locationHelper.py
Outdated
def __lt__(self, other): | ||
""" | ||
Returns whether self is less than other. | ||
This first compares y, than x coordinates. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
than --> then
source/locationHelper.py
Outdated
return False | ||
return other.left == self.left and other.top == self.top and other.right == self.right and other.bottom == self.bottom | ||
|
||
def __neq__(self, other): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here too I would just call __eq__
.
source/locationHelper.py
Outdated
return False | ||
return not (other.left == self.left and other.top == self.top and other.right == self.right and other.bottom == self.bottom) | ||
|
||
def __sub__(self,other): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you not define __add__
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, since I have no solid idea about what add should do. Note that for sets, you can also only subtract them.
source/locationHelper.py
Outdated
return Rect(self.left,self.top,self.right,self.bottom) | ||
|
||
class Rect(_RectMixin, namedtuple("Rect",("left","top","right","bottom"))): | ||
"""Represents a rectangle on the screen. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a short sentence summarizing how this is different from a Location?
source/locationHelper.py
Outdated
""" | ||
Converts the given input to L{Rect}. | ||
Input should be one of the following types: | ||
* One of l{_RECT_CLASSES}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Judging by the underscore, this is a private variable, which should probably not be in your documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part of the docs should definitely be public, so I'll also make these variables public.
source/locationHelper.py
Outdated
Converts the given input to L{Rect}. | ||
Input should be one of the following types: | ||
* One of l{_RECT_CLASSES}. | ||
* One of L{_POINT_CLASSES}: converted to L{Rect} square of one pixel. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same, also for the functions below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a good direction to move in, there are a few concerns I have with this implementation though.
It's not very clear what the difference between Location and Rect is. To me Location sounds like a Point. I think Rect and Location could be named better to highlight their differences.
return Point((self.x+other.x),(self.y+other.y)) | ||
|
||
def __radd__(self,other): | ||
"""Returns a new L{Point} with x = self.x + other.x and y = self.y + other.y.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unit test:
myPoint = sum([point1, point2, point3])
I think this will give an error, you need to check for other==0
and then return self
See http://www.marinamele.com/2014/04/modifying-add-method-of-python-class.html
#See the file COPYING for more details. | ||
#Copyright (C) 2017 NV Access Limited, Babbage B.V. | ||
|
||
"""Unit tests for the locationHelper module. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding unit tests, I have some experience with this kind of code through games and simulation development and can say that it is fraught with edge cases. I would like to see the tests expanded. I'll make notes through the code for edge cases I worry about, would you mind adding unit tests for these?
source/locationHelper.py
Outdated
return Point((self.x-other.x),(self.y-other.y)) | ||
|
||
def __rsub__(self,other): | ||
return self.__sub__(other) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As with __radd__
please add a unit test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you elaborate? For add and radd, we have the sum function, but I belief there is no such function for subtraction, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I didn't think through how this might get called. Can you answer that, how would this get called? Why is it necessary? Is there some case (as with __rsum__
) that it might get called with other
as an integer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would get called if you do ctypes.wintypes.POINT(3,3) + locationHelper.Point(4,4), in which case it should return locationHelper.Point(7,7)
May be that's just a valid ctest case to work with? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I didn't follow that. My questions were about the __rsub__
function. I found the following on the python docs
These functions are called to implement the binary arithmetic operations (
+
,-
,*
,/
,%
,divmod()
,pow()
,**
,«
,»
,&
,^
,|
) with reversed operands. These functions are only called if the left operand does not support the corresponding operation. For instance, to evaluate the expressionx-y
, wherey
is an instance of a class that has an__rsub__()
method,y.__rsub__(x)
is called. Note that ternarypow()
will not try calling__rpow__()
(the coercion rules would become too complicated).
So adding a test to show that:
ctypes.wintypes.POINT(3,3) - locationHelper.Point(1,1)
is locationHelper.Point(2,2)
and also perhaps a test that shows that:
0 - locationHelper.Point(1,1)
will raise an unsupported exception.
However, this raises a question: what happens in the case of negation:
-locationHelper.Point(1,1)
Does this return `locationHelper.Point(-1,-1)?
A test to demonstrate this outcome would be good.
source/locationHelper.py
Outdated
def __lt__(self,other): | ||
""" | ||
Returns whether self is less than other. | ||
This first compares y, then x coordinates. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally find this unintuitive, I would suggest this be a named method rather than using the built in operator. There are several different ways that these comparison operators could work; x then y, y then x, or both must be less. A user of this class may not consider that implementation detail and just rely on the operator, this would lead to subtle and hard to fix bugs. I would prefer to see named methods that convey the differences, this forces the user to consider what it is that they really want.
For instance:
- xWiseLessThan(point1, point2)
- yWiseLessThan(point1, point2)
- xAndYLessThan(point1, point2)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This goes for other comparison methods too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
p = How about Point(1,2)
p.compare(Point.Y_FIRST)
or p.compare(Point.Y_THEN_X)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I'm stull a bit puzzled about what names to use. xWiseLessThan sounds ok, but what to use for the le operator? xWiseLessThanOrEqual is a bit long. How about?
- yWiseLE
- yWiseLT
- yWiseGE
- yWiseGT
Etc.
@derekriemer: how would less than or equal to fit into your compare idea? Similar to the cmp method, returning 1, 0 or -1? It would be the simplest solution, not very pythonic though IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could shorten to:
xWiseLessOrEq
xWiseLessThan
xWiseGreaterThan
xWiseGreaterOrEq
I think that is a nice balance between brevity and clarity.
I would prefer not to return 1, 0, -1 from a single compare function.
Another option (borrowing from Derek's idea) would be:
point1.LessThan(Point.xWise, point2)
Though this is longer, with no more clarity. Though probably less lines of code in the Point class required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer your first suggestion.
tests/unit/test_locationHelper.py
Outdated
def test_gt(self): | ||
self.assertGreater(Point(x=3,y=4), Point(x=4,y=3)) | ||
|
||
def test_lt(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For each comparison method please cover other edge cases:
- Point is not "comparison type" eg not less than
- Point is not "comparison type" but is on the boundary eg not less than (because it is equal)
source/locationHelper.py
Outdated
other=toPoint(other) | ||
except ValueError: | ||
return False | ||
return self.x==other.x and self.y==other.y |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this code limited to working with integers? If working with real numbers is possible / required then some thought will need to be put into this. One method I have seen many times is the Abs(x1 -x2) < EPSILON as a test for equality, a more thorough approach is discussed here: https://stackoverflow.com/a/4029392
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean this code to work with integers only and I'm quite sure that things would go horribly wrong if you'd use floats and try to convert the resulting rectangle to logical coords or a RECT structure. There is no type checking though.
"""Converts self to a L{ctypes.wintypes.POINT}""" | ||
return POINT(self.x,self.y) | ||
|
||
def toLogical(self, hwnd): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is currently nothing to stop you from doing nonsense conversations? For instance p1.toPhysical(hwnd).toPhysical(hwnd)
It could be worth remembering the "type" of the coordinates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I'd say this is the responsibility of the person who uses these utilities in his code. Having said that, I've thought about a method to remember the type of coordinates (logical/physical), but I'm afraid that the only way to do this in a reliable manner is adding an extra initialisation argument to the class. I think the extra complexity involved with this is far from elegant.
source/locationHelper.py
Outdated
return False | ||
return self>other | ||
|
||
def __lt__(self,other): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, for the same arguments with point, I don't think operator overriding is a clear way of providing this behaviour.
source/locationHelper.py
Outdated
|
||
class Rect(_RectMixin, namedtuple("Rect",("left","top","right","bottom"))): | ||
"""Represents a rectangle on the screen. | ||
By convention, the right and bottom edges of the rectangle are normally considered exclusive. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you clarify what this means with an example? Or better, point to a unit test that demonstrates this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a convention followed by NVDA, as this convension is also followed by Windows. I will document this with references. I'm afraid unit tests do not make sense here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would remove the words "normally considered", unless there is some case where they are not exclusive? A unit test could show that given Rect(0, 0, 1, 1)
then Point(0,0)
is inside, but Point(1,1)
, Point(0,1)
, Point(1,0)
aren't.
source/locationHelper.py
Outdated
|
||
@property | ||
def center(self): | ||
return Point((self.left+self.right)/2, (self.top+self.bottom)/2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a test for this where self.left = -5
, self.right=5
, self.top=5
, self,bottom=-5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should have been more specific here. I was worried about 0 divided by 2. You can ignore this.
Thinking about this more carefully, there are other concerns:
- A minor one, is performance. Often multiplication is faster than division, though we are unlikely to be doing this so many times an update that this would be at all noticeable.
- Rounding, given
RectLTRB(0,2,2,0).center()
givesPoint(1,1)
andRectLTRB(0,3,3,0).center()
also givesPoint(1, 1)
callers may be expecting floating point numbers to be returned. - A bug:
RectLTRB(10,13,13,10).center()
givesPoint(1,1)
when it should givePoint(11,11)
Change the implementation ofcenter
toPoint( self.left+self.width/2, self.top-self.height/2)
Please add a test for this, and also for the same in the negative quadrant
Also, sorry it's taken so long for me to get to reviewing this! |
Any suggestions? I'd say that this naming shouldn't be a huge problem as it is perfectly clear from the classes doc strings what they do exactly. Alternatively, we could rename location to whRect, where w stands for width and h for height, indicating that these rectangles are based on width and height coords. Still, I'd rather have only one of these (preferably only rect), but that would break backwards compatibility. |
Looking at |
@feerrenrut commented on 3 jan. 2018 08:40 CET:
Yes, backwards compatibility. Object locations are tuples equivalent to Location, the displayModel makes use of rectangle tuples equivalent to Rect. To give an example, if we'd keep the Rect class and give it a fromLTWH method that creates a new Rect instance from the given lwth, treating the Rect object as a tuple will return (l,t,r,b). |
Ok, I understand now, we do things like In that case, my slight preference would be to make this tuple ordering clear from its typename. It's a bit ugly, but what do you think of:
|
@feerrenrut commented on 4 jan. 2018 00:41 CET:
Yes.
I like those, except for the underscores. Do you mind having these removed? |
Removing the underscores is fine by me. I realise now, I made a mistake the B for the 'Location' replacement name should be a H. This would give:
|
…ding according to mathematical rules
tests/unit/test_locationHelper.py
Outdated
|
||
class TestToRectLTWH(unittest.TestCase): | ||
|
||
def test_collection(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a comment to explain what this is testing? It isn't obvious to me.
tests/unit/test_locationHelper.py
Outdated
|
||
class TestToRectLTRB(unittest.TestCase): | ||
|
||
def test_collection(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a comment to explain what this is testing?
source/locationHelper.py
Outdated
other=toRectLTRB(other) | ||
except ValueError: | ||
return False | ||
return other.left == self.left and other.top == self.top and other.right == self.right and other.bottom == self.bottom |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this has already been considered, but it has occured to me that if we are mixing rectangle like classes that come from a source that uses the top left as the origin with those that use the bottom left then given a screen space 10 heigh (indexes 0 to 9):
top of the former is index 3, the top of the latter is index 6 then top should be the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I belief we aren't supporting rectangles that are using the bottom left as their origin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having said that, wx.Rect seems to be incompatible, so I"m removing that one from the supported classes.
r=wx.Rect(10,10,11,11)
>>> r.top
10
>>> r.left
10
>>> r.width
11
>>> r.height
11
>>> r.right
20
>>> r.bottom
20
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really understand what is going on in that wx.Rect
example? My guess is that right
and bottom
are inclusive rather than our Rect
class which are exclusive?
As I mentioned in my other comment, I think we should add a way to mix coordinate spaces, and force those constructing Rect
classes to consider what coordinate space they are in. Even if all the types they are using are in the same space, if you don't consider it you may be visualising things wrong and not get the results that you expect (as I did, even though, with hindsight, I know I should have considered it)
source/locationHelper.py
Outdated
other=toRectLTRB(other) | ||
except ValueError: | ||
return False | ||
return other.left >= self.left and other.top >= self.top and other.right <= self.right and other.bottom <= self.bottom |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe there is a bug in here (both superset and subset ), caused by a mix up of top and bottom comparisons.
Consider:
r1=RectLTRB(left=10, top=14, right=14, bottom=10)
r2= RectLTRB(left=11, top=15, right=13, bottom=9)
r1.isSubset(r2)
returnsFalse
r1.isSuperset(r2)
returnsTrue
r2 in r1
returnsTrue
And:
r3 = RectLTRB(left=10, top=14, right=14, bottom=10)
r4 = RectLTRB(left=11, top=13, right=13, bottom=11)
then:
r3.isSubset(r4)
returnsFalse
r3.isSuperset(r4)
returnsFalse
r4 in r3
returnsFalse
Fix:
For the top and bottom comparisons, other
and self
should be swapped to the other side of the >= operator. Notice that the __contains__
code for Point has this correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the top and bottom comparisons, other and self should be swapped to the other side of the >= operator. Notice that the contains code for Point has this correct.
The first question I have is, are these rectangles, where the top is lower than the bottom, considered to be valid? If not, I'd go for something like this
Furthermore, could you elaborate on the swapping of self and other? I'm reading this as you mean it to be
return other.left >= self.left and self.top >= other.top and other.right <= self.right and self.bottom <= other.bottom
But that doesn't seem to be valid
How about:
return self.left<=other.left<=other.right<=self.right and self.top<=other.top<=other.bottom<=self.bottom
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are these rectangles, where the top is lower than the bottom, considered to be valid?
Whether the top is lower than the bottom is matter of perspective, where0,0
is the top left and y values decreasing vertically, then yes a top of 14 is below a bottom of 10. However0,0
in the bottom left and y values increasing vertically, then 14 is above 10.
I hadn't really considered the coordinate space this was supposed to be working in when considering this bug. If I swap the top and bottom values for those rectangles (r1 and r2) the results are as expected.
One way to make the subset and superset functions more robust would be to calculate based on each corner being contained by the rectangle:
return all(p in self for p in [other.topLeft, other.topRight, other.bottomLeft, other.bottomRight])
But I think it might be wise to include the coordinate space with the object. When a rect is created from some other object it can be set automatically, however when it is created by hand, or an unsupported type, then the developer needs to consider the address space.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I think it might be wise to include the coordinate space with the object.
You mean as a property, and if so, what spaces should we support?
I've been thinking about creating overrides for new in order to allow providing additional parameters, so we can also support setting a property that indicates whether provided coordinates are screen/client and physical/logical.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there are really just two likely coordinate systems that will be used:
- Top left origin, y increases down the screen.
- Bottom left origin, y increases up the screen
I was thinking something like:
# when converting from some other library type, the space does not need to be mentioned
r1 = toRectLTWH(someScreenSpaceType)
assert(r1.coordSpace == TOP_LEFT_ORIGIN)
# when creating manually, the space is mentioned and disambiguates "top" and "bottom", and forces the developer to consider it.
r2 = RectLTWH(left=10, top=13, width=5, height=3, coordSpace=BOTTOM_LEFT_ORIGIN)
assert(r2.bottom == 10)
assert(r2.right == 15)
# then same as r2 but other coord system
r3 = RectLTWH(left=10, top=13, width=5, height=3, coordSpace=BOTTOM_LEFT_ORIGIN)
assert(r3.bottom == 16)
assert(r3.right == 15)
# Even though the values for bottom do not match, they refer to the same space
assert(r2.bottom == r3.bottom) # this is actually a problem. This really depends on the size of the space, whether 10 from the bottom is equal to 16 from the top is really dependent on the size of the screen... maybe things like:
r2.subset(r3) # should just result in an exception?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm honestly not convinced that we need a system for rectangles based on bottom left origin. Do you have reasons to think that they are currently part of NVDA core or will be in future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont know that we do... but I also dont know that we dont. It's the ambiguity that worries me. I would be fine with saying that this library does not support them, but considering how easy it is to miss a docstring, and make all kinds of mistakes, I would like to see if we can find a way to make it very clear to users of this library how they should be thinking about the values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about raising a ValueError when creating a rectangle with right<left and bottom<top for RectLTRB? I'm afraid it is much harder for RectLTWH to enforce clarity, though.
Actually, I have seen an object location of 0, 0, -32000, -32000 in the wild, but these locations don't make sense to me. It looked a bit like giving a location for something we do not know the location of.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A ValueError would be enough for RectLTRB
. For the RectLTWH
I can't think of anyway to check as it is currently, only with the introduction of an extra construction parameter / or a name change.
@feerrenrut: I have fixed another small bug. I stumbled upon the following. When trying to get bottomRight, bottomLeft or topRight coordinates from a rectangle, some coordinates are exclusive. for bottomRight, both are. For topRight, the x coordinate is exclusive, and for bottomLeft, the y coordiante is exclusive. Should we keep this that way and document it, or should we change this in such a way that these properties return inclusive coordinates? I think the latter might also make sense. |
This does seem to be documented already, right? In the docstring for class |
Hmm, the problem I have with making everything inclusive is that Windows
itself as well as the accessibility APIs use exclusive coordinates for
the bottom right. That means that we would have to modify every
rectangle to make the bottom right point inclusive.
|
Yes, this is a bit annoying. My preference is always with a design that makes it harder to make mistakes. It might require a few changes now, but we are not regularly introducing this kind of code. If we are expecting the same data structure eg RECT to hold both inclusive and exclusive points, then it might be best to provide explicit helpers, or require an explicit argument to the |
@michaelDCurran, @jcsteh, @dkager, it would be great to have your thoughts about the last 4 comments. |
I would have to familiarize myself with this entire PR properly. But the convention on Windows has always been that rects have an exclusive bottom right. |
I probably wasn't clear enough, I am fine with exclusive bottom right coordinate, as long as this is consistent regardless of the source of the rectangle. @LeonarddeR you mentioned that you "stumbled upon the following." Can you elaborate here? Did this cause a bug, or take some time to debug and understand? |
@feerrenrut commented on 19 jun. 2018 02:13 CEST:
Yeah sure, sorry for being a bit vague there. For future support of vision enhancement providers, including magnifiers, I've been working on support to get a bounding rectangle for a piece of text using text infos. However, in order for that to work properly, I need to retrieve the first and last offsets of the text that are visible on screen. For that, I was about to use the bottomRight point of the location of the object, until I realised that the bottomRight point is exclusive and therefore belongs to another object. |
# LVM_GETSUBITEMRECT requires a pointer to a RECT structure that will receive the subitem bounding rectangle information. | ||
localRect=RECT( | ||
left=LVIR_LABEL, # Returns the bounding rectangle of the entire item, including the icon and label | ||
top=index # The one-based index of the subitem |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering this is expecting a one-based index, I think this function should assert that index > 0
windll.user32.ClientToScreen(self.windowHandle,byref(localRect)) | ||
windll.user32.ClientToScreen(self.windowHandle,byref(localRect,8)) | ||
return (localRect.left,localRect.top,localRect.right-localRect.left,localRect.bottom-localRect.top) | ||
# ##8268: this might be a malformed rectangle |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: Too many hash symbols here.
windll.user32.ClientToScreen(self.windowHandle,byref(localRect,8)) | ||
return (localRect.left,localRect.top,localRect.right-localRect.left,localRect.bottom-localRect.top) | ||
# ##8268: this might be a malformed rectangle | ||
# (i.e. with a left coordinate that is greather than the right coordinate). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm concerned that this might happen in other cases, can we be more specific about when this is the case? Should location helper generally handle this case? If not, I would like to see that it detects it and alerts us.
Could you add a unit test specifically for malformed rectangles?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Locationhelper detects this and raises a ValueError for RectLTRB. For RectLTWH, there is currently no detection for negative widths.
I'm afraid that we can't make sure that IAccessible or UIA will never return rectangles with negative width and/or height. May be we should no longer raise a ValueError, but log a warning instead.
POINT(x=400, y=400), | ||
RectLTRB(left=450, top=450, right=490, bottom=990), | ||
RECT(450, 450, 490, 990) | ||
), rect) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to think about this carefully about this, and I'm still not sure I understand this test. How many RectLTRB instances are returned from this call to toRectLTRB
? If more than one, which parameters are used? The same questions go for the toRectLTWH
version of this test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
toRectLTRB and toRectLTWH always return only one rectangle containing all points in the parameters. For example:
locationHelper.toRectLTRB(locationHelper.Point(3,3), locationHelper.Point(4,4))
RectLTRB(left=3, top=3, right=4, bottom=4)
I realise now that there are still some inconsistencies with regard to inclusivity and exclusivity. toRectLTRB(Point(3,3)) returns RectLTRB(3,3,4,4), and toRectLTRB(Point(3,3), Point(4,4)) returns the same. The reason for this is that toRectLTRB converts a single point to a rectangle with a width and height of 1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wondered if it might do something like that. Is this actually used anywhere yet? If not, I suggest we remove it. If it is, then I would prefer a dedicated function with a name that conveys this better eg toRectLTRBContainingAll
and has its own documentation.
I realise now that there are still some inconsistencies with regard to inclusivity and exclusivity.
But it sounds like this also needs to be fixed. Perhaps some more tests for this as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the way, there are probably some good names for functions like this in collision detection libraries used in games. You would be looking for something that "creates the minimum volume axis-aligned 2D bounding box".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make sure, are you suggesting the following way of splitting the functionality of toRectLTRB and TORectLTWH?
- Make toRectLTRB and toRectLTWH only do conversion from rect classes and tuples of four integers
- Create separate functions to create bounding rectangles from several types of input
@feerrenrut: I've done some major refactoring: I removed the toRectLTRB, toRectLTWH and toPoint helper functions, and split them out into some factory functions on the classes itself. That way, I hope I've made a lot clearer what the several methods ought to do. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work here!
@@ -306,6 +306,7 @@ def event_stateChange(self): | |||
class ListItem(RowWithFakeNavigation, RowWithoutCellObjects, ListItemWithoutColumnSupport): | |||
|
|||
def _getColumnLocationRaw(self,index): | |||
assert index>0, "INvalid index: %d" % index |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: Too many capitals, the N can be lower case.
source/locationHelper.py
Outdated
import textInfos | ||
import wx | ||
|
||
class Point(namedtuple("Point",("x","y"))): | ||
"""Represents a point on the screen.""" | ||
|
||
@classmethod | ||
def fromNonInts(cls, *nonInts): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this work with anything other than a float? I'm concerned that the types that can be used here are are not so obvious. If it is only going to work on floats (and by looks of it also ints) then a name like fromFloatCollection
might be better?
Edit:
Upon reading the tests (yay tests), I have realised this is also intended to work with strings. I think this might be a mistake. Firstly, is there anywhere that actually returns coordinates in a string? Second, this would make it all too easy for someone to forget to consider variations in the format of the string due to localisation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Somehow, It must have been late when I believed that it would make sense to create a rectangle from numbers that are saved in strings, but I guess that doesn't make very much sense. I applied your suggestion.
source/locationHelper.py
Outdated
if isinstance(rect,RECT_CLASSES): | ||
if cls is RectLTWH: | ||
return cls(rect.left, rect.top, rect.right-rect.left, rect.bottom-rect.top) | ||
return cls(rect.left, rect.top, rect.right, rect.bottom) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If another type of rect
class is introduced this could lead to subtle errors. Instead, do elif cls is RectLTRB:
which will allow falling back to raise TypeError
source/locationHelper.py
Outdated
if isinstance(point,POINT_CLASSES): | ||
if cls is RectLTWH: | ||
return cls(point.x, point.y, 0, 0) | ||
return cls(point.x, point.y, point.x, point.y) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also here, be specific to the classes supported.
source/locationHelper.py
Outdated
other=toRectLTRB(other) | ||
except ValueError: | ||
return False | ||
return False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be return NotImplemented
? or perhaps raise NotImplementedError
?
Link to issue number:
Closes #7423
Summary of the issue:
Currently, there are many different ways in which a location is presented in NVDA:
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.
Implementation details
This pr adds a new locationHelper module to get rid of these inconsistencies and conversion code that must be duplicated several times. This includes 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.
Rect: also a named tuple with corresponding properties, including width and height properties.
Both Location and Rect are based on a mix-in class which provides the following facilities
Point: named tuple with x and y properties. It provides the following facilities:
Helper functions (toRect, toLocation and toPoint) to convert between the new locationHelper classes, textInfos and ctypes ones.
The new module is currently only used in the following situations.
Known issues
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 using the several helper methods and operator support. Regarding backwards compatibility with add-ons, it is best to keep textInfos.Point and textInfos.Rect for now. I've added a deprecated statement to the doc string of these classes.
@josephsl: as a prominent person in the add-ons community, you might be able to tell what add-ons cause compatibility issues if we convert all textInfos.Rect and textInfos.Point cases into locationhelper ones?