Review Request: KoAbstraction Library, iteration #3

Inge Wallin inge at lysator.liu.se
Fri Jan 7 22:45:13 GMT 2011 

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


I understand that this patch is the minimum of what is required to create mobile interfaces outside of the Calligra source tree, at least if you want to keep the interfaces sane.  

Fine.  This is a great idea, and I am very positive to it.  In the long run, I want a much bigger change where the current applications are split up into an engine part and a desktop UI part. At the same time, I'm sure that many things could be moved to a common UI part, like commands and actions. But that is a project for longer term, so I will only comment on this patch now.

In general I think it's fine. I can't comment on the changes of the source code that you did inside foffice, Mani has to comment on those.  I was asked by Jaroslaw to comment on it even though I don't know all the details of FreOffice.

My only negative comment is that I don't like the name "KoAbstraction": it is far too general.  Abstraction of what?  I would be happier if it could be changed to KoUiAbstraction, or maybe even something even more specific (KoMobileUiAbstraction?).  Other than that, I think it's a good thing, and recommend it to be applied.  However, I won't press "Ship it" because I think somebody with more detailed knowledge should look at it too.

- Inge


On 2011-01-05 00:22:18, Jarosław Staniek wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100304/
> -----------------------------------------------------------
> 
> (Updated 2011-01-05 00:22:18)
> 
> 
> Review request for Calligra.
> 
> 
> Summary
> -------
> 
> KoAbstraction Library
> 
> This is the third iteration, the previous (http://reviewboard.kde.org/r/5635/) was discarded because works on this one were already quite advanced.
> 
> More discussion at http://community.kde.org/Calligra/Libs/KoAbstraction
> 
> == Here's a copy from http://reviewboard.kde.org/r/5635/:
> 
> This is library utilizing facade design pattern in order to simplify implementation of custom graphical interfaces for various applications. This way standalone KOffice viewer widget can be delivered too.
> 
> * added koabstraction in tools/koabstraction/
> * it's dependent on KSpread so for my understanding does not fit to libs/
> * moved code that belong to koabstraction from f-office/, that is:
> ** KoAbstractApplication intermediate class
> ** KoAbstractApplicationController
> ** FoCellTool (renamed to KoCellTool)
> ** FoCellToolFactory (renamed to KoCellToolFactory, made private class)
> ** RemoveSheet (renamed to RemoveSheetCommand for more clarity, made private class)
> * FoCellEditor temporarily moved to koabstraction/ as private class, will be back here as soon as interface abstracting cell editor is ready
> * finished simplifying f-office/CMakeLists.txt (it looked like standalone project before with using find_package(Qt4 4.5.0 REQUIRED), etc.)
> * KoAbstractApplicationImpl.cpp stays in tools/f-office/ as it's a wrapper for building KoAbstractApplication.moc properly
> * added KoExternalEditorInterface class which is implemented by FoExternalEditor in f-office, abstraction uses it instead of concrete implementation of editor
> * FoImageSelectionWidget is not instatiated at all so constructor was removed
> * PanTool_ID, TextTool_ID, CellTool_ID constants from f-office replaced by KoAbstractApplicationController::panToolFactoryId(), etc. in koabstraction, so the tools can be freely accessed in any implementation
> * KoAbstractApplicationController uses controller() which is now KoController only; in contexts when KoControllerWidget is needed, appropriate dynamic_case is defined as KoAbstractApplicationController::controllerWidget(); this leads to API that could be usable for graphics-view-based UIs
> * PresentationTool is back in f-office's MainWindow now, same for StoreButtonPreview; code related to them is moved from abstraction to f-office
> 
> == Changes compared to http://reviewboard.kde.org/r/5635/:
> 
> * ported to Calligra: renamed namespaces, libs and paths; full namespaces used everywhere for improved readability
> * as requested by maemo devs, f-office/ now builds using non-kde4 buildsystem (no automoc)
> * KoAbstractApplicationOpenDocumentArguments added, provided setting on/off editing as default mode, implementation-independent way for multiple documents support (e.g. using multiple instances), and opening document as template
> * editing mode is now a flag of the controller, is set/unset automatically when needed by the abstraction, can be also set by the GUIs
> * added KoAbstractApplication::openDocumentAsTemplate()
> * temporarily, MS documents are saved as ODF counterparts because of mixed quality of filters
> * pointer optional to a splash screen moved to abstraction, displayed/closed when necessary
> * KoAbstractApplicationController:
> ** added supportedExtensions() and added supportedFilters() for improved code reuse
> ** added documentFileName() that can be used by GUIs
> ** added setHorizontalScrollBarVisible(bool), setVerticalScrollBarVisible(bool) abstractions
> ** added setOnlyDisplayDocumentNameInTitle(bool): if set to true only document name is displayed and not application name; This is to conserve space on small displays
> 
> Summing up, these are modified paths:
> 
> #       modified:   CMakeLists.txt
> #       modified:   tools/CMakeLists.txt
> #       modified:   tools/f-office/CMakeLists.txt
> #       modified:   tools/f-office/Common.h
> #       modified:   tools/f-office/FoExternalEditor.cpp
> #       modified:   tools/f-office/FoExternalEditor.h
> #       modified:   tools/f-office/FoImageSelectionWidget.cpp
> #       modified:   tools/f-office/FoImageSelectionWidget.h
> #       modified:   tools/f-office/KoAbstractApplicationImpl.cpp
> #       modified:   tools/f-office/Main.cpp
> #       modified:   tools/f-office/MainWindow.cpp
> #       modified:   tools/f-office/MainWindow.h
> #       modified:   tools/f-office/NotifyDialog.cpp
> #       modified:   tools/f-office/NotifyDialog.h
> #       new file:   tools/koabstraction/CMakeLists.txt
> #       renamed:    tools/f-office/FoCellEditor.cpp -> tools/koabstraction/FoCellEditor.cpp
> #       renamed:    tools/f-office/FoCellEditor.h -> tools/koabstraction/FoCellEditor.h
> #       renamed:    tools/f-office/KoAbstractApplication.h -> tools/koabstraction/KoAbstractApplication.h
> #       renamed:    tools/f-office/KoAbstractApplicationController.cpp -> tools/koabstraction/KoAbstractApplicationController.cpp
> #       renamed:    tools/f-office/KoAbstractApplicationController.h -> tools/koabstraction/KoAbstractApplicationController.h
> #       renamed:    tools/f-office/KoAbstractApplicationImpl.h -> tools/koabstraction/KoAbstractApplicationImpl.h
> #       renamed:    tools/f-office/FoCellTool.cpp -> tools/koabstraction/KoCellTool.cpp
> #       renamed:    tools/f-office/FoCellTool.h -> tools/koabstraction/KoCellTool.h
> #       renamed:    tools/f-office/FoCellToolFactory.cpp -> tools/koabstraction/KoCellToolFactory.cpp
> #       renamed:    tools/f-office/FoCellToolFactory.h -> tools/koabstraction/KoCellToolFactory.h
> #       new file:   tools/koabstraction/KoExternalEditorInterface.h
> #       renamed:    tools/f-office/RemoveSheet.cpp -> tools/koabstraction/RemoveSheetCommand.cpp
> #       renamed:    tools/f-office/RemoveSheet.h -> tools/koabstraction/RemoveSheetCommand.h
> #       new file:   tools/koabstraction/koabstraction_export.h
> 
> 
> Diffs
> -----
> 
>   CMakeLists.txt d58e152 
>   tools/CMakeLists.txt a10cd08 
>   tools/f-office/CMakeLists.txt cd2c6bd 
>   tools/f-office/Common.h 30b31f6 
>   tools/f-office/FoCellEditor.h c50fcbd 
>   tools/f-office/FoCellEditor.cpp 13038d4 
>   tools/f-office/FoCellTool.h 3128050 
>   tools/f-office/FoCellTool.cpp 63ce11f 
>   tools/f-office/FoCellToolFactory.h d29de43 
>   tools/f-office/FoCellToolFactory.cpp da4339b 
>   tools/f-office/FoExternalEditor.h cab8a87 
>   tools/f-office/FoExternalEditor.cpp bfc232f 
>   tools/f-office/FoImageSelectionWidget.h dddc61b 
>   tools/f-office/FoImageSelectionWidget.cpp 0da2880 
>   tools/f-office/KoAbstractApplication.h 47c7f08 
>   tools/f-office/KoAbstractApplicationController.h 7293b4e 
>   tools/f-office/KoAbstractApplicationController.cpp d3aa2bd 
>   tools/f-office/KoAbstractApplicationImpl.h 469430e 
>   tools/f-office/KoAbstractApplicationImpl.cpp 01014f2 
>   tools/f-office/Main.cpp ca88305 
>   tools/f-office/MainWindow.h 937a16a 
>   tools/f-office/MainWindow.cpp 3110056 
>   tools/f-office/NotifyDialog.h c51fee7 
>   tools/f-office/NotifyDialog.cpp e9ecab2 
>   tools/f-office/RemoveSheet.h d44351c 
>   tools/f-office/RemoveSheet.cpp 9f617b9 
>   tools/koabstraction/CMakeLists.txt PRE-CREATION 
>   tools/koabstraction/FoCellEditor.h PRE-CREATION 
>   tools/koabstraction/FoCellEditor.cpp PRE-CREATION 
>   tools/koabstraction/KoAbstractApplication.h PRE-CREATION 
>   tools/koabstraction/KoAbstractApplicationController.h PRE-CREATION 
>   tools/koabstraction/KoAbstractApplicationController.cpp PRE-CREATION 
>   tools/koabstraction/KoAbstractApplicationImpl.h PRE-CREATION 
>   tools/koabstraction/KoCellTool.h PRE-CREATION 
>   tools/koabstraction/KoCellTool.cpp PRE-CREATION 
>   tools/koabstraction/KoCellToolFactory.h PRE-CREATION 
>   tools/koabstraction/KoCellToolFactory.cpp PRE-CREATION 
>   tools/koabstraction/KoExternalEditorInterface.h PRE-CREATION 
>   tools/koabstraction/RemoveSheetCommand.h PRE-CREATION 
>   tools/koabstraction/RemoveSheetCommand.cpp PRE-CREATION 
>   tools/koabstraction/koabstraction_export.h PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/100304/diff
> 
> 
> Testing
> -------
> 
> FreOffice builds and runs properly for any of the three document types. Did some testing on standalone minimal test application which implements the interfaces - documents are opened and displayed properly.
> 
> 
> Thanks,
> 
> Jarosław
> 
>

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

More information about the calligra-devel mailing list