improving empty handling in tools::Rectangle

classic Classic list List threaded Threaded
10 messages Options
Noel Grandin-2 Noel Grandin-2
Reply | Threaded
Open this post in threaded view
|

improving empty handling in tools::Rectangle

Hi

For background: tools::Rectangle currently has magic values for it's bottom and right fields that indicate that
width/height is "empty".

Unfortunately, this leads to two problems
(1) the magic value is -32767, and sometimes that is a valid input value for a rectangle. Which means that rectangles
can suddenly become "empty".
(2) lots of code processes empty rectangles and just calls GetRight()/GetBottom(), which returns the magic -32767 value,
does some calculations on that value, and just continues happily.

All of which means that we pass around bogus data.

To improve this, I propose to do 3 things
(1) split the empty flags into their own bool values.
(2) make more of the tools::Rectangle methods do reasonable things when the empty flag is set.
(3) assert if code access Right or Bottom on an empty rectangle.

There are currently two gerrit changes related to this:

https://gerrit.libreoffice.org/#/c/71853/
Does a first piece, improving the conversion from tools::Rectangle to basegfx::B2DRectangle/B2IRectangle,
so that empty translates reasonably.

https://gerrit.libreoffice.org/#/c/71745/
Does the actual changes to tools::Rectangle.

I intend to land the first change (once Regina and Thorsten have weighed in), and then land the second change in stages,
peeling off pieces to make bugfixing easier if we have regressions.

Caolán, I anticipate that this work will result in some work from crash-testing which will probably flush out more
places that attempt to do computation on empty rectangles.

Regards, Noel.
_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
sberg sberg
Reply | Threaded
Open this post in threaded view
|

Re: improving empty handling in tools::Rectangle

On 07/05/2019 11:30, Noel Grandin wrote:
> (1) split the empty flags into their own bool values.

Is there a reason that information about emptyness needs to be encoded
more explicitly than just via getWidth()==0 and getHeigth()==0?
_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
Noel Grandin-2 Noel Grandin-2
Reply | Threaded
Open this post in threaded view
|

Re: improving empty handling in tools::Rectangle



On 2019/05/07 11:40 AM, Stephan Bergmann wrote:
> On 07/05/2019 11:30, Noel Grandin wrote:
>> (1) split the empty flags into their own bool values.
>
> Is there a reason that information about emptyness needs to be encoded more explicitly than just via getWidth()==0 and
> getHeigth()==0?

Yes, depending on which method you call (getWidth() vs GetWidth()), on tools::Rectangle, it can be regarded as a either
a half-open or a closed range.

Yes, this is truly truly awful, which is why I'm just trying to clean up one small part of it's awfulness.

_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
Luboš Luňák Luboš Luňák
Reply | Threaded
Open this post in threaded view
|

Re: improving empty handling in tools::Rectangle

On Tuesday 07 of May 2019, Noel Grandin wrote:

> On 2019/05/07 11:40 AM, Stephan Bergmann wrote:
> > On 07/05/2019 11:30, Noel Grandin wrote:
> >> (1) split the empty flags into their own bool values.
> >
> > Is there a reason that information about emptyness needs to be encoded
> > more explicitly than just via getWidth()==0 and getHeigth()==0?
>
> Yes, depending on which method you call (getWidth() vs GetWidth()), on
> tools::Rectangle, it can be regarded as a either a half-open or a closed
> range.
>
> Yes, this is truly truly awful, which is why I'm just trying to clean up
> one small part of it's awfulness.

 If you're going to already handle the problems, why not handle the awfulness
by fixing it, instead of patching it up? The class is broken, and it'll be
broken even after you tweak one consequence of the base problem. If the base
problem gets fixed, the consequence goes away too.

 The class can't be both, internally it stores just one value, and there are
functions such as the Size ctor or operator<< that do take a side (although
in line with the general stupidity, different functions take a different
side). And while the class gets used extensively, the usage can be fixed
mechanically.

--
 Luboš Luňák
 [hidden email]
_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
Noel Grandin-2 Noel Grandin-2
Reply | Threaded
Open this post in threaded view
|

Re: improving empty handling in tools::Rectangle

On 2019/05/07 1:06 PM, Luboš Luňák wrote:
>   If you're going to already handle the problems, why not handle the awfulness
> by fixing it, instead of patching it up? The class is broken, and it'll be
> broken even after you tweak one consequence of the base problem. If the base
> problem gets fixed, the consequence goes away too.
>

That would ideal, but this class is used __extensively__ by code that
already implements various workarounds and has lots of it's own issues.

What I'm doing here is making its behaviour in the empty case more reasonable,
and adding asserts that will flush out some of the existing dodgy code.

Note I say __some__, there are only a certain amount of regressions we are willing
to tolerate in the pursuit of better code :-)

>   The class can't be both, internally it stores just one value, and there are
> functions such as the Size ctor or operator<< that do take a side (although
> in line with the general stupidity, different functions take a different
> side). And while the class gets used extensively, the usage can be fixed
> mechanically.
>

