Review Request 122045: Calligra filters: refactor chart handling and make it sane

Jarosław Staniek staniek at kde.org
Mon Feb 2 15:20:14 GMT 2015 

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122045/#review75215
-----------------------------------------------------------

Ship it!


Looks cool! Minor notes, mostly for the future.


filters/libmso/NumberFormatParser.cpp
<https://git.reviewboard.kde.org/r/122045/#comment52009>

    Future: I recommend using faster QString::compare



filters/libmso/NumberFormatParser.cpp
<https://git.reviewboard.kde.org/r/122045/#comment52010>

    Future: I recommend using faster QString::compare



filters/libmso/NumberFormatParser.cpp
<https://git.reviewboard.kde.org/r/122045/#comment52011>

    Future: I recommend using faster QString::compare



filters/libmso/NumberFormatParser.cpp
<https://git.reviewboard.kde.org/r/122045/#comment52012>

    Future: I recommend using faster QString::compare



filters/libmso/NumberFormatParser.cpp
<https://git.reviewboard.kde.org/r/122045/#comment52013>

    Future: I recommend using faster QString::compare



filters/libmsooxml/MsooXmlDrawingTableStyleReader.cpp
<https://git.reviewboard.kde.org/r/122045/#comment52014>

    Same unit, so I guess "MsooXmlTheme.h" shall be used



filters/libmsooxml/MsooXmlImport.cpp
<https://git.reviewboard.kde.org/r/122045/#comment52015>

    Same unit, so I guess "MsooXmlTheme.h" shall be used



filters/libmsooxml/MsooXmlTheme.h
<https://git.reviewboard.kde.org/r/122045/#comment52017>

    add //!



filters/libmsooxml/MsooXmlTheme.h
<https://git.reviewboard.kde.org/r/122045/#comment52016>

    Now sure but while we're renaming to make it saner:
    -> lineStyles
    -
    -or do we want to keep naming consistent with MSO naming here?



filters/libodf2/chart/Charting.h
<https://git.reviewboard.kde.org/r/122045/#comment52019>

    Future: how about moving the implementation out of this header?



filters/libodf2/chart/Charting.h
<https://git.reviewboard.kde.org/r/122045/#comment52020>

    remove the line



filters/libodf2/chart/Charting.h
<https://git.reviewboard.kde.org/r/122045/#comment52018>

    Future: How about to removing m_ when the member is public? The use code using it will be clearer. The same for 4 members below.



filters/libodf2/chart/KoOdfChartWriter.h
<https://git.reviewboard.kde.org/r/122045/#comment52021>

    -- indent



filters/words/docx/import/CMakeLists.txt
<https://git.reviewboard.kde.org/r/122045/#comment52022>

    Will these three files be moved to a shared lib instead one day?


- Jarosław Staniek


On Jan. 18, 2015, 4:26 a.m., Inge Wallin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/122045/
> -----------------------------------------------------------
> 
> (Updated Jan. 18, 2015, 4:26 a.m.)
> 
> 
> Review request for Calligra, Lassi Nieminen and Jarosław Staniek.
> 
> 
> Repository: calligra
> 
> 
> Description
> -------
> 
> This patch is a refactoring of how charts are handled in the filters. There are two purposes of it:
> 1. Make it sane in general. As it was before, the various filters where reaching into each other for bits and pieces of code, and the naming was strange in places. There was also a strange mixture of MS binary and MSOOXML concepts in the same files.
> 2. To prepare for the next step which is sharing storage classes for charts between the filters and the chart shape. As it is now, many defines and both loading and saving is duplicated all over Calligra. This is not the way we want things to be.
> 
> There is hardly any new code in here, only rearranging of what was already there. Here is a summary (everything happens inside the filters/ tree):
>  - Factor out common parts and move them to libodf2/charts
>  - Factor out the parts common to MS handling and move them to libmso/
>  - Move common parts within the ooxml filters from the xlsx tree to libmsooxml/
>  - Move things that only have to do with MS binary parts to libmso/
>  - Rename the vaguely named namespace "Charting" to "KoChart"
>  - Rename ChartExport to KoOdfChartWriter, which should be clearer
>  - Make the filters use and link to the libraries instead of each other
> 
> 
> Diffs
> -----
> 
>   filters/libodf2/chart/PLAN PRE-CREATION 
>   filters/sheets/excel/export/CMakeLists.txt acc5a73 
>   filters/sheets/excel/import/CMakeLists.txt de653ee 
>   filters/sheets/excel/import/ExcelImport.cpp abf4c52 
>   filters/sheets/excel/import/excelimporttoods.cc 5d6887b 
>   filters/sheets/excel/sidewinder/CMakeLists.txt 097e2a8 
>   filters/sheets/excel/sidewinder/chartsubstreamhandler.h d22c02b 
>   filters/sheets/excel/sidewinder/chartsubstreamhandler.cpp ecd9071 
>   filters/sheets/excel/sidewinder/objects.h 5162cee 
>   filters/sheets/xlsx/CMakeLists.txt 294f048 
>   filters/sheets/xlsx/ChartExport.h aa9895d 
>   filters/sheets/xlsx/ChartExport.cpp 27b075b 
>   filters/sheets/xlsx/Charting.h 4fea470 
>   filters/words/msword-odf/CMakeLists.txt 2b975a1 
>   CMakeLists.txt 1f992dd 
>   filters/libmso/CMakeLists.txt 19aced3 
>   filters/libmso/MsoUtils.h PRE-CREATION 
>   filters/libmso/MsoUtils.cpp PRE-CREATION 
>   filters/libmso/NumberFormatParser.h PRE-CREATION 
>   filters/libmso/NumberFormatParser.cpp PRE-CREATION 
>   filters/libmso/XlsUtils.h PRE-CREATION 
>   filters/libmsooxml/CMakeLists.txt 7dc2d7f 
>   filters/libmsooxml/MsooXmlCommonReaderDrawingMLImpl.h e8748ef 
>   filters/libmsooxml/MsooXmlDrawingTableStyleReader.cpp c7bd7fe 
>   filters/libmsooxml/MsooXmlImport.cpp 49ad484 
>   filters/libmsooxml/MsooXmlTheme.h PRE-CREATION 
>   filters/libmsooxml/MsooXmlThemesReader.h 9682c34 
>   filters/libmsooxml/MsooXmlThemesReader.cpp 5335029 
>   filters/libmsooxml/MsooXmlUtils.h 055fa15 
>   filters/libmsooxml/MsooXmlUtils.cpp 31df41e 
>   filters/libodf2/CMakeLists.txt 3792771 
>   filters/libodf2/chart/Charting.h PRE-CREATION 
>   filters/libodf2/chart/KoOdfChartWriter.h PRE-CREATION 
>   filters/libodf2/chart/KoOdfChartWriter.cpp PRE-CREATION 
>   filters/sheets/xlsx/NumberFormatParser.h e877d40 
>   filters/sheets/xlsx/NumberFormatParser.cpp 433ffe8 
>   filters/sheets/xlsx/XlsxChartOdfWriter.h PRE-CREATION 
>   filters/sheets/xlsx/XlsxChartOdfWriter.cpp PRE-CREATION 
>   filters/sheets/xlsx/XlsxImport.cpp b0091ca 
>   filters/sheets/xlsx/XlsxUtils.h 2858ce4 
>   filters/sheets/xlsx/XlsxXmlChartReader.h a2b07e8 
>   filters/sheets/xlsx/XlsxXmlChartReader.cpp 0434961 
>   filters/sheets/xlsx/XlsxXmlCommentsReader.h e025a06 
>   filters/sheets/xlsx/XlsxXmlCommonReader.h e406b0a 
>   filters/sheets/xlsx/XlsxXmlDocumentReader.h 6395799 
>   filters/sheets/xlsx/XlsxXmlDocumentReader.cpp a4961ee 
>   filters/words/docx/import/DocxXmlDocumentReader.h bfb9171 
>   filters/words/docx/import/DocxXmlDocumentReader.cpp ae7cb0e 
>   filters/stage/pptx/CMakeLists.txt 36434f9 
>   filters/stage/pptx/PptxXmlSlideReader.cpp 702a381 
>   filters/words/docx/import/CMakeLists.txt 70490d4 
>   filters/sheets/xlsx/XlsxXmlWorksheetReader.h 559a029 
>   filters/sheets/xlsx/XlsxXmlWorksheetReader.cpp 74e1302 
>   filters/sheets/xlsx/XlsxXmlStylesReader.h 4793ae3 
>   filters/sheets/xlsx/XlsxXmlDrawingReader.h 69c75ac 
>   filters/sheets/xlsx/XlsxXmlDrawingReader.cpp def1ff6 
> 
> Diff: https://git.reviewboard.kde.org/r/122045/diff/
> 
> 
> Testing
> -------
> 
> There is no new code at all, only rearrangement. But all the filters work as expected.
> 
> 
> Thanks,
> 
> Inge Wallin
> 
>

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

More information about the calligra-devel mailing list