Review Request 111545: Port komain away from KParts

Boudewijn Rempt boud at valdyas.org
Wed Aug 14 07:22:21 BST 2013 


> On Aug. 13, 2013, 10:36 p.m., Friedrich W. H. Kossebau wrote:
> > Only had time for the first 3 pages tonight. Will do the rest tomorrow.
> > 
> > (And grr, the shell->mainWindow rename should have really been a separate step. Next time please, please think of your reviewers if you want more quality reviews and happy reviewers. It really complicates the review unneededly of code that is already complex :( )

Yeah, that's why it's all different commits in the branch. In any case, what I really wanted to know is: can you confirm the issue you had in plan is now fixed?


> On Aug. 13, 2013, 10:36 p.m., Friedrich W. H. Kossebau wrote:
> > libs/main/KoDocument.h, line 260
> > <http://git.reviewboard.kde.org/r/111545/diff/4/?file=178696#file178696line260>
> >
> >     UI stuff in KoDocument? Not so perfect.
> >     Can this be somehow delegated to the kopart instance?

Not, it's not perfect. But it's better than it used to be and KoPart is also the wrong place for gui stuff. This will need to be cleaned up in later patches, but as Inge said, it doesn't seem a good idea to wait with merging until the patch is really unmanagably big.


> On Aug. 13, 2013, 10:36 p.m., Friedrich W. H. Kossebau wrote:
> > libs/main/KoDocument.h, line 749
> > <http://git.reviewboard.kde.org/r/111545/diff/4/?file=178696#file178696line749>
> >
> >     QWidget in the KoDocument API :/
> >     
> >     Can this functionality be handled by the view somehow, instead of the doc?
> >     
> >     At least add a big TODO for now.

Here, I don't really believe in adding big TODO's -- not after having removed several that came from 1999 or so...


> On Aug. 13, 2013, 10:36 p.m., Friedrich W. H. Kossebau wrote:
> > libs/main/KoDocument.cpp, lines 356-360
> > <http://git.reviewboard.kde.org/r/111545/diff/4/?file=178697#file178697line356>
> >
> >     Meh. UI/QWidget in the KoDocument.
> >     
> >     What about a signal that is catched by the KoPart, which then does what is needed to present this info to the user?

KoPart is the wrong place. With the move to Qt5, KIO will also be split, and that will make it a lot easier to actually properly split out all the QWidget stuff. It's not something I want to fix in this iteration of the refactoring.


- Boudewijn


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


On Aug. 13, 2013, 8:17 a.m., Boudewijn Rempt wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111545/
> -----------------------------------------------------------
> 
> (Updated Aug. 13, 2013, 8:17 a.m.)
> 
> 
> Review request for Calligra, Friedrich W. H. Kossebau and Marijn Kruisselbrink.
> 
> 
> 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
> -----
> 
>   CMakeLists.txt 06e4971 
>   active/src/CATextDocumentHandler.cpp 609832f 
>   braindump/plugins/quickstates/CMakeLists.txt ecadaea 
>   braindump/src/CMakeLists.txt 7bfe50b 
>   cmake/modules/FindCalligraLibs.cmake.in 1e49d17 
>   flow/part/FlowPart.cpp 5fedf83 
>   karbon/data/CMakeLists.txt 5183c06 
>   karbon/data/karbon_viewplugin.desktop PRE-CREATION 
>   karbon/main.cpp b341d27 
>   karbon/plugins/flattenpath/CMakeLists.txt 60bfe96 
>   karbon/plugins/flattenpath/FlattenPathPlugin.cpp 936ddcc 
>   karbon/plugins/flattenpath/karbonflattenpathplugin.desktop PRE-CREATION 
>   karbon/plugins/refinepath/CMakeLists.txt 6d3de3d 
>   karbon/plugins/refinepath/RefinePathPlugin.cpp 2e375cf 
>   karbon/plugins/refinepath/karbonrefinepathplugin.desktop PRE-CREATION 
>   karbon/plugins/roundcorners/CMakeLists.txt f22cd94 
>   karbon/plugins/roundcorners/RoundCornersPlugin.cpp 9892ce9 
>   karbon/plugins/roundcorners/karbonroundcornersplugin.desktop PRE-CREATION 
>   karbon/plugins/whirlpinch/CMakeLists.txt accac4e 
>   karbon/plugins/whirlpinch/WhirlPinchPlugin.cpp 4a50720 
>   karbon/plugins/whirlpinch/karbonwhirlpinchplugin.desktop PRE-CREATION 
>   karbon/ui/KarbonDocument.cpp da78714 
>   karbon/ui/KarbonPart.cpp ab6774e 
>   karbon/ui/KarbonView.cpp d84b6da 
>   krita/plugins/assistants/RulerAssistant/CMakeLists.txt 957fb4f 
>   krita/plugins/extensions/dockers/flipbook/flipbookdocker_dock.cpp 49cf62a 
>   krita/sdk/tests/ui_manager_test.h 63d8397 
>   krita/ui/actions/kis_selection_action_factories.cpp bf49d0a 
>   krita/ui/kis_flipbook.cpp 918df92 
>   krita/ui/kis_part2.cpp 83d5f34 
>   krita/ui/kis_view2.cpp 2deab8b 
>   krita/ui/tests/kis_action_manager_test.cpp f4df430 
>   krita/ui/tests/kis_doc2_test.cpp d86e20a 
>   krita/ui/tests/kis_zoom_and_pan_test.cpp 794c1ae 
>   krita/ui/widgets/kis_workspace_chooser.cpp 0100372 
>   libs/kokross/KoScriptingDocker.cpp d2b7e16 
>   libs/kokross/KoScriptingModule.h 4d57e94 
>   libs/kokross/KoScriptingModule.cpp 304d547 
>   libs/kokross/KoScriptingPart.cpp 68054ac 
>   libs/kopageapp/KoPAView.h 80b9f78 
>   libs/kopageapp/KoPAView.cpp bb88ee4 
>   libs/kopageapp/tests/CMakeLists.txt 0d44ee3 
>   libs/main/CMakeLists.txt 3a9110c 
>   libs/main/KoApplication.h d973820 
>   libs/main/KoApplication.cpp 7421107 
>   libs/main/KoApplicationAdaptor.h d71356f 
>   libs/main/KoDocument.h 3372060 
>   libs/main/KoDocument.cpp 622678f 
>   libs/main/KoDocumentEntry.cpp abbb55b 
>   libs/main/KoDocument_p.h 6f3931f 
>   libs/main/KoFilterEntry.cpp 8e23eac 
>   libs/main/KoMainWindow.h 1b3503f 
>   libs/main/KoMainWindow.cpp 26327aa 
>   libs/main/KoPart.h d4ecf5f 
>   libs/main/KoPart.cpp 56e27b5 
>   libs/main/KoPartAdaptor.h b7eb540 
>   libs/main/KoPartAdaptor.cpp 58acde5 
>   libs/main/KoStartupDialog.h PRE-CREATION 
>   libs/main/KoStartupDialog.cpp PRE-CREATION 
>   libs/main/KoVersionDialog.cpp 2f38510 
>   libs/main/KoView.h 7b8d943 
>   libs/main/KoView.cpp 1b4a722 
>   libs/pigment/KoColorSpaceMaths.h a517b2f 
>   libs/rdf/CMakeLists.txt bd29ead 
>   plan/kptmaindocument.cpp 26cd92b 
>   plan/kptpart.cpp 7674480 
>   plan/kptview.h 02acb77 
>   plan/kptview.cpp 64d0c68 
>   plan/libs/ui/kptviewbase.h 9c4022a 
>   plan/libs/ui/kptviewbase.cpp 7049a76 
>   plan/workpackage/CMakeLists.txt a1fbb87 
>   plugins/chartshape/ChartShape.cpp 34a3db9 
>   plugins/colorengines/lcms2/CMakeLists.txt faa07a9 
>   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 a74ecd5 
>   sheets/part/Part.cpp 7fd573e 
>   sheets/part/View.h 49bcdf6 
>   sheets/part/View.cpp 9dae4a5 
>   sheets/plugins/scripting/CMakeLists.txt 90a0b24 
>   sheets/ui/MapViewModel.h ef3cf55 
>   sheets/ui/MapViewModel.cpp 070ee29 
>   stage/part/KPrDocument.cpp 7bdd5f9 
>   stage/part/KPrPart.cpp b79da91 
>   stage/part/KPrView.cpp fd6b762 
>   words/part/CMakeLists.txt 4e2650c 
>   words/part/KWDocument.h 7654aa3 
>   words/part/KWDocument.cpp 259fd4e 
>   words/part/KWGui.cpp 26c5281 
>   words/part/KWPart.h 63e4e5f 
>   words/part/KWPart.cpp 9e3a818 
>   words/part/KWView.cpp 48265fd 
>   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/20130814/ed63a984/attachment.htm>

More information about the calligra-devel mailing list