I'd be happy to be proven wrong, but I'm not aware of any mechanical fixes.

And I would not be surprised to find code that calls both getWidth() and GetWidth()
on the same object.

_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
Luboš Luňák Luboš Luňák
Reply | Threaded
Open this post in threaded view
|

Re: improving empty handling in tools::Rectangle

On Tuesday 07 of May 2019, Noel Grandin wrote:
> On 2019/05/07 1:06 PM, Luboš Luňák wrote:
> >   The class can't be both, internally it stores just one value, and there
> > are functions such as the Size ctor or operator<< that do take a side
> > (although in line with the general stupidity, different functions take a
> > different side). And while the class gets used extensively, the usage can
> > be fixed mechanically.
>
> I'd be happy to be proven wrong, but I'm not aware of any mechanical fixes.

 A clang plugin that rewrites all "getWidth()" to "(GetWidth() + 1)" (or the
other way around).

> And I would not be surprised to find code that calls both getWidth() and
> GetWidth() on the same object.

 The way I see it, that doesn't matter. The class internally stores 4
coordinates, the problem is just the interpretation of width/height. But
that's purely a matter of definition, not of actual code. If the fix would be
(let's say) defining that the class uses closed range and dumping getWidth(),
then changing all getWidth() to (GetWidth() + 1) will keep the functionality
exactly the same. It may lead to some code looking silly, but how should that
break anything if the functionality doesn't change?

--
 Luboš Luňák
 [hidden email]
_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
Tomaž Vajngerl Tomaž Vajngerl
Reply | Threaded
Open this post in threaded view
|

Re: improving empty handling in tools::Rectangle

Hi,

On Tue, May 7, 2019 at 8:42 PM Luboš Luňák <[hidden email]> wrote:
  A clang plugin that rewrites all "getWidth()" to "(GetWidth() + 1)" (or the
other way around).


Well the problem is that getWidth() is not always GetWidth() + 1 , which you see when you examine its implementation. AFAIK this is the reason previous attempts abandoned the idea to mechanically change it. 

 Regards, Tomaž

_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
Luboš Luňák Luboš Luňák
Reply | Threaded
Open this post in threaded view
|

Re: improving empty handling in tools::Rectangle

On Tuesday 07 of May 2019, Tomaž Vajngerl wrote:

> Hi,
>
> On Tue, May 7, 2019 at 8:42 PM Luboš Luňák <[hidden email]> wrote:
> >   A clang plugin that rewrites all "getWidth()" to "(GetWidth() + 1)" (or
> > the
> > other way around).
>
> Well the problem is that getWidth() is not always GetWidth() + 1 , which
> you see when you examine its implementation. AFAIK this is the reason
> previous attempts abandoned the idea to mechanically change it.


 I see. One idea that comes to mind then is to split the current class into a
sane tools::Rectangle and keep the insane stuff in tools::SillyRectangle.
Then at least it would be clearly defined what is good and what is not, the
awfulness would be contained, wouldn't spread and its usage could be reduced
over time.

--
 Luboš Luňák
 [hidden email]
_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
Thorsten Behrens-6 Thorsten Behrens-6
Reply | Threaded
Open this post in threaded view
|

Re: improving empty handling in tools::Rectangle

In reply to this post by Noel Grandin-2
Hi Noel, *,

Noel Grandin wrote:
> What I'm doing here is making its behaviour in the empty case more reasonable,
> and adding asserts that will flush out some of the existing dodgy code.
>
Yep, on balance I think that's beneficial.

> I'd be happy to be proven wrong, but I'm not aware of any mechanical
> fixes.
>
Right, and I'm afraid those would obfuscate things even more.

The plan back in the day was to either use float ranges (where - for
graphics - this half vs. closed interval question usually becomes
moot), or B2IBox and B2IRange respectively. And then gradually move
code over to use basegfx & drawinglayer.

From how I see this, that change is best done manual.

Cheers,

-- Thorsten

_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice

signature.asc (1K) Download Attachment
Chris Sherlock Chris Sherlock
Reply | Threaded
Open this post in threaded view
|

Re: improving empty handling in tools::Rectangle



> On 9 May 2019, at 6:23 am, Thorsten Behrens <[hidden email]> wrote:
>
> Hi Noel, *,
>
> Noel Grandin wrote:
>> What I'm doing here is making its behaviour in the empty case more reasonable,
>> and adding asserts that will flush out some of the existing dodgy code.
>>
> Yep, on balance I think that's beneficial.
>
>> I'd be happy to be proven wrong, but I'm not aware of any mechanical
>> fixes.
>>
> Right, and I'm afraid those would obfuscate things even more.
>
> The plan back in the day was to either use float ranges (where - for
> graphics - this half vs. closed interval question usually becomes
> moot), or B2IBox and B2IRange respectively. And then gradually move
> code over to use basegfx & drawinglayer.
>
> From how I see this, that change is best done manual.

Can we file an easy hack to manually start changing to either half or closed intervals (whatever basegfx does)?

Chris

>
> Cheers,
>
> -- Thorsten
_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice