Review Request 111545: Port komain away from KParts

Friedrich W. H. Kossebau kossebau at kde.org
Thu Jul 18 15:55:25 BST 2013 

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


Awesome work, good to see you pushing this forward, go, Boud, go!

Did not yet compile or give it some very deep thoughts, but at least a first set of comments, see inline comments.

Also all the file deletions in krita are better done in a separate patch:
krita/sdk/createplugin.rb:deleted
krita/sdk/createtest.py:deleted
krita/sdk/pop_extract.rb:deleted
...

(BTW, I might soon start to create small KPart wrappers for all Calligra app modules, so Calligra integrates again with KPart world...)


braindump/plugins/quickstates/BraindumpQuickStatesPlugin.cpp
<http://git.reviewboard.kde.org/r/111545/#comment26744>

    Why not still
      : QObject(parent)
    ?
    
    Who is doing memory management for this plugin?
    Otherwise please remove this parent parameter, only confuses.



cmake/modules/FindCalligraLibs.cmake.in
<http://git.reviewboard.kde.org/r/111545/#comment26745>

    please remove line with spaces



karbon/data/karbon_plugin.desktop
<http://git.reviewboard.kde.org/r/111545/#comment26746>

    If the service type is Karbon/ViewPlugin, then the desktop file IMHO better has a similar name, e.g. karbon_viewplugin.desktop



karbon/plugins/flattenpath/karbonflattenpath.desktop
<http://git.reviewboard.kde.org/r/111545/#comment26747>

    If the module has only one plugin, then perhaps better name the desktop file the same like the module name, i.e. here 
    karbon_flattenpathplugin.desktop



karbon/plugins/refinepath/karbonrefinepath.desktop
<http://git.reviewboard.kde.org/r/111545/#comment26748>

    Same as other desktop file, name ideally identic to module name.



karbon/plugins/roundcorners/karbonroundcorners.desktop
<http://git.reviewboard.kde.org/r/111545/#comment26749>

    as before, desktop file name == module file name?



karbon/plugins/whirlpinch/karbonwhirlpinch.desktop
<http://git.reviewboard.kde.org/r/111545/#comment26750>

    desktop file name...



libs/main/CMakeLists.txt
<http://git.reviewboard.kde.org/r/111545/#comment26751>

    remove empty line with spaces



libs/main/CMakeLists.txt
<http://git.reviewboard.kde.org/r/111545/#comment26752>

    spaces



libs/main/KoMainWindow.cpp
<http://git.reviewboard.kde.org/r/111545/#comment26753>

    What is the difference between "activePart" and "m_activePart"?
    
    And shouldn't members of the Private class not be without the m_ prefix?



plan/plugins/scripting/planscripting.desktop
<http://git.reviewboard.kde.org/r/111545/#comment26754>

    desktopfile name <- module name



plugins/kexi/spreadsheet/CMakeLists.txt
<http://git.reviewboard.kde.org/r/111545/#comment26755>

    please remove empty line with soaces



plugins/reporting/web/CMakeLists.txt
<http://git.reviewboard.kde.org/r/111545/#comment26756>

    empty line with spaces


Awesome work!

All the file deletions in krita are better done in a separate patch:
krita/sdk/createplugin.rb:deleted
krita/sdk/createtest.py:deleted
krita/sdk/pop_extract.rb:deleted
...

- Friedrich W. H. Kossebau


