Review Request: implement marker settings in charts

[Inge Wallin]

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


In general very nice, but there are a couple of serious issues. See the comments below.


plugins/chartshape/ChartConfigWidget.h
<http://git.reviewboard.kde.org/r/105252/#comment11666>

    We should isolate kdchart more, not less. This means that we shouldn't introduce a dependency of kdchart enums into the config widget. Instead we should use an ODF specific enum here. This comment goes for all the places where the code references the kdchart marker enum, except for the internals of DataSet.cpp.



plugins/chartshape/ChartTool.cpp
<http://git.reviewboard.kde.org/r/105252/#comment11667>

    Same comment as above regarding kdchart internal enums.



plugins/chartshape/kdchart/src/KDChartMarkerAttributes.h
<http://git.reviewboard.kde.org/r/105252/#comment11668>

    I don't understand this change at all. Especially since I bothered to make a special comment that we keep the binary compatibility with other kdchart users by keeping the order of the markers. There is no way that kdab is going to accept a patch that changes something fundamental like this. For binary compatibility with old code we can never change enums, only add to them.


- Inge Wallin


On June 15, 2012, 6:25 a.m., Brijesh Patel wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105252/
> -----------------------------------------------------------
> 
> (Updated June 15, 2012, 6:25 a.m.)
> 
> 
> Review request for Calligra, Inge Wallin and C. Boemann.
> 
> 
> Description
> -------
> 
> This patch adds UI for markers and implements setting of marker for datasets.
> 
> 
> Diffs
> -----
> 
>   plugins/chartshape/ChartConfigWidget.h aa89815 
>   plugins/chartshape/ChartConfigWidget.cpp 4f4a705 
>   plugins/chartshape/ChartConfigWidget.ui b142635 
>   plugins/chartshape/ChartTool.h 779b7ef 
>   plugins/chartshape/ChartTool.cpp bab225a 
>   plugins/chartshape/DataSet.h 023c060 
>   plugins/chartshape/DataSet.cpp 2821254 
>   plugins/chartshape/Legend.cpp e74a77f 
>   plugins/chartshape/kdchart/src/KDChartAbstractDiagram.h a410cb8 
>   plugins/chartshape/kdchart/src/KDChartMarkerAttributes.h f8b7646 
>   plugins/chartshape/kdchartpatches/README 9d3f184 
>   plugins/chartshape/kdchartpatches/arrange-markers.diff PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/105252/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Brijesh Patel
> 
>

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