Review Request 122910: Introduce KMoreTools

Sun Mar 15 10:31:06 UTC 2015

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

I don't think that the patch in it's current form is ok to commit.

It contains lots of comments and code parts that are marked as todo, and there are quite some criterias that I'd consider not yet good enough.

But then, I also think that the idea is quite good and worth to follow, so we'll eventually reach a state where we can give a ship-it.

Of course, other comments and opinions are welcome.

src/kmoretools/kmoretools.h
<https://git.reviewboard.kde.org/r/122910/#comment53231>

    Proper doxygen would look like this: The first sentence is always the "brief" documentation.
    Currently, your brief documentation says "Introduction". This is certainly not what you want.

    More correct is:
    /**
     * Easy access to more tools.
     * 
     * @section kmt_intro Introduction
     * 
     * KMoreTools helps to you provide easy access to external applications ...

    It's also better to start with a true text, and not immediately with a list after a single word.

    If you want, you can have a look at e.g. ktexteditor/src/include/Document.h for how you can use doxygen to comment the code.

    Also AFAIK, it is still recomended to wrap the code and comments at latest 80 characters.

src/kmoretools/kmoretools.h
<https://git.reviewboard.kde.org/r/122910/#comment53230>

    In doxygen, correct is:
    /**
     * A service described in a .desktop file which is
     * -# either found on the system (isInstalled() == true)
     * -# or if not, provided in the kmt-desktopfiles location (isInstalled() == false)
     */

    Or you can also use 1. and 2., but not 1) and 2).

    See: http://www.stack.nl/~dimitri/doxygen/manual/lists.html

src/kmoretools/kmoretools.h
<https://git.reviewboard.kde.org/r/122910/#comment53229>

    missing API documentation.

src/kmoretools/kmoretools.h
<https://git.reviewboard.kde.org/r/122910/#comment53228>

    missing API documentation.

src/kmoretools/kmoretools.h
<https://git.reviewboard.kde.org/r/122910/#comment53227>

    Plese never return this in setters. This is considered as bad API in the Qt/KDE world.

    It's much more readable to have:
    service->setText(...);
    service->setIcon(...);
    service->setEnabled(true);

    instead of
    service->setText(...)->setIcon(...)->setEnabled(true);

    Never chain statements.

    Among bad readability, it also makes debugging a lot (!) harder.

src/kmoretools/kmoretools.h
<https://git.reviewboard.kde.org/r/122910/#comment53226>

    @return, never @returns.

    Further, can you be mor verbose?

    @return Returns the tools service ... .... The returned pointer may be a nullptr.

src/kmoretools/kmoretools.h
<https://git.reviewboard.kde.org/r/122910/#comment53225>

    No colos. Correct is:
    @param someParameter This is some bla parameter.

src/kmoretools/kmoretools.h
<https://git.reviewboard.kde.org/r/122910/#comment53224>

    @param is used to list the paramters. What you want here is '@p', not @param.

src/kmoretools/kmoretools.h
<https://git.reviewboard.kde.org/r/122910/#comment53232>

    Correct is:
    @note Please note that action() returns a nullptr if the requested service is not installed.

src/kmoretools/kmoretools.h
<https://git.reviewboard.kde.org/r/122910/#comment53223>

    Do we need the kmt-prefix? Why not just
    KMoreToolsService * service();

src/kmoretools/kmoretools.h
<https://git.reviewboard.kde.org/r/122910/#comment53222>

    Qt API never returns a this pointer when settings some properties.

    While not explicitely stated here, these links may be useful:
    - http://qt-project.org/wiki/API-Design-Principles
    - http://kate-editor.org/2011/08/28/coding-style-and-api-design/

    For me, this function should be:
    void setInitialItemText(const QString & itemText);

src/kmoretools/kmoretools.h
<https://git.reviewboard.kde.org/r/122910/#comment53233>

    Too many todo's in the code, commented out code and also the comments.

    So I consider this as work in progress, right?

I

- Dominik Haumann

On March 11, 2015, 11:06 p.m., Gregor Mi wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/122910/
> -----------------------------------------------------------
> 
> (Updated March 11, 2015, 11:06 p.m.)
> 
> 
> Review request for KDE Frameworks, Emmanuel Pescosta and Jeremy Whiting.
> 
> 
> Repository: knewstuff
> 
> 
> Description
> -------
> 
> Moved from kservice (https://git.reviewboard.kde.org/r/122576/) to here (knewstuff).
> 
> Example usages:
> - dolphin's SpaceInfoToolsMenu: https://git.reviewboard.kde.org/r/122911/
> - kate's project addin: https://git.reviewboard.kde.org/r/122374/
> 
> 
> Diffs
> -----
> 
>   src/CMakeLists.txt 3b9a403eb473e0c3760506b2757c71d603b99775 
>   src/kmoretools/kmoretools.h PRE-CREATION 
>   src/kmoretools/kmoretools.cpp PRE-CREATION 
>   src/kmoretools/kmoretools_p.h PRE-CREATION 
>   src/kmoretools/kmoretoolsconfigdialog_p.h PRE-CREATION 
>   src/kmoretools/kmoretoolsconfigdialog_p.cpp PRE-CREATION 
>   src/kmoretools/ui/kmoretoolsconfigwidget.ui PRE-CREATION 
>   tests/CMakeLists.txt 103c5fc98deaf83288b843cc66a87f2d95ad9dfb 
>   tests/kmoretools/1/a.desktop PRE-CREATION 
>   tests/kmoretools/1/b.desktop PRE-CREATION 
>   tests/kmoretools/1/c.desktop PRE-CREATION 
>   tests/kmoretools/2/kate.desktop PRE-CREATION 
>   tests/kmoretools/2/kate.png PRE-CREATION 
>   tests/kmoretools/2/mynotinstalledapp.desktop PRE-CREATION 
>   tests/kmoretools/2/mynotinstalledapp.png PRE-CREATION 
>   tests/kmoretools/kmoretoolstest.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/122910/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Gregor Mi
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20150315/44fc1993/attachment-0001.html>