Review Request 110541: Synchronized tag categories and their views for resource choosers, KoResourceFiltering refactored a bit.

Sun May 26 15:02:56 BST 2013

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

A little bit of code style feedback, did not look at the logic though, left to others or me later.

krita/ui/kis_palette_manager.cpp
<http://git.reviewboard.kde.org/r/110541/#comment24540>

    no STL-style code, please use append().

libs/widgets/KoResourceFiltering.cpp
<http://git.reviewboard.kde.org/r/110541/#comment24517>

    guess this slipped your eyes, or is there a reason? otherwise
    const QString -> const QString &

libs/widgets/KoResourceItemChooser.cpp
<http://git.reviewboard.kde.org/r/110541/#comment24537>

    You might want to use tags to highlight certain terms in the text, like the example search terms. See for a translator-friendly way here:
    http://techbase.kde.org/Development/Tutorials/Localization/i18n_Semantics#Semantic_Tags

libs/widgets/KoResourceItemChooser.cpp
<http://git.reviewboard.kde.org/r/110541/#comment24539>

    Not sure if "Tag" should be upper-case. I would tend to write it "tag" everywhere.

libs/widgets/KoResourceItemChooser.cpp
<http://git.reviewboard.kde.org/r/110541/#comment24538>

    thus e.g. "save" -> "<interface>Save</interface>"

libs/widgets/KoResourceItemChooser.cpp
<http://git.reviewboard.kde.org/r/110541/#comment24536>

    For consistency please do not use the STL-style variants of the methods, but the CamelCase Qt-style ones. So prepend() here.

libs/widgets/KoResourceItemChooser.cpp
<http://git.reviewboard.kde.org/r/110541/#comment24534>

    No need to call setText here then, as the button has an empty text per default.

    And in general use QString() instead of "" (mini-code/runtime optimization).

libs/widgets/KoResourceItemChooser.cpp
<http://git.reviewboard.kde.org/r/110541/#comment24535>

    As above

libs/widgets/KoResourceItemChooser.cpp
<http://git.reviewboard.kde.org/r/110541/#comment24533>

    Usual Code style is:
    no spaces next to the brackets, spaces between arguments.
    See http://techbase.kde.org/Policies/Kdelibs_Coding_Style which we try to follow in Calligra. Exception is e.g. the Qt Includes, where we leave the module name out, in preparation for the Qt5 move.

    Some old code does not yet follow that style, but any codelines that are touched/added should be adapted/following it.

libs/widgets/KoResourceItemChooser.cpp
<http://git.reviewboard.kde.org/r/110541/#comment24532>

    This tooltip could be much more descriptive :)

libs/widgets/KoResourceItemChooser.cpp
<http://git.reviewboard.kde.org/r/110541/#comment24530>

    Accidental white space changes here (possibly tab vs. whitespaces)?
    Please revert.

libs/widgets/KoResourceItemChooser.cpp
<http://git.reviewboard.kde.org/r/110541/#comment24531>

    click messages seem to be usually done without a closing "." so please remove that here.
    Too bad the KDE UI style guide does not mention something about click messages so I could refer to something here :/ 

libs/widgets/KoResourceItemChooser.cpp
<http://git.reviewboard.kde.org/r/110541/#comment24528>

    List the argument in the i18n call, like
    i18n("Remove from \"%1\".", curTag).
    That way the translators see what %1 is.

    Though in this case this might not help that much, given that curTag is not really descriptive. You might want to add a comment to this string, using i18nc instead, like this
    i18nc("remove this item from the group with the tag curTag", "Remove from \"%1\".", curTag) or similar

libs/widgets/KoResourceItemChooser.cpp
<http://git.reviewboard.kde.org/r/110541/#comment24529>

    Remove the ":" at the end of the text string, not needed/usually done

libs/widgets/KoResourceItemChooser.cpp
<http://git.reviewboard.kde.org/r/110541/#comment24525>

    QString() instead of ""
    And one space between the arguments, i.e. behind the ,
    You might want to list the arguments in two lines, as the single line gets too wide
    QString tagName could also be const QString tagName

libs/widgets/KoResourceItemChooser.cpp
<http://git.reviewboard.kde.org/r/110541/#comment24526>

    one space between the arguments
    arg1, arg2

libs/widgets/KoResourceItemChooser.cpp
<http://git.reviewboard.kde.org/r/110541/#comment24524>

    QString() instead of ""

libs/widgets/KoResourceModel.h
<http://git.reviewboard.kde.org/r/110541/#comment24523>

    const QString &tag

libs/widgets/KoResourceServerAdapter.h
<http://git.reviewboard.kde.org/r/110541/#comment24522>

    const QStringList &filteredNames

libs/widgets/KoResourceServerAdapter.h
<http://git.reviewboard.kde.org/r/110541/#comment24520>

    const QString &tag

libs/widgets/KoResourceServerAdapter.h
<http://git.reviewboard.kde.org/r/110541/#comment24521>

    const QString &tag

libs/widgets/KoResourceServerAdapter.cpp
<http://git.reviewboard.kde.org/r/110541/#comment24518>

    const QString &tag

libs/widgets/KoResourceServerAdapter.cpp
<http://git.reviewboard.kde.org/r/110541/#comment24519>

    const QString &tag

- Friedrich W. H. Kossebau

On May 26, 2013, 9:57 a.m., Sascha Suelzer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/110541/
> -----------------------------------------------------------
> 
> (Updated May 26, 2013, 9:57 a.m.)
> 
> 
> Review request for Calligra.
> 
> 
> Description
> -------
> 
> With this patch, the tag categories the user can define for various resources will update for any resource chooser of the same type.
> Examples: 
> Defining a new tag in the preset popup combobox will also create the respective tag entry in the preset docker one.
> If both choosers would display the same tag category, and the user adds a resource to the category in one chooser, the view of the other would reflect the change as well.
> 
> Both these things are achieved with a new set of signals and slots regarding tag category creation and modification.
> 
> Other changes:
> Refactored KoResourceFiltering a bit to make intent more clear and functionality a bit more fine grained, also fixed the naming for its private support class members from m_foo to foo since the fields are all public.
> Tag categories are now ordered in an alphabetical manner at all times.
> 
> --------------------------------
> 
> Remarks:
> 
> I am not really sure if KoResourceModel should be the class for the signals:
> signals:
>     void tagBoxEntryModified();
>     void tagBoxEntryAdded(QString tag);
>     void tagBoxEntryRemoved(QString tag);
> 
> It seems to me that perhaps all of this tagging and filtering stuff should go into the adapter directly, perhaps? But The KoResourceItemChoosers would still require the notifications and the model is pretty much the only bridge, so maybe the place is okay after all.
> 
> I also want to refactor some of the new code I introduced to make it look nicer, but I wanted to get this review request out for now so it is know that it is being worked on, since this builds on the former shortcomings of 
> https://git.reviewboard.kde.org/r/110429/
> 
> 
> Diffs
> -----
> 
>   krita/plugins/paintops/libbrush/kis_brush_server.cpp 5e4db8d 
>   krita/ui/kis_palette_manager.cpp 568859a 
>   krita/ui/ko_favorite_resource_manager.h fbfa86f 
>   krita/ui/ko_favorite_resource_manager.cpp 2481ce1 
>   krita/ui/widgets/kis_preset_chooser.h 5df3b56 
>   krita/ui/widgets/kis_preset_chooser.cpp 9f07c8a 
>   libs/widgets/KoResourceFiltering.h 9f8f967 
>   libs/widgets/KoResourceFiltering.cpp a62a873 
>   libs/widgets/KoResourceItemChooser.h d26f5a5 
>   libs/widgets/KoResourceItemChooser.cpp a2750b5 
>   libs/widgets/KoResourceModel.h 3ccb28b 
>   libs/widgets/KoResourceModel.cpp a9cb212 
>   libs/widgets/KoResourceServer.h 1ab4ad6 
>   libs/widgets/KoResourceServerAdapter.h 02ab31e 
>   libs/widgets/KoResourceServerAdapter.cpp f1dc50f 
>   libs/widgets/KoResourceServerObserver.h 52bc7fc 
>   libs/widgets/KoResourceTagging.h 0624096 
>   libs/widgets/KoResourceTagging.cpp b678000 
> 
> Diff: http://git.reviewboard.kde.org/r/110541/diff/
> 
> 
> Testing
> -------
> 
> Only tested for Krita, everything seems to work as it should.
> 
> 
> Thanks,
> 
> Sascha Suelzer
> 
>

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