Review Request: Add markers (arrows and other stuff put at the end of a line)

Sun Nov 20 21:53:15 GMT 2011

> On Nov. 17, 2011, 12:33 p.m., Jarosław Staniek wrote:
> > libs/flake/KoLineBorder.h, line 103
> > <http://git.reviewboard.kde.org/r/103152/diff/2/?file=41211#file41211line103>
> >
> >     A small proposal. Can we start using passing via QPainter* recommended by Qt API design guidelines? This change would have to be adopted by other methods in this class and elsewhere but complying with the guideline in the new code would reduce amount of work in the future. 
> >     The same note applies to other methods you added in this patch.
> 
> Thorsten Zachmann wrote:
>     I think we should not start to use 2 different ways in our code. If we want to change it then we should to do it in one go. However I think using a QPainter& makes more sense as you don't need to check for it as if you would pass a pointer as it always expects a object to be there.

Regarding "one go" - either approach has consequences and given we have limited resources for the task I have not problem with either approeach.

Regarding passing QPainter: passing as pointer is extremely frequent in both Qt and KDE libs (see various paint() methods), and historically there is assumption there is no NULL passed. I'd go for the 'least surprise' principle.

> On Nov. 17, 2011, 12:33 p.m., Jarosław Staniek wrote:
> > libs/flake/KoMarker.cpp, line 92
> > <http://git.reviewboard.kde.org/r/103152/diff/2/?file=41214#file41214line92>
> >
> >     I am afraid this expression does not give unique name. To make it unique one would have to escape '_' character as well.
> 
> Thorsten Zachmann wrote:
>     Not sure I understand you comment here. This just makes sure that only valid characters are used in the identifier. The uniqness of the name is done one line down when inserting it into the styles.

OK

> On Nov. 17, 2011, 12:33 p.m., Jarosław Staniek wrote:
> > libs/flake/KoMarkerCollection.h, line 37
> > <http://git.reviewboard.kde.org/r/103152/diff/2/?file=41215#file41215line37>
> >
> >     no need for virtual here...
> 
> Thorsten Zachmann wrote:
>     As the base class has already a virtual destructor the class automatically gets a virtual destructor. And I prefer to also show that in the code.

Ok, overlooked the base class.

> On Nov. 17, 2011, 12:33 p.m., Jarosław Staniek wrote:
> > libs/flake/KoShapeSavingContext.cpp, line 232
> > <http://git.reviewboard.kde.org/r/103152/diff/2/?file=41227#file41227line232>
> >
> >     it's easier to use: !d->markerRefs.contains(marker)
> 
> Thorsten Zachmann wrote:
>     Using !d->markerRefs.contains(marker) would mean an additional lookup in the hash which is avoided by the above code. So this code is faster then using contains.

OK forgot there's it.value() ;P

- Jarosław

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/103152/#review8261
-----------------------------------------------------------

On Nov. 20, 2011, 7:09 a.m., Thorsten Zachmann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/103152/
> -----------------------------------------------------------
> 
> (Updated Nov. 20, 2011, 7:09 a.m.)
> 
> 
> Review request for Calligra.
> 
> 
> Description
> -------
> 
> At the sprint it was suggested to have this feature/bug fix in 2.4. Therefore positing for review. It adds one new string.
> 
> This add support for markers to calligra. 
> 
> Loading, saving, manipulation, work without problems
> 
> At the moment the document loaded need to have markers so that the user can select a marker on a path shape. I'm working on adding markers per default to the docker at the moment.
> 
> 
> This addresses bugs 260421, 260423 and 260431.
>     http://bugs.kde.org/show_bug.cgi?id=260421
>     http://bugs.kde.org/show_bug.cgi?id=260423
>     http://bugs.kde.org/show_bug.cgi?id=260431
> 
> 
> Diffs
> -----
> 
>   libs/flake/CMakeLists.txt 5face45 
>   libs/flake/KoDocumentResourceManager.h 2d6c8c0 
>   libs/flake/KoLineBorder.cpp a2f0645 
>   libs/flake/KoMarker.h PRE-CREATION 
>   libs/flake/KoMarker.cpp PRE-CREATION 
>   libs/flake/KoMarkerCollection.h PRE-CREATION 
>   libs/flake/KoMarkerCollection.cpp PRE-CREATION 
>   libs/flake/KoMarkerData.h PRE-CREATION 
>   libs/flake/KoMarkerData.cpp PRE-CREATION 
>   libs/flake/KoMarkerSharedLoadingData.h PRE-CREATION 
>   libs/flake/KoMarkerSharedLoadingData.cpp PRE-CREATION 
>   libs/flake/KoPathShape.h 5b5b91b 
>   libs/flake/KoPathShape.cpp 1898862 
>   libs/flake/KoPathShapeFactory.cpp f3a98b8 
>   libs/flake/KoPathShape_p.h e1e2843 
>   libs/flake/KoShapeLoadingContext.cpp 9d3d1da 
>   libs/flake/KoShapeSavingContext.h a06e040 
>   libs/flake/KoShapeSavingContext.cpp 34f55c8 
>   libs/flake/commands/KoPathShapeMarkerCommand.h PRE-CREATION 
>   libs/flake/commands/KoPathShapeMarkerCommand.cpp PRE-CREATION 
>   libs/widgets/CMakeLists.txt 473a264 
>   libs/widgets/KoMarkerItemDelegate.h PRE-CREATION 
>   libs/widgets/KoMarkerItemDelegate.cpp PRE-CREATION 
>   libs/widgets/KoMarkerModel.h PRE-CREATION 
>   libs/widgets/KoMarkerModel.cpp PRE-CREATION 
>   libs/widgets/KoMarkerSelector.h PRE-CREATION 
>   libs/widgets/KoMarkerSelector.cpp PRE-CREATION 
>   libs/widgets/KoStrokeConfigWidget.h 3ae44b9 
>   libs/widgets/KoStrokeConfigWidget.cpp 9b2532f 
>   marker_todo.txt PRE-CREATION 
>   plugins/dockers/strokedocker/StrokeDocker.h dde8db7 
>   plugins/dockers/strokedocker/StrokeDocker.cpp 82f18b5 
> 
> Diff: http://git.reviewboard.kde.org/r/103152/diff/diff
> 
> 
> Testing
> -------
> 
> Tested various documents with markers and they all worked without problems. Tested manipulation of path shapes that have a marker attached.
> 
> 
> Thanks,
> 
> Thorsten Zachmann
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/calligra-devel/attachments/20111120/9a103b4a/attachment.htm>