Review Request: Implement saving for the KoUnavailShape

Sun Feb 13 05:09:10 GMT 2011

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

Please implement the comments and post for review again.

braindump/src/SectionsIO.cpp
<http://git.reviewboard.kde.org/r/100629/#comment1148>

    I think the KoEmbeddedDocumentSaver and the KoEmbeddedFileSaver classes should be merged to be only one class. There can be two different methods to add files. This removes the need to add an additional parameter and to change all the files and will make the patch much smaller

braindump/src/SectionsIO.cpp
<http://git.reviewboard.kde.org/r/100629/#comment1149>

    please don't use not use !

libs/flake/KoUnavailShape.cpp
<http://git.reviewboard.kde.org/r/100629/#comment1150>

    I think it makes more sense to use a QList<FileEntry> instead a list of pointers. No need for an additional new/delete

libs/flake/KoUnavailShape.cpp
<http://git.reviewboard.kde.org/r/100629/#comment1151>

    looks like embeddedFiles is not deleted

libs/flake/KoUnavailShape.cpp
<http://git.reviewboard.kde.org/r/100629/#comment1152>

    Please remove the commented out code.

libs/flake/KoUnavailShape.cpp
<http://git.reviewboard.kde.org/r/100629/#comment1153>

    Please remove commented out code or if you like to keep it please give a comment to say like //uncomment to get detailed debug

libs/flake/KoUnavailShape.cpp
<http://git.reviewboard.kde.org/r/100629/#comment1154>

    Any reason to copy the data here instead of using the parameters directly when used.

libs/flake/KoUnavailShape.cpp
<http://git.reviewboard.kde.org/r/100629/#comment1155>

    Please don't leave space between the type and the variable name. We don't use that code style in flake.

libs/flake/KoUnavailShape.cpp
<http://git.reviewboard.kde.org/r/100629/#comment1156>

    For strange object names that can have very bad side effects like creating invalid xml. Checking it inside the tag where it should be should fix that problem

libs/flake/KoUnavailShape.cpp
<http://git.reviewboard.kde.org/r/100629/#comment1159>

    how about adding a method to KoOdfLoadingContext to get the manifestEntry per path. This removes the need to iterate over all manifest entries.

libs/flake/KoUnavailShape.cpp
<http://git.reviewboard.kde.org/r/100629/#comment1158>

    you can use temp instead of looking it up again with manifest.value(j)

libs/flake/KoUnavailShape.cpp
<http://git.reviewboard.kde.org/r/100629/#comment1160>

    please add comment why it is uncommented

libs/flake/KoUnavailShape.cpp
<http://git.reviewboard.kde.org/r/100629/#comment1161>

    Would it not make sense to store embedded files on harddisk if a specific size is reached. The content is not really needed in memory and doing it like that should save quite some memory.

libs/odf/KoEmbeddedFileSaver.h
<http://git.reviewboard.kde.org/r/100629/#comment1162>

    Please pass contents as a const & 

libs/odf/KoEmbeddedFileSaver.h
<http://git.reviewboard.kde.org/r/100629/#comment1163>

    please pass contents as const &

libs/odf/KoEmbeddedFileSaver.h
<http://git.reviewboard.kde.org/r/100629/#comment1164>

    The method should be const

libs/odf/KoEmbeddedFileSaver.cpp
<http://git.reviewboard.kde.org/r/100629/#comment1165>

    any reason to make the number at least 4 chars long?

libs/odf/KoEmbeddedFileSaver.cpp
<http://git.reviewboard.kde.org/r/100629/#comment1166>

    still it can be passed as a const reference here. True you need to copy it when saved to the entry.

libs/odf/KoOdfManifest.h
<http://git.reviewboard.kde.org/r/100629/#comment1167>

    please write out the parameters

- Thorsten

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/b0f1c711/attachment.htm>