[Kde-pim] Review Request 112635: GID migration.

Tue Sep 10 18:19:08 BST 2013

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

migration/gid/gidmigrationjob.h
<http://git.reviewboard.kde.org/r/112635/#comment29263>

    4.12, unless you can mastered timetravel :)

migration/gid/gidmigrationjob.h
<http://git.reviewboard.kde.org/r/112635/#comment29279>

    const&

migration/gid/gidmigrationjob.cpp
<http://git.reviewboard.kde.org/r/112635/#comment29264>

    You can also set setFetchRemoteIdentification( false ) to avoid fetching RID and remoteRevision.

migration/gid/gidmigrationjob.cpp
<http://git.reviewboard.kde.org/r/112635/#comment29265>

    You should fix it instead of workarounding it :P

migration/gid/gidmigrationjob.cpp
<http://git.reviewboard.kde.org/r/112635/#comment29275>

    I'm afraid this will pile tons of ItemModifyJobs and execute them one by one only after the fetch job finishes.

    Maybe you could just store the items in a list and after the FetchJob finishes, you could process them one by one, like you do in GidMigrationJob::processCollection()? It should not have a big performance impact, but it will prevent the application consuming too much memory.

migration/gid/gidmigrationjob.cpp
<http://git.reviewboard.kde.org/r/112635/#comment29266>

    When an error occurs, the GidMigrationJob will hang forever. There should be an else branch that re-emits the error from UpdateJob to this job and calls emitResult()

migration/gid/gidmigrator.cpp
<http://git.reviewboard.kde.org/r/112635/#comment29276>

    QLatin1String()

migration/gid/gidmigrator.cpp
<http://git.reviewboard.kde.org/r/112635/#comment29267>

    I don't know whether showing users mimetype is a good idea.

    Also, %1 is the first argument :)

migration/gid/gidmigrator.cpp
<http://git.reviewboard.kde.org/r/112635/#comment29277>

    %1

migration/gid/main.cpp
<http://git.reviewboard.kde.org/r/112635/#comment29271>

    This should probably be set via options.add("+mimetype", ik18n(...));

migration/gid/main.cpp
<http://git.reviewboard.kde.org/r/112635/#comment29272>

    I think you should connect migrator's finished() signal to app's quit() slot, otherwise it will never finish?

migration/infodialog.cpp
<http://git.reviewboard.kde.org/r/112635/#comment29251>

    You are missing break after each statement

migration/migratorbase.h
<http://git.reviewboard.kde.org/r/112635/#comment29273>

    I'm not sure I understand point of this class...?

migration/migratorbase.h
<http://git.reviewboard.kde.org/r/112635/#comment29274>

    Having KMigrationBase and MigrationBase is quite confusing...so maybe KDataMigrationBase, since this one is oriented on migrating data instead of resources? (just a suggestion, not an issue) 

migration/migratorbase.h
<http://git.reviewboard.kde.org/r/112635/#comment29269>

    Should we even show to users something like this? I mean all they need to care about is "oh, it's updating my stuff so that it's more awesome" - showing some technobabble will just confuse and scare them :)

migration/migratorbase.cpp
<http://git.reviewboard.kde.org/r/112635/#comment29252>

    QLatin1String()

migration/migratorbase.cpp
<http://git.reviewboard.kde.org/r/112635/#comment29253>

    QLatin1String()s

migration/migratorbase.cpp
<http://git.reviewboard.kde.org/r/112635/#comment29308>

    It should be documented that start() is blocking (or it should be made non-blocking, so that it's easier to run the Migration in a different thread)

migration/migratorbase.cpp
<http://git.reviewboard.kde.org/r/112635/#comment29254>

    The error should be translated if it will be shown to users.

migration/migratorbase.cpp
<http://git.reviewboard.kde.org/r/112635/#comment29257>

    Maybe just call logMessage(Info, displayName() + " started"); ?

migration/migratorbase.cpp
<http://git.reviewboard.kde.org/r/112635/#comment29255>

    i18n()

migration/migratorbase.cpp
<http://git.reviewboard.kde.org/r/112635/#comment29258>

    i18n()

migration/migratorbase.cpp
<http://git.reviewboard.kde.org/r/112635/#comment29259>

    i18n()

migration/migratorbase.cpp
<http://git.reviewboard.kde.org/r/112635/#comment29260>

    i18n()

migration/migratorbase.cpp
<http://git.reviewboard.kde.org/r/112635/#comment29261>

    Why not just store static_cast<int>(mMigrationState) ?

migration/migratorbase.cpp
<http://git.reviewboard.kde.org/r/112635/#comment29262>

    i18n()

One more thing: could you please fix the coding style? :) You are mixing styles even in your own code, the biggest inconsistency are spaces inside parentheses. Also please remove the whitespaces.

- Dan Vrátil

On Sept. 10, 2013, 3:08 p.m., Christian Mollekopf wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112635/
> -----------------------------------------------------------
> 
> (Updated Sept. 10, 2013, 3:08 p.m.)
> 
> 
> Review request for KDEPIM.
> 
> 
> Description
> -------
> 
> GID migration.
> 
> A new MigratorBase as interface for migrators.
> 
> I didn't adapt the existing KMigratorBase as it taylored towards one migrator per resource and I found a couple of other incompatibilities so I figured I rather make something clean from scratch.
> 
> It should be easy enough to adapt KMigratorBase to be based on top MigratorBase if we still need that code.
> 
> 
> Diffs
> -----
> 
>   migration/CMakeLists.txt 3d15b97ab49a092e56416dce1c88725bcca3695e 
>   migration/gid/CMakeLists.txt PRE-CREATION 
>   migration/gid/gidmigrationjob.h PRE-CREATION 
>   migration/gid/gidmigrationjob.cpp PRE-CREATION 
>   migration/gid/gidmigrator.h PRE-CREATION 
>   migration/gid/gidmigrator.cpp PRE-CREATION 
>   migration/gid/main.cpp PRE-CREATION 
>   migration/infodialog.h 0003c48d3eab4260e4f0cc6a3ebf17ac762a1e1f 
>   migration/infodialog.cpp 6f08db23922c181b98d809d77b98564f7b6a3ca8 
>   migration/kmigratorbase.h 3a57917f0d0655320147ac13ef9544975c0188d0 
>   migration/migratorbase.h PRE-CREATION 
>   migration/migratorbase.cpp PRE-CREATION 
>   migration/tests/CMakeLists.txt a9129a6bd033867345a668c2fdda0c3c0adae7d6 
>   migration/tests/testgidmigration.h PRE-CREATION 
>   migration/tests/testgidmigration.cpp PRE-CREATION 
>   migration/tests/testmigratorbase.cpp PRE-CREATION 
>   migration/tests/unittestenv/config.xml fa0eafe88d573518ad72269f1b8aafb7f93c885a 
>   migration/tests/unittestenv/kdehome/share/config/akonadi_knut_resource_0rc PRE-CREATION 
>   migration/tests/unittestenv/kdehome/share/config/kdebugrc 32317f7453f4c0a0ec778bd494ced8e7d65078f2 
>   migration/tests/unittestenv/kdehome/testdata-res1.xml PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/112635/diff/
> 
> 
> Testing
> -------
> 
> 
> 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/