Calligra/Libs/Flake/PathShapes API review

Data store in general

While doing the cleanup of flake I need to make all exported classes future proof, this means to use a d-pointer so we can always add more variables later and not break binary compatiblity. This has happened for the KoPathSegment and the KoPathPoint objects already as well as others. But these two seem relevant for the overhead that this concept brings. Each class now gets an extra malloc on creation for the d-pointer. And the objects themselves are also classes which require a malloc. So, if you imagine a big vector object then each 'knot' in the object is 2 mallocs and each path connecting two knots is another two mallocs.

KoPathSegment really is only a helper class to centralize some bezier curve math into one place. KoPathSegments are not stored inside KoPathShape, they are created on the fly when calling segmentsAt or segmentByIndex jaham

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.

Ideally this needs to be done before we can freeze flake in a binary and source compatible way as the KoPathSegment and KoPathPoint objects are exported so a change in the way they are storing and or sharing their data will most likely be reflected in the public API.

API problems

• 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

• 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. jahamDone; Thomas
  • void applyViewboxTransformation(const KoXmlElement & element);
    Yes can be made private. jahamDone; Thomas

• 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 Done; Thomas

• 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 Done; Thomas.

Minor issues

• 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

• KoPathShape::fromQPainterPath() is a static method that is implements the factory design pattern and instantiates a new object. So ownership is a factor. I suggest to rename this to start with 'create' to be consistent with the design pattern and just as important be inconsistent with Qt (see QString) which returns implicitly shared objects so is significantly different.
  Ok, I have no problems with that if it makes the intent of the function clearer. jaham Done; Thomas
• 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
• pointCountSubpath() has an inconsistent naming; there are subpathCount() and pointCount() as well, I suggest to make all 3 end with 'count'
  I always wanted to change that, but haven't had time. So I am all for it. jaham Done; Thomas
• KoPointPosition is typedeffed in KoPathShape.h but never used.
  Indeed is not used anywhere anymore, so we can remove that. jaham Done; Thomas
• KoPathShapePointIndexMap is typedeffed in KoPathShape.h but never used.
  Indeed is not used anywhere anymore, so we can remove that. jaham Done; Thomas