Review Request 120890: Supporting changes for Calligra Gemini

Friedrich W. H. Kossebau kossebau at kde.org
Wed Oct 29 18:49:18 GMT 2014 

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


Here some code nitpicks from some quick code browsing.
In general:
* please qDebug/qWarning -> kDebug/kWarning
* more {} brackets for one-liners :)
* no trailing whitespaces
* const(ref) as const(ref) can :)

As noted inside ViewModeSwitchEvent.h needs another location, or the event handling changed. Right now this patch cannot be tested.


active/src/MainWindow.cpp
<https://git.reviewboard.kde.org/r/120890/#comment48600>

    That wants to be something configurable, no? :)



cmake/productsets/words+stage.cmake
<https://git.reviewboard.kde.org/r/120890/#comment48603>

    All these plugins are already pulled in by the WORDS and STAGE productsets, cmp. their deps in CalligraProducts.cmake.
    
    Actually I propose to rename the GEMINI product to APP_GEMINI (which describes just the executable and what it needs to run/build at all) and have another GEMINI productset in CalligraProducts.cmake, by the example of e.g. the productsets WORDS and STAGE. So in the end drop this file.



cmake/productsets/words+stage.cmake
<https://git.reviewboard.kde.org/r/120890/#comment48602>

    Listing PLUGIN_QTQUICK is not needed, as GEMINI has that as dependency, so will be automatically built.



cmake/productsets/words+stage.cmake
<https://git.reviewboard.kde.org/r/120890/#comment48601>

    Better move GEMINI to # apps section, to not be missed out here.



libs/flake/KoToolManager.h
<https://git.reviewboard.kde.org/r/120890/#comment48604>

    Please add a comment why attemptCanvasControllerRemoval() is needed next to removeCanvasController() and how these two should be used together.



libs/main/KoTemplate.h
<https://git.reviewboard.kde.org/r/120890/#comment48605>

    color/swatch/variantName and custom thumbnail might need some explanation how they are supposed to work, as it is at least not immediate obvious. I guess for each template there can be variants in both(?) color and swatch, which also needs a custom thumbnail.
    
    Best there would be some explanation in the API dox of the KoTemplate class how such variants of a template need to be installed filewise.



libs/odf/KoEncryptedStore.cpp
<https://git.reviewboard.kde.org/r/120890/#comment48606>

    Yes, something I have a WIP branch on my harddisk where KoStore classes no longer directly use KMessageBox etc., but use setable delegates (so a shutup-delegate just drops any messages). Need to get this done in time for 2.9



qtquick/CQCanvasBase.cpp
<https://git.reviewboard.kde.org/r/120890/#comment48607>

    whitespace :) also some more below



qtquick/CQCanvasController.h
<https://git.reviewboard.kde.org/r/120890/#comment48610>

    QSize and QPoint only need to be forward declared here.



qtquick/CQCanvasController.cpp
<https://git.reviewboard.kde.org/r/120890/#comment48611>

    For consistency include path without QtCore module part



qtquick/CQCanvasController.cpp
<https://git.reviewboard.kde.org/r/120890/#comment48609>

    Q_UNUSED for all unused parameters in this file, for less noisy  build.



qtquick/CQCanvasController.cpp
<https://git.reviewboard.kde.org/r/120890/#comment48608>

    make const



qtquick/CQCanvasControllerItem.cpp
<https://git.reviewboard.kde.org/r/120890/#comment48612>

    whitespace



qtquick/CQCanvasControllerItem.cpp
<https://git.reviewboard.kde.org/r/120890/#comment48614>

    there is no space between "if" and "(" all over this file at least. ideally there would be some.



qtquick/CQCanvasControllerItem.cpp
<https://git.reviewboard.kde.org/r/120890/#comment48613>

    indentation. could also get some braces around.



qtquick/CQImageProvider.h
<https://git.reviewboard.kde.org/r/120890/#comment48615>

    no module in path for consistency



qtquick/CQImageProvider.h
<https://git.reviewboard.kde.org/r/120890/#comment48616>

    -> static const char identificationString[], spares the redirection from pointer address to actual data address (mini improvement, but getting used to it is good ;) )



qtquick/CQImageProvider.cpp
<https://git.reviewboard.kde.org/r/120890/#comment48617>

    m_images.insert(id, image); for mini improvement



qtquick/CQLinkArea.h
<https://git.reviewboard.kde.org/r/120890/#comment48618>

    -> include <QDeclarativeItem> for consistency



qtquick/CQLinkArea.cpp
<https://git.reviewboard.kde.org/r/120890/#comment48619>

    license header missing



qtquick/CQLinkArea.cpp
<https://git.reviewboard.kde.org/r/120890/#comment48620>

    no module in path



qtquick/CQLinkArea.cpp
<https://git.reviewboard.kde.org/r/120890/#comment48621>

    one line: } else {



qtquick/CQPresentationCanvas.cpp
<https://git.reviewboard.kde.org/r/120890/#comment48622>

    Hm, should ViewModeSwitchEvent.h perhaps move to qtquick folder, to resolve this inverted dependency? Or rather be moved to some libs/gemini/ folder or libs/komain? Actually it needs to move out of gemini, as it is used by code (in the parts) that should not all need to link to Gemini stuff.
    Also means that this patch cannot be tested, as it is missing this file and more :)
    Perhaps instead of having the parts reacting to this event, some gemini products could install event filters to catch the event and handle themselves. That way the parts do not need to know about it.
    How does Krita solve this?



qtquick/CQPresentationCanvas.cpp
<https://git.reviewboard.kde.org/r/120890/#comment48623>

    pass by const ref



qtquick/CQPresentationModel.cpp
<https://git.reviewboard.kde.org/r/120890/#comment48624>

    make const



qtquick/CQPresentationModel.cpp
<https://git.reviewboard.kde.org/r/120890/#comment48631>

    whitespace



qtquick/CQSpreadsheetCanvas.cpp
<https://git.reviewboard.kde.org/r/120890/#comment48625>

    pass as const ref



qtquick/CQSpreadsheetListModel.cpp
<https://git.reviewboard.kde.org/r/120890/#comment48630>

    make const



qtquick/CQTextDocumentCanvas.h
<https://git.reviewboard.kde.org/r/120890/#comment48629>

    explicit



qtquick/CQTextDocumentCanvas.cpp
<https://git.reviewboard.kde.org/r/120890/#comment48628>

    pass as const ref



qtquick/CQTextDocumentCanvas.cpp
<https://git.reviewboard.kde.org/r/120890/#comment48627>

    pass as const ref



qtquick/CQTextDocumentCanvas.cpp
<https://git.reviewboard.kde.org/r/120890/#comment48626>

    pass as const ref



qtquick/CQTextDocumentNotesModel.h
<https://git.reviewboard.kde.org/r/120890/#comment48634>

    QModelIndex -> QAbstractListModel



qtquick/CQTextDocumentNotesModel.h
<https://git.reviewboard.kde.org/r/120890/#comment48632>

    explicit



qtquick/CQTextDocumentNotesModel.h
<https://git.reviewboard.kde.org/r/120890/#comment48633>

    pass QString as const ref



qtquick/CQTextDocumentNotesModel.cpp
<https://git.reviewboard.kde.org/r/120890/#comment48635>

    .insert(role, string) would be mini-opt here



qtquick/CQTextToCModel.cpp
<https://git.reviewboard.kde.org/r/120890/#comment48636>

    .insert again, for consistency if



qtquick/CQThumbnailItem.h
<https://git.reviewboard.kde.org/r/120890/#comment48637>

    -> <QDeclarativeItem>



qtquick/CalligraQtQuickComponentsPlugin.h
<https://git.reviewboard.kde.org/r/120890/#comment48638>

    no module in path



qtquick/CalligraQtQuickComponentsPlugin.h
<https://git.reviewboard.kde.org/r/120890/#comment48639>

    whitespace at end



words/part/dockers/KWNavigationWidget.cpp
<https://git.reviewboard.kde.org/r/120890/#comment48640>

    Please add proper {} brackets



words/part/frames/KWCopyShape.cpp
<https://git.reviewboard.kde.org/r/120890/#comment48641>

    Hm, I prefer the parameter names being written and marked with Q_UNUSED. Here both seems to be done? One should be enough.



words/part/pagetool/KWPageTool.cpp
<https://git.reviewboard.kde.org/r/120890/#comment48642>

    No need to remove the Q_UNUSED when the include is readded


- Friedrich W. H. Kossebau


On Okt. 29, 2014, 3:07 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. 29, 2014, 3:07 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
> -----
> 
>   words/templates/Wordprocessing/fax.png PRE-CREATION 
>   words/templates/Wordprocessing/professional.png PRE-CREATION 
>   words/part/pagetool/KWPageTool.h 45ad7d5 
>   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 
>   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 
>   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 
>   stage/templates/odf/skyline_night.desktop 28e3a18 
>   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/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/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 
>   qtquick/CQSpreadsheetListModel.cpp PRE-CREATION 
>   qtquick/CQTextDocumentCanvas.h PRE-CREATION 
>   qtquick/CQTextDocumentCanvas.cpp PRE-CREATION 
>   qtquick/CQTextDocumentModel.h PRE-CREATION 
>   qtquick/CQTextDocumentModel.cpp PRE-CREATION 
>   qtquick/CQTextDocumentNotesModel.h PRE-CREATION 
>   qtquick/CQTextDocumentNotesModel.cpp PRE-CREATION 
>   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 
>   stage/part/animations/KPrAnimateColor.cpp 59a5e0e 
>   stage/part/animations/KPrAnimateTransform.cpp 914047e 
>   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 
>   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 
>   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/odf/KoEncryptedStore.cpp 53c75b6 
>   libs/textlayout/KoTextDocumentLayout.cpp 805f11d 
>   libs/widgets/KoZoomAction.h c5f69b0 
>   plugins/defaultTools/defaulttool/SelectionDecorator.h 83e715e 
>   plugins/defaultTools/defaulttool/SelectionDecorator.cpp 3e16cfa 
>   plugins/textshape/TextShape.cpp dac4a90 
>   qtquick/CMakeLists.txt 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 
>   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 
>   cmake/modules/FindLibgit2.cmake PRE-CREATION 
>   cmake/modules/FindLibqgit2.cmake PRE-CREATION 
>   cmake/productsets/words+stage.cmake 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 
>   CMakeLists.txt fa00fdd 
>   CalligraProducts.cmake c2ef8ac 
>   active/qml/Test.qml PRE-CREATION 
>   active/src/MainWindow.cpp c378881 
> 
> 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/20141029/6ba11b17/attachment.htm>

More information about the calligra-devel mailing list