[Development] QRect::contains and undocumented(?) edge

[Tomi Pannila]

Hi Lars and Eirik,


thank you for the explanations. I guess Eirik kind of guessed right that 
I view QRect from $\mathbb{R}^2$ point of view.
I'm a mathematician and this is the most natural point of view for me. 
For me, rectangles have edges=boundary which are measure zero.



To Lars

 >> With separate class one could have custom edge sizes as a bonus.
 > That would be a rect with a border. But where do you stop then? One 
border width for all sizes of configurable for each side independently? 
In addition, you’d need more data to store that information making the 
class bigger.
 > So far there hasn’t been a need to have this as a general purpose 
class in Qt.
You are right, it would be a border instead an edge. Isn't the current 
implementation about edge also in some sense a border check of the 
"weird interior"?
I don't know where one is expected to stop if one creates a class for 
rectangles with edges.
It was a question related on separating rectangles with edges from 
ordinary rectangles.
An idea to clarify the meaning and usage of rectangles.
If one is serious on separating them, then the question "how far one 
would go" depends on "how edges are expected to be used" in Qt and 
outside Qt?


 > Historical reasons means this has been the behavior for the last 25 
years, and a huge amount of code relies on this behavior. Changing it 
would lead to subtle breaks in lots of places for our users, breakage 
that is not easily
 > found at compile time. So we don’t want to change it to not break 
lots of user code out there relying on it.
I understand. I was hoping that nobody outside Qt would use the edges 
and hence the change would be plausible.
I guess one could compile Qt with a warning logged if 
"rect.contains(point, true)" is called.
This might give an idea how much it is actually used.



Best Regards,
Tomi Pannila

On 20.4.2022 10.27, Lars Knoll wrote:
> Hi Tomi,
>
>> On 19 Apr 2022, at 21:49, Tomi Pannila <tpannila at gmail.com> wrote:
>>
>> Hi Qt developers,
>>
>> could you please explain how your method "QRect::contains(const QRect &r, bool proper)" at
>> https://code.qt.io/cgit/qt/qtbase.git/tree/src/corelib/tools/qrect.cpp?h=dev
>> satisfies your API design principles
>> 1. Have clear and simple semantics
>> 2. Be intuitive
>> 3. Be easy to memorize
>> 4. Lead to readable code
>> from https://wiki.qt.io/API_Design_Principles ?
> Short answer: It doesn’t. It’s a very old API from the early days of Qt, and wouldn’t be done this way today anymore. At the very least, the boolean argument should be replaced by an enum indicating what the mode does. rect.contains(otherRect, true) is not understandable without reading documentation. rect.contains(otherRect, QRect::IncludeEdges) would be already much better.
>
>> More precisely, I'm talking about the edge related stuff, which is not documented properly in Qt documentation. The meaning of an edge is not defined(?).
> As we’re dealing with integers, it’s the difference between less and less-equal, but I agree it’s not documented all to well.
>
>> What files make use of the edge property?
>> Wouldn't it be better to create a separate class for edged rectangles, if rectangles with edges are needed?
> No. You don’t want to add a complete new class for one test functions like contains(). QRect is being passed into and returned from quite a lot of methods in Qt, and you wouldn’t want to have to overload all of those with both a QRect and a QInclusiveRect. In addition, you’d duplicate lots of APIs and both classes would behave the same for most methods.
>
>> With separate class one could have custom edge sizes as a bonus.
> That would be a rect with a border. But where do you stop then? One border width for all sizes of configurable for each side independently? In addition, you’d need more data to store that information making the class bigger.
>
> So far there hasn’t been a need to have this as a general purpose class in Qt.
>
>> It would also be good if in your documentation you would define mathematically what is meant by a rectangle.
>> Something like
>> "
>> For $x1 < 2$ and $y1 < y2$,
>> $$QRect(x1, x2, y1, y2) := \set{ (x, y) \in int^2  \quad |  \quad x1 \le x \le x2 \quad \text{and} \quad y1 \le y \le y2 }$$
>> "
>> could work together with mathjax, https://www.mathjax.org/ .
>>
>> I can guess that x1 x2, with x2 < x1, is "allowed" in QRect due to easier manipulation of QRects in some cases (all ints) (references to such cases?),
>> but it flips the left accessor to actually right(?), hence making it illogical. Similarly for y1 and y2. I see the isValid method.
>> I mean, one could have "int left, top;" and "unsigned int width, height;" instead of "int x1, x2, y1, y2" to describe QRect.
>>
>>
>>
>> Earlier I was browsing qcosmeticstroke.cpp where clip check with a point is multiple times checked(?) with
>> "
>> const QRect &cl = stroker->clip;
>>      if (x < cl.x() || x > cl.right() || y < cl.y() || y > cl.bottom())
>>          return;
>> "
>> https://code.qt.io/cgit/qt/qtbase.git/tree/src/gui/painting/qcosmeticstroker.cpp
> Yeah, that could probably use rect.contains(point).
>> I was expecting to see this kind of implementation, with negation, in QRect::contains. I was also thinking if performance in rendering
>> is so important that a call to QRect::contains cannot be made, and that for some reason QRect::contains cannot be made inline.
>> Perhaps there is a way to include an inline implementation of QRect::contains to QRect.h which could be used here?
> I doubt that some contains() calls would be preformance critical in that code path.
>> At
>> https://doc-snapshots.qt.io/qt6-dev/qrect.html
>> something is mentioned about "historical reasons". No further arguments or links given. Perhaps this is the source of the confusion I have?
>> Are these historical reasons permanent or do you plan to move away from these historical reasons? What would go wrong if you abandon the historical reasons?
> Historical reasons means this has been the behaviour for the last 25 years, and a huge amount of code relies on this behaviour. Changing it would lead to subtle breaks in lots of places for our users, breakage that is not easily found at compile time. So we don’t want to change it to not break lots of user code out there relying on it.
>
>> I'm aware that QRect has been illogical for a long time
>> https://www.qtcentre.org/threads/6561-what-are-the-historical-reasons-mentioned-in-the-QRect-class-reference
>>
>>
>>
>> I have no commits in Qt. This is my first post to development mailing list.
> Welcome then! :)
>
> Cheers,
> Lars
>
>>
>>
>> Best Regards,
>> Tomi Pannila
>>
>>
>> _______________________________________________
>> Development mailing list
>> Development at qt-project.org
>> https://lists.qt-project.org/listinfo/development