On July 17, 2013, 11:41 a.m., Boudewijn Rempt wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111545/
> -----------------------------------------------------------
> 
> (Updated July 17, 2013, 11:41 a.m.)
> 
> 
> Review request for Calligra.
> 
> 
> Description
> -------
> 
> This is a continuation of the KoDocument/KoPart split that Camilla and I worked on last year, and it is a step towards the MVC refactoring discussed in Essen.
> 
> We actually did not use any of the kparts functionality anymore: not the embedding of Calligra parts in other applications, not document embedding, and the various plugins that were kparts actually did not do anything beyond what a kxmlguiclient plugin did.
> 
> This patch makes KoView and KoPart a KXMLGuiClient and lets KoMainWindow manage the KoParts. It is the first step in further refactoring:
> 
> * Make KoMainWindow a generic window for Calligra apps
> * Make KoDocument responsible for all loading and saving
> * Make KoPart a factory class that can create documents, views and canvaes
> * Make KoView more properly an application-specific mainwindow implementation for an application that can show one or more documents
> 
> Current issues:
> 
> * In CalligraPlan I have commented out some code that didn't seem to actually do anything but was dependent on KParts. That needs to be checked by Friedrich for regressions
> * KoMainWindow has way too many local KoPart pointers... That needs to be cleaned up.
> 
> Then I want to move the load/save code from KoPart to KoDocument and start the bigger refactoring... That will hopefully give us split view/mdi/tabbed view of documents again. And make the flipbook and animation plugins for krita easier to handle.
> 
> For easy testing I recommend checking out the krita-mvc-rempt branch.
> 
> 
> This addresses bugs 310310, 313482 and 313483.
>     http://bugs.kde.org/show_bug.cgi?id=310310
>     http://bugs.kde.org/show_bug.cgi?id=313482
>     http://bugs.kde.org/show_bug.cgi?id=313483
> 
> 
> Diffs
> -----
> 
>   braindump/src/CMakeLists.txt 7bfe50b 
>   braindump/plugins/quickstates/BraindumpQuickStatesPlugin.cpp bc27946 
>   braindump/plugins/quickstates/CMakeLists.txt ecadaea 
>   braindump/plugins/quickstates/BraindumpQuickStatesPlugin.h b9625b0 
>   CMakeLists.txt 4f38f33 
>   braindump/src/View.cpp ef19171 
>   cmake/modules/FindCalligraLibs.cmake.in 1e49d17 
>   devtools/cstester/cstester.cpp b88619b 
>   extras/okularodpgenerator/OkularOdpGenerator.cpp db1d5e5 
>   flow/part/FlowPart.cpp 5fedf83 
>   karbon/data/CMakeLists.txt 6b1e329 
>   karbon/data/karbon_module.desktop 7acc680 
>   karbon/data/karbon_plugin.desktop PRE-CREATION 
>   karbon/plugins/flattenpath/CMakeLists.txt f83620b 
>   karbon/plugins/flattenpath/FlattenPathPlugin.h 622973c 
>   karbon/plugins/flattenpath/FlattenPathPlugin.cpp d03ccca 
>   karbon/plugins/flattenpath/karbonflattenpath.desktop PRE-CREATION 
>   karbon/plugins/refinepath/CMakeLists.txt caf6aa7 
>   karbon/plugins/refinepath/RefinePathPlugin.h be16e29 
>   karbon/plugins/refinepath/RefinePathPlugin.cpp b951adc 
>   karbon/plugins/refinepath/karbonrefinepath.desktop PRE-CREATION 
>   karbon/plugins/roundcorners/CMakeLists.txt 82b7159 
>   karbon/plugins/roundcorners/RoundCornersPlugin.h 5cd610a 
>   karbon/plugins/roundcorners/RoundCornersPlugin.cpp 7ac63bf 
>   karbon/plugins/roundcorners/karbonroundcorners.desktop PRE-CREATION 
>   karbon/plugins/whirlpinch/CMakeLists.txt 2e920e2 
>   karbon/plugins/whirlpinch/WhirlPinchPlugin.h 51280a1 
>   karbon/plugins/whirlpinch/WhirlPinchPlugin.cpp 94a3527 
>   karbon/plugins/whirlpinch/karbonwhirlpinch.desktop PRE-CREATION 
>   karbon/ui/KarbonFactory.cpp 8bbabbe 
>   karbon/ui/KarbonPart.cpp be13dfe 
>   karbon/ui/KarbonView.cpp 7e54ede 
>   karbon/ui/dockers/KarbonLayerDocker.cpp 1cf8395 
>   krita/plugins/assistants/RulerAssistant/CMakeLists.txt 957fb4f 
>   krita/plugins/extensions/shiva/shivagenerator.h 55894f4 
>   krita/plugins/paintops/libbrush/kis_brush_registry.cpp f54a619 
>   krita/plugins/paintops/particle/particle_paintop_plugin.h bd6b245 
>   krita/sdk/createplugin.rb 1f52c38 
>   krita/sdk/createtest.py 4b7843f 
>   krita/sdk/pop_extract.rb 0d88588 
>   krita/sdk/templates/docker/APPNAMELC.h 56c8d19 
>   krita/sdk/templates/docker/APPNAMELC.cc 740de47 
>   krita/sdk/templates/docker/APPNAMELC_dock.h 919d5d5 
>   krita/sdk/templates/docker/APPNAMELC_dock.cc 6379bfb 
>   krita/sdk/templates/docker/CMakeLists.txt f671812 
>   krita/sdk/templates/docker/krita_APPNAMELC.desktop f287069 
>   krita/sdk/templates/filefilter/APPNAMELC_converter.h ddbd5d0 
>   krita/sdk/templates/filefilter/APPNAMELC_converter.cc 2da0b2f 
>   krita/sdk/templates/filefilter/APPNAMELC_export.h 4e8a612 
>   krita/sdk/templates/filefilter/APPNAMELC_export.cc 9798937 
>   krita/sdk/templates/filefilter/APPNAMELC_import.h e2672a7 
>   krita/sdk/templates/filefilter/APPNAMELC_import.cc b063ec3 
>   krita/sdk/templates/filefilter/CMakeLists.txt bc7afdc 
>   krita/sdk/templates/filefilter/krita_APPNAMELC.desktop 736f9a5 
>   krita/sdk/templates/filefilter/krita_APPNAMELC_export.desktop 81ac62e 
>   krita/sdk/templates/filefilter/krita_APPNAMELC_import.desktop 0542ae3 
>   krita/sdk/templates/filter/APPNAMELC.h 141d813 
>   krita/sdk/templates/filter/APPNAMELC.cc 1ff583b 
>   krita/sdk/templates/filter/CMakeLists.txt f5e94e9 
>   krita/sdk/templates/filter/kritaAPPNAMELC.desktop b5b86af 
>   krita/sdk/templates/tool/CMakeLists.txt f4ec68c 
>   krita/sdk/templates/tool/kis_tool_APPNAMELC.h 01e49fa 
>   krita/sdk/templates/tool/kis_tool_APPNAMELC.cc 9ab759b 
>   krita/sdk/templates/tool/kritatoolAPPNAMELC.desktop af9ea01 
>   krita/sdk/templates/tool/tool_APPNAMELC.h ce9a416 
>   krita/sdk/templates/tool/tool_APPNAMELC.cc 1a162fa 
>   krita/sdk/templates/view/APPNAMELC.h 960576f 
>   krita/sdk/templates/view/APPNAMELC.cc cc761f6 
>   krita/sdk/templates/view/APPNAMELC.rc 3c9f9aa 
>   krita/sdk/templates/view/CMakeLists.txt 65be2c7 
>   krita/sdk/templates/view/kritaAPPNAMELC.desktop f287069 
>   krita/ui/kis_factory2.h 888b742 
>   krita/ui/kis_factory2.cc b7896e5 
>   krita/ui/kis_paintop_box.cc e97a400 
>   krita/ui/kis_part2.cpp 83d5f34 
>   krita/ui/kis_view2.cpp c8b8d84 
>   krita/ui/kis_view_plugin.h ff19b2a 
>   krita/ui/kis_view_plugin.cpp 69f5b9d 
>   libs/kokross/KoScriptingPart.h 4f4c933 
>   libs/kokross/KoScriptingPart.cpp 09b8cbb 
>   libs/kopageapp/KoPAView.h 80b9f78 
>   libs/kopageapp/KoPAView.cpp 58b4940 
>   libs/kopageapp/tests/CMakeLists.txt 0d44ee3 
>   libs/main/CMakeLists.txt 9b8503b 
>   libs/main/KoDocument.h 3372060 
>   libs/main/KoDocument.cpp c5605bd 
>   libs/main/KoDocumentEntry.cpp e1b03f7 
>   libs/main/KoDocument_p.h 6f3931f 
>   libs/main/KoFactory.h dc4ada0 
>   libs/main/KoFactory.cpp c361a11 
>   libs/main/KoFilterEntry.cpp 8e23eac 
>   libs/main/KoMainWindow.h 1b3503f 
>   libs/main/KoMainWindow.cpp 12423a7 
>   libs/main/KoPart.h d4ecf5f 
>   libs/main/KoPart.cpp 56e27b5 
>   libs/main/KoServiceProvider.h 35d603a 
>   libs/main/KoServiceProvider.cpp f063401 
>   libs/main/KoView.h 10b6403 
>   libs/main/KoView.cpp 1b4a722 
>   libs/rdf/CMakeLists.txt bd29ead 
>   plan/CMakeLists.txt 4ff06eb 
>   plan/kptmaindocument.cpp 333310c 
>   plan/kptpart.cpp 7674480 
>   plan/kptview.h 02acb77 
>   plan/kptview.cpp 3d5f27f 
>   plan/plan_viewplugin.desktop PRE-CREATION 
>   plan/plugins/scripting/CMakeLists.txt 836176d 
>   plan/plugins/scripting/ScriptingPart.cpp e6d6004 
>   plan/plugins/scripting/planscripting.desktop PRE-CREATION 
>   plan/workpackage/CMakeLists.txt a1fbb87 
>   plugins/chartshape/ChartShape.cpp 55d5eb8 
>   plugins/colorengines/lcms2/CMakeLists.txt faa07a9 
>   plugins/colorengines/lcms2/LcmsEnginePlugin.h 36a5497 
>   plugins/kexi/spreadsheet/CMakeLists.txt 78fbe7e 
>   plugins/reporting/barcode/CMakeLists.txt 7423872 
>   plugins/reporting/chart/CMakeLists.txt a0d6378 
>   plugins/reporting/maps/CMakeLists.txt 6bc29a0 
>   plugins/reporting/web/CMakeLists.txt 03a9aeb 
>   plugins/staging/CMakeLists.txt 754362f 
>   plugins/staging/googledocs/onlinedocument.h d4141f3 
>   plugins/staging/googledocs/onlinedocument.cpp aca9fa7 
>   plugins/staging/pivottables/CMakeLists.txt efb2753 
>   plugins/staging/pivottables/pivotplugin.h 17fc384 
>   plugins/staging/pivottables/pivotplugin.cpp 1bef7d2 
>   plugins/staging/pivottables/sheetspivottables.desktop PRE-CREATION 
>   sheets/CMakeLists.txt e0f4a02 
>   sheets/part/Part.cpp 7fd573e 
>   sheets/part/View.h 49bcdf6 
>   sheets/part/View.cpp 0dc055a 
>   sheets/plugins/scripting/CMakeLists.txt 27e3861 
>   sheets/plugins/scripting/ScriptingPart.h ac9e614 
>   sheets/plugins/scripting/ScriptingPart.cpp 26431b0 
>   sheets/plugins/scripting/sheetsscripting.desktop PRE-CREATION 
>   sheets/plugins/solver/CMakeLists.txt ad813e6 
>   sheets/plugins/solver/Solver.h 21ee006 
>   sheets/plugins/solver/Solver.cpp c87639e 
>   sheets/plugins/solver/sheetssolver.desktop PRE-CREATION 
>   sheets/sheets_viewplugin.desktop PRE-CREATION 
>   sheets/ui/MapViewModel.h ef3cf55 
>   sheets/ui/MapViewModel.cpp 070ee29 
>   stage/part/KPrPart.cpp b79da91 
>   words/part/CMakeLists.txt 4e2650c 
>   words/part/KWPart.cpp 9e3a818 
>   words/part/author/CAuPart.cpp 0863305 
> 
> Diff: http://git.reviewboard.kde.org/r/111545/diff/
> 
> 
> Testing
> -------
> 
> Ran all apps, loaded and saved documents and ran unittests.
> 
> 
> Thanks,
> 
> Boudewijn Rempt
> 
>

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

More information about the calligra-devel mailing list