Add AdjustX method to basegfx::B2DPoint

classic Classic list List threaded Threaded
5 messages Options
Alexander Farrow Alexander Farrow
Reply | Threaded
Open this post in threaded view
|

Add AdjustX method to basegfx::B2DPoint

Hi everybody,

Recently for GSoC I have been looking into resolving

This involves changing the types of variables from tools::Point to basegfx::B2DPoint.
tools::Point has a method called AdjustX(long nHorzMove), which adds nHorzMove to X:

basegfx::B2DPoint does not have such a method. So calls to AdjustX(nHorzMove) are
being replaced by something like setX(getX() + nHorzMove).

It works fine of course but I think AdjustX looks cleaner and it feels like a step
backwards when moving from tools::Point to basegfx::B2DPoint.

An OpenGrok search for AdjustX reveals it is used frequently in the code-base,
which suggests it is a method which developers have found useful in the past.

If it is agreed that we should add such a method I would be happy to implement it.
Also for consistency we might want to add a similar method to other classes in
basegfx such as B2IPoint.

With many thanks,
Alexander Farrow (IRC: Alexander Farrow)

_______________________________________________
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: Add AdjustX method to basegfx::B2DPoint

Hi Alex,

Alexander Farrow wrote:
> basegfx::B2DPoint
> <https://docs.libreoffice.org/basegfx/html/classbasegfx_1_1B2DPoint.html>
> does not have such a method. So calls to AdjustX(nHorzMove) are
> being replaced by something like setX(getX() + nHorzMove).
>
tools::Point is not exactly a role model for a nicely designed class,
so I'd hesitate to take any literal bits out of that. ;)

Many places using AdjustX() also call AdjustY() just the next line, so
for that you could use basegfx::B2DPoint::operator+=(const B2DTuple&)

If you believe you *must* use a single-component modifier, please come
up with something that blends well with the b2dtuple/point
concept. Also ideally, you will maintain some amount of internal
consistency (e.g. compare how operator*, *= etc. all got added in
farious flavours, such that math with b2dtuples works like the
'canonical' way when done with skalars or various forms of vectors).

Cheers,

-- Thorsten

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

signature.asc (1K) Download Attachment
Noel Grandin-2 Noel Grandin-2
Reply | Threaded
Open this post in threaded view
|

Re: Add AdjustX method to basegfx::B2DPoint


Are we going to be doing this behind the
   VCL_FLOAT_DEVICE_PIXEL
define and using the DeviceCoordinate typedef ?

I have no skin in the game either way, just making sure we don't end up with overlapping functionality.

(as a side note I had no idea we had this enabled on mac and ios already)


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

Re: Add AdjustX method to basegfx::B2DPoint

In reply to this post by Thorsten Behrens-6
Hi Thorsten,

Thanks for your reply.

Thorsten Behrens wrote:
>tools::Point is not exactly a role model for a nicely designed class,
>so I'd hesitate to take any literal bits out of that. ;)

Good to know, I'm new to the codebase so I'll keep that in mind.


>Many places using AdjustX() also call AdjustY() just the next line, so
>for that you could use basegfx::B2DPoint::operator+=(const B2DTuple&)

Sure but there are occasions in the code where we are just modifying
a single coordinate. My recent patch is an example:
https://gerrit.libreoffice.org/#/c/70329/ <https://gerrit.libreoffice.org/#/c/70329/>


>If you believe you *must* use a single-component modifier

A single-component modifier isn't a must but it might be nice for
when we are only changing one coordinate.


> please come up with something that blends well with the b2dtuple/point
>concept. Also ideally, you will maintain some amount of internal
>consistency (e.g. compare how operator*, *= etc. all got added in
>farious flavours, such that math with b2dtuples works like the
>'canonical' way when done with skalars or various forms of vectors).

If we do decide to go ahead with this I'll be sure to follow your
implementation requirements.

With many thanks,
Alex
_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
Alexander Farrow Alexander Farrow
Reply | Threaded
Open this post in threaded view
|

Re: Add AdjustX method to basegfx::B2DPoint

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

Noel Grandin wrote:
>Are we going to be doing this behind the
>   VCL_FLOAT_DEVICE_PIXEL
>define and using the DeviceCoordinate typedef ?

I'm assuming your referring to my recent patch:
https://gerrit.libreoffice.org/#/c/70329/ <https://gerrit.libreoffice.org/#/c/70329/>

That's a good point, do you mean doing something like:
#if VCL_FLOAT_DEVICE_PIXEL
    B2DPoint aCurrPos(0.0, 0.0);
#else
    B2IPoint aCurrPos(0, 0);
#endif


>(as a side note I had no idea we had this enabled on mac and ios already)

That's interesting, I didn't know that. I've OpenGrok searched the code for
where we are doing this but I couldn't find it. I found line 10247 in
configure.ac, which looks related:
https://opengrok.libreoffice.org/xref/core/configure.ac?r=c51b68dd#10247 <https://opengrok.libreoffice.org/xref/core/configure.ac?r=c51b68dd#10247>
But it's commented out (line starts with dnl).


With many thanks,
Alex
_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice