Review Request: Implement saving for the KoUnavailShape

[Inge Wallin]

> On Feb. 13, 2011, 5:09 a.m., Thorsten Zachmann wrote:
> > libs/odf/KoEmbeddedFileSaver.cpp, line 82
> > <http://git.reviewboard.kde.org/r/100629/diff/1/?file=8942#file8942line82>
> >
> >     any reason to make the number at least 4 chars long?

It just felt right.


> On Feb. 13, 2011, 5:09 a.m., Thorsten Zachmann wrote:
> > libs/odf/KoEmbeddedFileSaver.h, line 56
> > <http://git.reviewboard.kde.org/r/100629/diff/1/?file=8941#file8941line56>
> >
> >     Please pass contents as a const &

No, I don't think we should do that, and there is a good reason for it: When the call to saveFile and embedFile is done, there is no way for the KoEmbeddedFileSaver to know what the shape does with the QByteArray.  The QByteArray is implicitly shared, meaning it is ref counted and by creating a copy this way I protect against the contents being deleted between the call to saveFile and when the contents is actually saved to the store.  But I should really add a comment about that.


> On Feb. 13, 2011, 5:09 a.m., Thorsten Zachmann wrote:
> > libs/flake/KoUnavailShape.cpp, line 335
> > <http://git.reviewboard.kde.org/r/100629/diff/1/?file=8923#file8923line335>
> >
> >     how about adding a method to KoOdfLoadingContext to get the manifestEntry per path. This removes the need to iterate over all manifest entries.

I can do that, but I think we should instead create a real Manifest class that could have these methods.


> On Feb. 13, 2011, 5:09 a.m., Thorsten Zachmann wrote:
> > libs/flake/KoUnavailShape.cpp, line 213
> > <http://git.reviewboard.kde.org/r/100629/diff/1/?file=8923#file8923line213>
> >
> >     Any reason to copy the data here instead of using the parameters directly when used.

To simplify the code below (the cost is not that big since it's implicitly shared).  But I can remove it if you think it's too expensive.


> On Feb. 13, 2011, 5:09 a.m., Thorsten Zachmann wrote:
> > braindump/src/SectionsIO.cpp, line 165
> > <http://git.reviewboard.kde.org/r/100629/diff/1/?file=8912#file8912line165>
> >
> >     please don't use not use !

I didn't put it there, i just copied the code just above it.  You have to tell Cyrille to stop putting them there.  The code above has since been fixed by somebody to make it compile on Solaris, I think.  All this said, of course I'll remove it.


- Inge


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


On Feb. 11, 2011, 9:36 a.m., Inge Wallin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100629/
> -----------------------------------------------------------
> 
> Review request for Calligra.
> 
> 
> Summary
> -------
> 
> This patch implements saving in the KoUnavailShape.
> 
> The method it uses is to introduce a new class, KoEmbeddedFileSaver, in koodf. This class is a bit like the KoEmbeddedObjectSaver class, and is created and instantiated in the same places. It is also part of the KoShapeSavingContext.
> 
> As a bonus, this patch also implements saving for the Vector shape, since I used that for testing in early stages.
> 
> 
> Diffs
> -----
> 
>   braindump/src/SectionsIO.cpp 9d8949b 
>   karbon/common/KarbonDocument.cpp 1a6a2a5 
>   kformula/flake/FormulaDocument.cpp 84872fa 
>   kformula/flake/KoFormulaTool.cpp d9eada1 
>   krita/ui/flake/kis_shape_layer.cc f695b8a 
>   krita/ui/flake/kis_shape_selection.cpp f5e2f28 
>   libs/flake/KoDrag.cpp 8ed2656 
>   libs/flake/KoDragOdfSaveHelper.h b1e6212 
>   libs/flake/KoDragOdfSaveHelper.cpp 727e6c5 
>   libs/flake/KoShapeSavingContext.h a06e040 
>   libs/flake/KoShapeSavingContext.cpp fd8fb41 
>   libs/flake/KoUnavailShape.cpp 3f10acf 
>   libs/kokross/KoScriptingOdf.cpp 17a4f7d 
>   libs/kopageapp/KoPADocument.cpp 2ef4ac1 
>   libs/kopageapp/KoPAOdfPageSaveHelper.h d8f064a 
>   libs/kopageapp/KoPAOdfPageSaveHelper.cpp c60d0d8 
>   libs/kopageapp/KoPAPastePage.cpp 01bafe0 
>   libs/kopageapp/KoPASavingContext.h c678e2c 
>   libs/kopageapp/KoPASavingContext.cpp 52524bf 
>   libs/kotext/KoTextDrag.cpp f30cb5c 
>   libs/kotext/KoTextOdfSaveHelper.h 155aecd 
>   libs/kotext/KoTextOdfSaveHelper.cpp d012557 
>   libs/kotext/KoTextShapeSavingContext.h 4d27bc4 
>   libs/kotext/KoTextShapeSavingContext.cpp d5ade20 
>   libs/kotext/opendocument/tests/TestChangeLoading.cpp ac42ddd 
>   libs/kotext/opendocument/tests/TestLoading.cpp e6a790a 
>   libs/main/KoDocument.cpp 956e179 
>   libs/odf/CMakeLists.txt 1a6a131 
>   libs/odf/KoEmbeddedDocumentSaver.cpp a8e13d8 
>   libs/odf/KoEmbeddedFileSaver.h PRE-CREATION 
>   libs/odf/KoEmbeddedFileSaver.cpp PRE-CREATION 
>   libs/odf/KoOdfDocument.h c242776 
>   libs/odf/KoOdfLoadingContext.h 023b92f 
>   libs/odf/KoOdfLoadingContext.cpp ce53992 
>   libs/odf/KoOdfManifest.h PRE-CREATION 
>   libs/odf/KoOdfManifest.cpp PRE-CREATION 
>   plugins/chartshape/ChartDocument.cpp f85bb19 
>   plugins/chartshape/ChartShape.cpp edce6be 
>   plugins/vectorshape/VectorShape.cpp 9965d68 
>   tables/Cell.cpp 7aa8306 
>   tables/DocBase.cpp 5ccaef3 
>   words/part/KWDocument.cpp f3dc5d4 
>   words/part/KWOdfWriter.h 4264e99 
>   words/part/KWOdfWriter.cpp b507bbf 
> 
> Diff: http://git.reviewboard.kde.org/r/100629/diff
> 
> 
> Testing
> -------
> 
> Tested with a simple odt file with an embedded spreadsheet.
> 
> 
> Thanks,
> 
> Inge
> 
>

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