Review Request 120890: Supporting changes for Calligra Gemini
Friedrich W. H. Kossebau
kossebau at kde.org
Thu Oct 30 19:45:04 GMT 2014
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120890/#review69542
-----------------------------------------------------------
And another round of rather code-style level review. Given no QML experience, I better keep it also like that ;)
Still have not tested this patch, should be able to do so tomorrow evening.
Did not mention before, all in all things look clean, it's easy to read, so has been more a pleasure to review. But hey, I expected that from you :P
CalligraProducts.cmake
<https://git.reviewboard.kde.org/r/120890/#comment48677>
Hm, PLUGIN_QTQUICK defined after being referenced before by APP_GEMINI is not supported (no lazy id resolving implemeneted yet). So this would need to move before the definition of APP_GEMINI.
Not sure, but is this not more kind of a part then a plugin? So I would list PLUGIN_QTQUICK in the section "# parts" and perhaps also rename it to PART_QTQUICK.
CalligraProducts.cmake
<https://git.reviewboard.kde.org/r/120890/#comment48688>
This actually wants to require APP_GEMINI instead of those two parts (which would be pulled in automatically via APP_GEMINI via PLUGIN_QTQUICK).
So the productset GEMINI requires APP_GEMINI and optionally includes all those plugins and filters, if buildable:
```
calligra_define_productset(GEMINI "Calligra for 2:1 devices"
REQUIRES
APP_GEMINI
OPTIONAL
# plugins
...
# filters
...
)
```
cmake/modules/FindLibgit2.cmake
<https://git.reviewboard.kde.org/r/120890/#comment48690>
Misses copyright and license statement
cmake/modules/FindLibqgit2.cmake
<https://git.reviewboard.kde.org/r/120890/#comment48692>
Misses copyright and license statement
filters/words/msword-odf/wv2/src/utilities.cpp
<https://git.reviewboard.kde.org/r/120890/#comment48695>
Why toLocal8Bit()? I would have expected toLatin1()?
libs/main/KoDocumentSectionDelegate.cpp
<https://git.reviewboard.kde.org/r/120890/#comment48696>
I guess future code readers might need a hint why this flag is needed. Also not immediately obvious to me already now.
When can it happen that the same delegate gets called in paint() while it already has been called there and is only executing it?
libs/main/KoTemplate.h
<https://git.reviewboard.kde.org/r/120890/#comment48697>
Thanks, that should help everyone to create new Calligra-style templates, with variants :)
libs/main/KoTemplateGroup.cpp
<https://git.reviewboard.kde.org/r/120890/#comment48698>
merge with previous line to `} else {`
libs/main/KoView.cpp
<https://git.reviewboard.kde.org/r/120890/#comment48699>
Please also add a comment why multiple KoMainWindow can nest each other, surprising to the innocent code reader, e.g. me :)
libs/main/gemini/ViewModeSwitchEvent.h
<https://git.reviewboard.kde.org/r/120890/#comment48700>
This line intended? If so, "m" might not be the best symbol name here...
And also needs a comment, to avoid further "hu?" faces...
qtquick/CMakeLists.txt
<https://git.reviewboard.kde.org/r/120890/#comment48701>
Is this include needed?
qtquick/CQCanvasController.h
<https://git.reviewboard.kde.org/r/120890/#comment48702>
whitespace!
qtquick/CQCanvasController.cpp
<https://git.reviewboard.kde.org/r/120890/#comment48703>
More whitespace here and below. You like red (at least on reviewboard)? :)
qtquick/CQCanvasControllerItem.h
<https://git.reviewboard.kde.org/r/120890/#comment48704>
whitespace
qtquick/CQImageProvider.cpp
<https://git.reviewboard.kde.org/r/120890/#comment48705>
This then also needs to be
```
const char CQImageProvider::identificationString[] = "cqimage";
```
as char[] is an own type (so I learned just some weeks ago).
qtquick/CQImageProvider.cpp
<https://git.reviewboard.kde.org/r/120890/#comment48706>
Should this not be `*size = QSize();`?
Currently this just leaks an QSize object, as `size` itself is only a pointer var inside this method.
Also wondering why `size` is passed as pointer and not reference. Similar code with pointers checks before if the pointer != 0. But not sure if this is not required by the interface with this method, need to find that first.
qtquick/CQLinkArea.h
<https://git.reviewboard.kde.org/r/120890/#comment48707>
-> CQLINKAREA_H for consistency
qtquick/CQSpreadsheetCanvas.cpp
<https://git.reviewboard.kde.org/r/120890/#comment48708>
con_s_t
qtquick/CQTextDocumentCanvas.h
<https://git.reviewboard.kde.org/r/120890/#comment48709>
Q_SIGNALS for consistency
qtquick/CQTextDocumentCanvas.h
<https://git.reviewboard.kde.org/r/120890/#comment48710>
Q_SLOTS for consistency
qtquick/CQTextDocumentCanvas.cpp
<https://git.reviewboard.kde.org/r/120890/#comment48712>
const QString&
qtquick/CQTextDocumentCanvas.cpp
<https://git.reviewboard.kde.org/r/120890/#comment48713>
merge with line before to
``` if (factory) {```
qtquick/CQTextDocumentCanvas.cpp
<https://git.reviewboard.kde.org/r/120890/#comment48711>
const ref for all parameters, please
qtquick/CQTextDocumentCanvas.cpp
<https://git.reviewboard.kde.org/r/120890/#comment48714>
merge with line before to
```
if (factory) {
```
qtquick/CQTextDocumentModel.h
<https://git.reviewboard.kde.org/r/120890/#comment48715>
Q_SIGNALS
qtquick/CQTextDocumentNotesModel.h
<https://git.reviewboard.kde.org/r/120890/#comment48716>
Private* _const_ d;
qtquick/CQTextToCModel.h
<https://git.reviewboard.kde.org/r/120890/#comment48717>
explicit
qtquick/CQTextToCModel.h
<https://git.reviewboard.kde.org/r/120890/#comment48718>
Private* _const_ d;
qtquick/CQTextToCModel.cpp
<https://git.reviewboard.kde.org/r/120890/#comment48719>
move bracket to end of last line
words/part/dockers/KWStatisticsWidget.cpp
<https://git.reviewboard.kde.org/r/120890/#comment48723>
whitespace :)
words/part/frames/KWCopyShape.h
<https://git.reviewboard.kde.org/r/120890/#comment48722>
Can be reverted as well.
- Friedrich W. H. Kossebau
On Okt. 30, 2014, 2:54 nachm., Dan Leinir Turthra Jensen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/120890/
> -----------------------------------------------------------
>
> (Updated Okt. 30, 2014, 2:54 nachm.)
>
>
> Review request for Calligra.
>
>
> Repository: calligra
>
>
> Description
> -------
>
> The many changes in this patch are all the ones found in the Calligra Gemini branch. It includes the addition of the Qt Quick compinents created a while ago by Arjen and Shantanu, as well as a number of changes which improves the writing performance and experience in Words, especially when dealing with very large documents. It further extends the template system with support for variations.
>
> It does not include the gemini subdirectory itself (and as such to compile this you would need to check out the branch itself, rather than apply this patch), the reason being the large amount of data found in this subdirectory took the patch from the current 233KiB to a nice, solid 55MiB. So, i thought this would be more useful.
>
>
> Diffs
> -----
>
> stage/templates/common_desktop/emptyLandscapeWide.desktop c5f8f8a
> stage/templates/odf/CMakeLists.txt f7db7d0
> stage/templates/odf/blue_orange_vector.desktop 31dae09
> stage/templates/odf/blue_orange_vector.png PRE-CREATION
> stage/templates/odf/burning_desire.desktop ac79022
> stage/templates/odf/burning_desire.png PRE-CREATION
> stage/templates/odf/business.desktop d552db3
> stage/templates/odf/business.png PRE-CREATION
> stage/templates/odf/cohere-blue.desktop PRE-CREATION
> stage/templates/odf/cohere-blue.otp PRE-CREATION
> stage/templates/odf/cohere-blue.png PRE-CREATION
> stage/templates/odf/cohere-gold.desktop PRE-CREATION
> stage/templates/odf/cohere-gold.otp PRE-CREATION
> stage/templates/odf/cohere-gold.png PRE-CREATION
> stage/templates/odf/cohere-magenta.desktop PRE-CREATION
> stage/templates/odf/cohere-magenta.otp PRE-CREATION
> stage/templates/odf/cohere-magenta.png PRE-CREATION
> stage/part/animations/KPrAnimateColor.cpp 59a5e0e
> stage/part/animations/KPrAnimateTransform.cpp 914047e
> qtquick/CQTextToCModel.h PRE-CREATION
> qtquick/CQTextToCModel.cpp PRE-CREATION
> qtquick/CQThumbnailItem.h PRE-CREATION
> qtquick/CQThumbnailItem.cpp PRE-CREATION
> qtquick/CalligraQtQuickComponentsPlugin.h PRE-CREATION
> qtquick/CalligraQtQuickComponentsPlugin.cpp PRE-CREATION
> qtquick/qmldir PRE-CREATION
> stage/part/KPrView.h b4bab24
> stage/part/KPrView.cpp 1edd8e8
> stage/part/animations/KPrAnimTransitionFilter.cpp 3347fe1
> qtquick/CQTextDocumentModel.cpp PRE-CREATION
> qtquick/CQTextDocumentNotesModel.h PRE-CREATION
> qtquick/CQTextDocumentNotesModel.cpp PRE-CREATION
> qtquick/CQCanvasBase.h PRE-CREATION
> qtquick/CQCanvasBase.cpp PRE-CREATION
> qtquick/CQCanvasController.h PRE-CREATION
> qtquick/CQCanvasController.cpp PRE-CREATION
> qtquick/CQCanvasControllerItem.h PRE-CREATION
> qtquick/CQCanvasControllerItem.cpp PRE-CREATION
> qtquick/CQImageProvider.h PRE-CREATION
> qtquick/CQImageProvider.cpp PRE-CREATION
> qtquick/CQLinkArea.h PRE-CREATION
> qtquick/CQLinkArea.cpp PRE-CREATION
> qtquick/CQPluginLoaderProxy.h PRE-CREATION
> qtquick/CQPluginLoaderProxy.cpp PRE-CREATION
> qtquick/CQPresentationCanvas.h PRE-CREATION
> qtquick/CQPresentationCanvas.cpp PRE-CREATION
> qtquick/CQPresentationModel.h PRE-CREATION
> qtquick/CQPresentationModel.cpp PRE-CREATION
> qtquick/CQPresentationView.h PRE-CREATION
> qtquick/CQPresentationView.cpp PRE-CREATION
> qtquick/CQSpreadsheetCanvas.h PRE-CREATION
> qtquick/CQSpreadsheetCanvas.cpp PRE-CREATION
> qtquick/CQSpreadsheetListModel.h PRE-CREATION
> qtquick/CQSpreadsheetListModel.cpp PRE-CREATION
> qtquick/CQTextDocumentCanvas.h PRE-CREATION
> qtquick/CQTextDocumentCanvas.cpp PRE-CREATION
> qtquick/CQTextDocumentModel.h PRE-CREATION
> filters/words/docx/export/DocxFile.cpp d67e41f
> filters/words/msword-odf/wv2/src/utilities.cpp 4be8c38
> krita/sketch/KisSketchView.cpp 5cb9172
> krita/sketch/SketchDeclarativeView.cpp e352fc9
> libs/flake/KoCanvasController.h 804a1e4
> libs/flake/KoCanvasController.cpp 5190f62
> libs/flake/KoToolManager.h ac17dd0
> libs/flake/KoToolManager.cpp 5d4da91
> libs/kopageapp/KoPADocumentModel.cpp 22bd75a
> libs/kopageapp/tools/backgroundTool/KoPABackgroundToolFactory.cpp f442b84
> libs/main/KoApplication.cpp b20c0e9
> libs/main/KoDocument.h c3936a9
> libs/main/KoDocumentSectionDelegate.cpp 81c53c0
> libs/main/KoMainWindow.cpp 8db43f8
> libs/main/KoPart.cpp 04b8d25
> libs/main/KoTemplate.h f96099c
> libs/main/KoTemplate.cpp f1d3bd9
> libs/main/KoTemplateCreateDia.cpp 72573cc
> libs/main/KoTemplateGroup.cpp ab35dd1
> libs/main/KoTemplateTree.cpp 8f864b4
> libs/main/KoView.cpp 4770dcc
> libs/main/gemini/ViewModeSwitchEvent.h PRE-CREATION
> libs/odf/KoEncryptedStore.cpp 53c75b6
> libs/widgets/KoZoomAction.h c5f69b0
> plugins/defaultTools/defaulttool/SelectionDecorator.h 83e715e
> plugins/defaultTools/defaulttool/SelectionDecorator.cpp 3e16cfa
> qtquick/CMakeLists.txt PRE-CREATION
> words/templates/Wordprocessing/professional.png PRE-CREATION
> words/templates/Wordprocessing/FaxA4.desktop 2b43998
> words/templates/Wordprocessing/FaxLetter.desktop fc1c161
> words/templates/Wordprocessing/ProfessionalA4.desktop 7c84f34
> words/templates/Wordprocessing/ProfessionalLetter.desktop 5f8b94f
> words/templates/Wordprocessing/a4.png PRE-CREATION
> words/templates/Wordprocessing/colorful.png PRE-CREATION
> words/templates/Wordprocessing/fax.png PRE-CREATION
> stage/templates/odf/cohere-red.desktop PRE-CREATION
> stage/templates/odf/cohere-red.otp PRE-CREATION
> stage/templates/odf/cohere-red.png PRE-CREATION
> stage/templates/odf/curious_penguin.desktop 429e7c9
> stage/templates/odf/curious_penguin.png PRE-CREATION
> stage/templates/odf/flood_light.desktop 23adbb2
> stage/templates/odf/flood_light.png PRE-CREATION
> stage/templates/odf/hisc-action-cohere-blue.svgz PRE-CREATION
> stage/templates/odf/hisc-action-cohere-gold.svgz PRE-CREATION
> stage/templates/odf/hisc-action-cohere-magenta.svgz PRE-CREATION
> stage/templates/odf/hisc-action-cohere-red.svgz PRE-CREATION
> stage/templates/odf/kde_events.desktop afa4534
> stage/templates/odf/kde_events.png PRE-CREATION
> stage/templates/odf/rounded_square.desktop 85015a2
> stage/templates/odf/rounded_square.png PRE-CREATION
> stage/templates/odf/simple_waves.desktop dd2bd96
> stage/templates/odf/simple_waves.png PRE-CREATION
> stage/templates/odf/skyline_monotone.desktop 0c9dd68
> stage/templates/odf/skyline_monotone.png PRE-CREATION
> stage/templates/odf/skyline_monotone_wide.desktop 152c719
> stage/templates/odf/skyline_monotone_wide.png PRE-CREATION
> stage/templates/odf/skyline_morning.desktop ec4140d
> stage/templates/odf/skyline_morning.png PRE-CREATION
> stage/templates/odf/skyline_morning_wide.desktop d3337ae
> stage/templates/odf/skyline_morning_wide.png PRE-CREATION
> stage/templates/odf/skyline_night.desktop 28e3a18
> stage/templates/odf/skyline_night.png PRE-CREATION
> stage/templates/odf/skyline_night_wide.desktop e188171
> stage/templates/odf/skyline_night_wide.png PRE-CREATION
> stage/templates/odf/strange_far_hills.desktop 1a17807
> stage/templates/odf/strange_far_hills.png PRE-CREATION
> words/part/KWCanvasBase.cpp 0d2d28f
> words/part/KWCanvasItem.cpp cfdae98
> words/part/KWGui.cpp 65aedf7
> words/part/KWPageStyle.h a3bf58b
> words/part/KWView.h f21f8e3
> words/part/KWView.cpp 1285d21
> words/part/commands/KWShapeCreateCommand.h eb268f8
> words/part/dockers/KWNavigationWidget.cpp 1bb54e0
> words/part/dockers/KWStatisticsWidget.h eac4d44
> words/part/dockers/KWStatisticsWidget.cpp 63d0384
> words/part/frames/KWCopyShape.h 01eedf5
> words/part/frames/KWCopyShape.cpp 744f292
> words/part/pagetool/KWPageTool.cpp 7935390
> words/templates/Wordprocessing/A4.desktop 8a35eaf
> words/templates/Wordprocessing/CMakeLists.txt 88b73fa
> words/templates/Wordprocessing/ColorfulA4.desktop 04caf8e
> words/templates/Wordprocessing/ColorfulLetter.desktop ce38ab5
> CMakeLists.txt fa00fdd
> CalligraProducts.cmake c2ef8ac
> active/main.cpp 7e4c888
> active/qml/Test.qml PRE-CREATION
> active/src/CASpreadsheetHandler.cpp e8cebff
> active/src/CATextDocumentModel.cpp 4e72581
> active/src/MainWindow.cpp c378881
> cmake/modules/FindLibgit2.cmake PRE-CREATION
> cmake/modules/FindLibqgit2.cmake PRE-CREATION
>
> Diff: https://git.reviewboard.kde.org/r/120890/diff/
>
>
> Testing
> -------
>
> * Compiles and shipped to users on Windows (through the Calligra Gemini packages)
> * Used every day for writing in a production environment (for producing my novel and recent short story)
>
>
> Thanks,
>
> Dan Leinir Turthra Jensen
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/calligra-devel/attachments/20141030/1135a44d/attachment.htm>
More information about the calligra-devel
mailing list