[prev in list] [next in list] [prev in thread] [next in thread]
List: koffice-devel
Subject: Re: flake API, kopathshape and friends
From: Thorsten Zachmann <t.zachmann () zagge ! de>
Date: 2009-12-18 6:03:04
Message-ID: 200912180703.04539.t.zachmann () zagge ! de
[Download RAW message or body]
On Thursday 17 December 2009 10:46:40 Thomas Zander wrote:
> On Saturday 12. December 2009 19.09.15 Thomas Zander wrote:
> > I wrote stuff down in the wiki
> > http://wiki.koffice.org/index.php?title=Libs/Flake/PathShapes_API_review
> > and I'd love some help here.
>
> Thorsten; we got some really nice comments from Jan there already, but some
> open questions that I would like your input on.
>
> Can you comment either here or on the wiki?
>
--
I think that we should do some profiling on normal usecases to see if the
overhead we added didn't reach a point where we need to rethink the way we
store the data that represents a path.
--
I think we should wait to see what the test about the performance will show. I
myself don't think the overhead here is big. E.g. by using pointers you don't
need to copy that data when put into a map as would be needed if you just
change the class to use no pointers.
--
KoPathShape has 2 typedefs which are relevant for its one protected data
member KoSubpathList m_subpaths; typedef QList<KoPathPoint *> KoSubpath;
typedef QList<KoSubpath *> KoSubpathList;. This has two problems;
Putting a member in the exported header means we can never change it, we
should likely encapsulate this member. Moreover since a QList is a template
class, so not great to use in an installed header.
Why is using a template class not good to use in a header? Encapsulating the
member is probably a good idea, but then we should look hard at which
functions we have to provide to make modifying the member not be less efficient.
jaham
The two typedefs hide this a bit but we use a pointer to a QList. Which is
frowned upon behavior. The user of the API still has to know this is a QList
as she will need to use the QList api to do something with it. This is
something we should likely not advertise as a way of working for all pathshape
based plugin implementations.
I don't know why it is a list of pointers to subpaths, Thorsten can you
explain your reasoning please. jaham
--
That a pointer is used has nothing to do with the fact that the user has to
know it is a QLIst. I'm against wrapping the stuff inside class as Jan already
pointed out will reduce performance.
The reason why pointers are used are historical. At the beginning only the
pointer where stored in the commands. That got changed over time to no longer
be the case. However as written above I think before we change that we should
really make performance tests to see if that is a problem or not.
--
On KoPathShape there are various method protected but only used by the object
itself. They can be made private and be moved out of the public header as long
as they are not used externally.
QRectF handleRect(const QPointF &p, qreal radius) const;
This one is used at least in KoParameterShape as well. Not sure if we could
move that helper function to the flake namespace. jaham
void applyViewboxTransformation(const KoXmlElement & element);
Yes can be made private. jaham
More methods that are all private. I didn't check usage, but they are
candidates to move to the private object.
void map(const QMatrix &matrix);
void updateLast(KoPathPoint ** lastPoint);
void closeSubpath(KoSubpath *subpath);
void closeMergeSubpath(KoSubpath *subpath);
KoSubpath * subPath(int subpathIndex) const;
void paintDebug(QPainter &painter);
No objections to move them to the private object. jaham
--
I agree with Jan
--
KoParameterShape::setModified(bool) is meant to indicate that the parameter
shape no longer can be modified using its parameters. In other words, that it
can only be modified by altering its individual knots. Essentially turning it
into a KoPathShape. (please correct me if this is wrong). I think the naming
we have currently is not making this significant but clear. Modified is not a
good description for this as that can be seen to be a very different concept as
well, equally applicable to this shape type.
I agree that the name is not the most obvious. Maybe something like
isParametrized() or similar would be better, but this would invert the meaning
so we have to be careful when renaming. jaham
--
I suggest setParametric to be in line with isParametricShape which should be
renamed to isParametric. But as Jan noted this is invers logic and needs to be
changed in the places where it is called.
--
KoPointGroup is a non-exported class but appears in an installed header. We
should somehow move that out of the header.
I would like to get rid of that class completely. jaham
--
The cIass is needed for the following:
* In svg it is possible when you use a close and the create a new subpath not
using
* a moveTo that the new subpath starts at the same point as the last subpath.
As
* every point can only have 2 control points we have this class to group
points
* together which should be handled as one in e.g. a move.
Therefor I think we should keep the class. I don't think it needs to be
exported. So not installing the header should do the trick.
--
Having debugPath() as part of the public, installed header file is not a good
idea. We would ship binaries where this method is not even there and would
confuse external users.
Agreed. jaham
--
I really like to keep this functionality of the function. It is really
helpful for debugging problems with path shapes. I see the following possible
solutions:
1. always compile the function
easy to archive.
2. move the function to live in KoPathShape.cpp
However this has the drawback that it can only be used inside KoPathShape.
So it is no longer as useful as it is at the moment as it can't be used in
the tool
3. Move to somewhere else.
This has the drawback that we need to give that place full access to the
internals of the KoPathShape e.g. by making it a friend.
4. Keep it as it is. Make sure the function does not show up in the
documentation. If you look in the header file you see it is only there it is a
debug build so that is not really a problem.
Looks like you missed a question I posted here: http://lists.kde.org/?l=kde-
commits&m=126068011209434&w=2
> However I think the step of getting the points can be removed instead the
> new points should be push_back 'ed at the end of handles or?
Can we get rid of the getting the points?
Thorsten
_______________________________________________
koffice-devel mailing list
koffice-devel@kde.org
https://mail.kde.org/mailman/listinfo/koffice-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic