Review Request: Add support for 3D scenes to libs/odf

[Inge Wallin]

> On April 22, 2012, 5:08 a.m., Thorsten Zachmann wrote:
> > libs/odf/Ko3dScene.h, lines 79-80
> > <http://git.reviewboard.kde.org/r/104688/diff/1/?file=58308#file58308line79>
> >
> >     You should pass the KoShapeLoadingContext and the KoShapeSavingContext to this methods so in case it is needed later we don't need to change the signature.
> >     That is true for all those methods.

This is where I disagree.  I am trying hard to decouple the odf library from the flake one and passing the KoShape contexts would hardly be a step in the right direction.  I could do it with the KoOdfLoadingContext and KoOdfSavingConext though.

Besides, are you sure?  *None* of the storage classes in libs/odf used any Saving or Loading context except KoOdfDocument which is a bit special.  I would be all for this change but in that case we should do it also for the other classes and mark it as a policy somewhere.


> On April 22, 2012, 5:08 a.m., Thorsten Zachmann wrote:
> > libs/odf/Ko3dScene.h, lines 99-100
> > <http://git.reviewboard.kde.org/r/104688/diff/1/?file=58308#file58308line99>
> >
> >     As this is not a shape I'm not sure it makes sense to split those into two different methods.

They are not shapes but they are used in shapes, so it is still necessary.  You will see when I add the user as you requested.


> On April 22, 2012, 5:08 a.m., Thorsten Zachmann wrote:
> > libs/odf/Ko3dScene.h, lines 83-86
> > <http://git.reviewboard.kde.org/r/104688/diff/1/?file=58308#file58308line83>
> >
> >     Please move the implementation to the cpp file.
> >     Please remove the additional spaces.
> >

Spaces makes the code more readable, and it is not mentioned in the standard. I read it from start to end just to be sure.

What is mentioned is 'use one space after keywords' like 'if' and 'while'.


> On April 22, 2012, 5:08 a.m., Thorsten Zachmann wrote:
> > libs/odf/Ko3dScene.cpp, line 150
> > <http://git.reviewboard.kde.org/r/104688/diff/1/?file=58309#file58309line150>
> >
> >     The save method of the Ko3dScene should also save the tag drd3:screen tag.

dr3d:screen doesn't exist. At least I couldn't find it in ODF 1.2.  Where did you see that?


> On April 22, 2012, 5:08 a.m., Thorsten Zachmann wrote:
> > libs/odf/Ko3dScene.cpp, line 51
> > <http://git.reviewboard.kde.org/r/104688/diff/1/?file=58309#file58309line51>
> >
> >     Please don't give a default argument of "". There is already a default value of an empty QString so need to pass an empty string that needs to be converted to a QString.
> >     That is true for more place in the code.

Fixed


> On April 22, 2012, 5:08 a.m., Thorsten Zachmann wrote:
> > libs/odf/Ko3dScene.h, lines 103-113
> > <http://git.reviewboard.kde.org/r/104688/diff/1/?file=58308#file58308line103>
> >
> >     Implementation should be moved to cpp file

This is part of d-pointer'ify it.  Fixed.


- Inge


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


On April 21, 2012, 12:39 p.m., Inge Wallin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104688/
> -----------------------------------------------------------
> 
> (Updated April 21, 2012, 12:39 p.m.)
> 
> 
> Review request for Calligra.
> 
> 
> Description
> -------
> 
> This new code adds support for 3D scenes in libs/odf. It was ported from my threedscene branch when I found out that exactly the same attributes are used in 3D charts.
> 
> The only tricky thing with this code is that the attributes are not stored in a style but in the actual elements (dr3d:scene and chart:plot-area respectively). This means that we have to save the attributes and the children separately since the saving code of these objects may want to add other attributes *and* children.
> 
> You may note that I didn't d-pointer-ify the class.  Right now the odf library is not binary compatibility frozen so I don't strictly need to but if there is a strong wish for it I can do it.
> 
> 
> Diffs
> -----
> 
>   libs/odf/CMakeLists.txt c411a8c 
>   libs/odf/Ko3dScene.h PRE-CREATION 
>   libs/odf/Ko3dScene.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/104688/diff/
> 
> 
> Testing
> -------
> 
> I have tested this code while working on the 3D Scene shape.
> 
> 
> Thanks,
> 
> Inge Wallin
> 
>

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