Re: Tools module geometry classes

[Luboš Luňák <l.lunak@xxxxxxxxxxxxx>]

On Wednesday 29 of June 2022, Chris Sherlock wrote:
> On Wed, Jun 29, 2022 at 9:06 AM Chris Sherlock <chris.sherlock79@xxxxxxxxx>
> wrote:
> > On Mon, Jun 27, 2022 at 4:52 PM Miklos Vajna <vmiklos@xxxxxxxxxxxxx>
> > wrote:
> >> On Mon, Jun 13, 2022 at 01:03:30PM +1000, Chris Sherlock <
> >> chris.sherlock79@xxxxxxxxx> wrote:
> >> > Are there any plans for deprecating the usage of the tools geometry
> >> primitives? I understand it will be needed for deserialising some legacy
> >> svm files, but is the intention to ever start replacing the tools
> >> primitives with the basegfx primitives?

 I guess that depends on what exactly you mean by "intention". If you look at 
tools/README.md, it says it's deprecated and "will be removed in the near 
future". Except that comment is from 2013. So presumably the intention has 
always been there, except it's never actually taken place. The common OOo 
pattern of starting to write something new and shiny in order to replace 
something old and then never finishing it.

> >> The trouble with e.g. tools::Rectangle is that it can have both a closed
> >> or a half-open interval, and you need to read the surrounding code to
> >> understand which mode is in use. basegfx::B2IRange is explicitly closed.
> >>
> >> So it would help readability to go with basegfx::B2IRange everywhere,
> >> but it's not an easy hack to do such conversions.

 While tools::Rectangle is indeed completely brain-damaged with that 
open/closed interval problem, I'm not that convinced that basegfx primitives 
are that much better, as they have their own list of dumb things. The size 
class is actually a typedef of the vector class (because everybody totally 
thinks of sizes as vectors, right), the design is ... interesting, so 
e.g. 'point = size;' is perfectly valid code, and the classes have like the 5 
most basic functions implemented and that's it. So even if it weren't for the 
trouble with converting the open/closed problem, the conversion still 
wouldn't be simple and it's a question if it would be an actual improvement 
in the end.

> > Thanks Miklos. I read the comment above the Rectangle header definition
> > which starts with "Note: this class is a true marvel of engineering:
> > because the author could not decide whether it's better to have a closed
> > or half-open interval, they just implemented *both* in the same class!"
> >
> > What is the feasibility of changing this class to be only explicitly
> > closed?

 Next to impossible, I'm afraid. I had a look at it in the past 
(https://lists.freedesktop.org/archives/libreoffice/2020-March/084702.html), 
and IIRC the summary is that the open/closed interval problem is not a 
technical problem. The class uses open or closed interval depending on the 
intention of the developer. Some functions assume one or another, but some 
work for both and you basically can't tell which one it is unless you know. I 
expect there is even code that mixes that for one rectangle instance. So it 
is my understanding that you cannot fix that with code, it can be fixed only 
by declaring how it's supposed to be used.

 BTW, it's also worth noting that the linked to discussion basically points 
out that people apparently even can't agree on what a rectangle class 
actually should look like. I rather quickly gave up on it and decided I could 
spend my time better.

> > Is this the main issue with not converting over to basegfx?

 I would say it's a big part, since you'd need to decipher whether each usage 
of tools::Rectangle assumes open or closed intervals. Or you'd need to guess 
and then fix the fallout and hope there wouldn't be much broken that'd go 
undetected. Which would be presumably a tremendous amount of work, and also 
boring, obnoxious, error-prone and whatnot.

 But as I wrote above, I see more problems with the conversion than just this.

> A quick followup - I have changed the getWidth() and getHeight() functions
> in tools::Rectangle to getHalfOpenWidth/getHalfOpenHeight() - it seems
> needlessly confusing to have functions with the same name except
> capitalizing the first letter (!!) and have differing functionality.
>
> The gerrit commit is here:
>
> https://gerrit.libreoffice.org/c/core/+/136575
>
> Is this reasonable?

 I think getHalfOpenWidth() is too long, the 'half' part can be dropped.

 But while this is an improvement for the class, it doesn't really change much 
about it still being broken as such. The class itself will still be confusing 
to use, other function will still be unclear on what they assume, etc.

 So I think a good question to ask is what it is you want to achieve and how 
much time and energy are you willing to spend on it. I don't want to 
demoralize or stop you, it certainly would be really nice if this madness 
finally got sorted out, it's just that it hasn't got sorted out yet because 
nobody so far has been willing to give it all the effort it'd need.

-- 
 Luboš Luňák
 l.lunak@xxxxxxxxxxxxx