[Kde-pim] Review Request 112636: Migration agent that schedules various migrators and exposes an interface for status and control.

Tue Sep 10 18:17:01 BST 2013

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


agents/migration/migrationagent.cpp
<http://git.reviewboard.kde.org/r/112636/#comment29282>

    I would move that and the <migration> include above the KDE includes.
    recommended ordering is local to system, and KDE core/gui headers are more "system" than kdepimlib headers


agents/migration/migrationagent.cpp
<http://git.reviewboard.kde.org/r/112636/#comment29283>

    space after :


agents/migration/migrationagent.cpp
<http://git.reviewboard.kde.org/r/112636/#comment29284>

    space after keyword


agents/migration/migrationagent.desktop
<http://git.reviewboard.kde.org/r/112636/#comment29281>

    might need a better icon :)


agents/migration/migrationagent.h
<http://git.reviewboard.kde.org/r/112636/#comment29278>

    I think you can remove the Akonadi:: qualifier, you are inside the Akonadi namespace scope here


agents/migration/migrationagent.h
<http://git.reviewboard.kde.org/r/112636/#comment29280>

    I think we use & at the argument name, i.e. &id


agents/migration/migrationscheduler.h
<http://git.reviewboard.kde.org/r/112636/#comment29286>

    no QtCore, you are not using it for the other headers either


agents/migration/migrationscheduler.h
<http://git.reviewboard.kde.org/r/112636/#comment29285>

    see above


agents/migration/migrationscheduler.h
<http://git.reviewboard.kde.org/r/112636/#comment29287>

    trailing whitespace


agents/migration/migrationscheduler.h
<http://git.reviewboard.kde.org/r/112636/#comment29288>

    &parent


agents/migration/migrationscheduler.h
<http://git.reviewboard.kde.org/r/112636/#comment29289>

    &index


agents/migration/migrationscheduler.h
<http://git.reviewboard.kde.org/r/112636/#comment29290>

    &child


agents/migration/migrationscheduler.h
<http://git.reviewboard.kde.org/r/112636/#comment29292>

    *parent


agents/migration/migrationscheduler.cpp
<http://git.reviewboard.kde.org/r/112636/#comment29294>

    IMHO it is nicer to have the argument and use Q_UNUSED if it is not needed.
    

agents/migration/migrationscheduler.cpp
<http://git.reviewboard.kde.org/r/112636/#comment29296>

    such short strings should alway be used with i18nc() variants to provide extra context for the translators
    See http://techbase.kde.org/Development/Tutorials/Localization/i18n#Standard_Context_For_Common_Phrases
    and
    http://techbase.kde.org/Development/Tutorials/Localization/i18n_Semantics


agents/migration/migrationscheduler.cpp
<http://git.reviewboard.kde.org/r/112636/#comment29297>

    &identifier


agents/migration/migrationscheduler.cpp
<http://git.reviewboard.kde.org/r/112636/#comment29298>

    &identifier


agents/migration/migrationscheduler.cpp
<http://git.reviewboard.kde.org/r/112636/#comment29299>

    &identifer


agents/migration/migrationscheduler.cpp
<http://git.reviewboard.kde.org/r/112636/#comment29300>

    &identifier


agents/migration/migrationscheduler.cpp
<http://git.reviewboard.kde.org/r/112636/#comment29301>

    &identifier


agents/migration/migrationstatuswidget.h
<http://git.reviewboard.kde.org/r/112636/#comment29304>

    before Qt includes


agents/migration/migrationstatuswidget.h
<http://git.reviewboard.kde.org/r/112636/#comment29302>

    *parent


agents/migration/migrationstatuswidget.cpp
<http://git.reviewboard.kde.org/r/112636/#comment29305>

    *parent


agents/migration/migrationstatuswidget.cpp
<http://git.reviewboard.kde.org/r/112636/#comment29306>

    wouldn't it also be possible to just get the current selection model directyl from the view?


agents/migration/tests/schedulertest.cpp
<http://git.reviewboard.kde.org/r/112636/#comment29307>

    &identifier, *parent


- Kevin Krammer


On Sept. 10, 2013, 1:17 p.m., Christian Mollekopf wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112636/
> -----------------------------------------------------------
> 
> (Updated Sept. 10, 2013, 1:17 p.m.)
> 
> 
> Review request for KDEPIM.
> 
> 
> Description
> -------
> 
> Migration agent that schedules various migrators and exposes an interface for status and control.
> 
> This does not yet include a plugin interface for making plugging in new migrators plugin based. That would be easy enough to add but only makes sense if we push the MigratorBase interface to kdepimlibs along with a dbus control interface, so i.e. applications such as kmail can install new migration plugins and control them via the dbus interface.
> 
> The agent automatically starts migration jobs that have shouldAutostart() enabled and exposes an interface in it's configuration dialog to start/stop migrators. The interface is yet very basic and is also no necessarily in the most convenient place. We should likely have some central place for controlling background processes or simply ensure the automatic processing is handled well enough.
> 
> Ideas how to do that best are appreciated.
> 
> 
> Diffs
> -----
> 
>   agents/CMakeLists.txt 22b6db73687f82c9d05a19c63708be32b6508513 
>   agents/migration/CMakeLists.txt PRE-CREATION 
>   agents/migration/migrationagent.cpp PRE-CREATION 
>   agents/migration/migrationagent.desktop PRE-CREATION 
>   agents/migration/migrationagent.h PRE-CREATION 
>   agents/migration/migrationscheduler.h PRE-CREATION 
>   agents/migration/migrationscheduler.cpp PRE-CREATION 
>   agents/migration/migrationstatuswidget.h PRE-CREATION 
>   agents/migration/migrationstatuswidget.cpp PRE-CREATION 
>   agents/migration/tests/CMakeLists.txt PRE-CREATION 
>   agents/migration/tests/schedulertest.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/112636/diff/
> 
> 
> Testing
> -------
> 
> I tried the GID migration for contacts, which worked fine for me.
> 
> 
> Thanks,
> 
> Christian Mollekopf
> 
>

_______________________________________________
KDE PIM mailing list kde-pim at kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/