[Kde-pim] Review Request 112635: GID migration.
Dan Vrátil
dvratil at redhat.com
Tue Sep 10 19:50:53 BST 2013
> On Sept. 10, 2013, 7:19 p.m., Dan Vrátil wrote:
> > migration/gid/gidmigrationjob.cpp, line 62
> > <http://git.reviewboard.kde.org/r/112635/diff/1/?file=188580#file188580line62>
> >
> > 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.
> >
>
> Christian Mollekopf wrote:
> I don't see why a modify job would take up significantly more space than the item with the full payload. The modifyjob could even be smart and drop the payload once it realizes it's ignored anyways and the GID is extracted. Are you sure this is an issue?
See http://commits.kde.org/kdepim-runtime/53f531d54d0e408e94c699068d8f9f462f5778f9 - there was a similar problem with piling Create/Modify jobs in Maildir - it was growing up to 1.5G or memory usage with 70k-large collections.
> On Sept. 10, 2013, 7:19 p.m., Dan Vrátil wrote:
> > migration/gid/gidmigrationjob.cpp, line 112
> > <http://git.reviewboard.kde.org/r/112635/diff/1/?file=188580#file188580line112>
> >
> > 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()
>
> Christian Mollekopf wrote:
> I think KCompositeJob takes already care of that, no?
> See KCompositeJob::slotResult.
Cool, didn't know that!
But still add at least a comment that "Error is handler automatically by parent job" or something
> On Sept. 10, 2013, 7:19 p.m., Dan Vrátil wrote:
> > migration/gid/gidmigrator.cpp, line 36
> > <http://git.reviewboard.kde.org/r/112635/diff/1/?file=188582#file188582line36>
> >
> > I don't know whether showing users mimetype is a good idea.
> >
> > Also, %1 is the first argument :)
>
> Christian Mollekopf wrote:
> Well, the idea is that we will have a GidMigrator instantiated for each mimetype because that's how extractors are added. Because also the state is saved per migrator I figured this would be a useful granularity.
>
> Soooo, the mimetype seems like a useful information for distinguishing between multiple gidmigrators ;-)
>
> Note that I'm really not sure if any of that information should be exposed to the user.
> I use that information in the control interface of the agent, but I rather see this as a tool for advanced users, respectively for cases where manual intervention is required. (So we could arguably drop i18n from those).
>
> From a user perspective not much more information than "Akonadi is doing some background processing" should be leaking out (IMO).
Once it's user-facing, it has to be translated :-) (or so I'm told)
Ok, if you are not going to shove it to users face in every PIM app, then that's probably fine :)
> On Sept. 10, 2013, 7:19 p.m., Dan Vrátil wrote:
> > migration/migratorbase.h, line 32
> > <http://git.reviewboard.kde.org/r/112635/diff/1/?file=188587#file188587line32>
> >
> > I'm not sure I understand point of this class...?
>
> Christian Mollekopf wrote:
> It implements the NullObject pattern. The point is that I don't have to if (configIsAvailable()) { accessConfig() } throughout the code, but I can simply access the config and if non is available simply nothing happens.
>
> This is mostly used for testing the stuff because it easily allows to inject a null config which persists nothing.
Ok, thanks for explanation
> On Sept. 10, 2013, 7:19 p.m., Dan Vrátil wrote:
> > migration/migratorbase.cpp, line 219
> > <http://git.reviewboard.kde.org/r/112635/diff/1/?file=188588#file188588line219>
> >
> > Why not just store static_cast<int>(mMigrationState) ?
>
> Christian Mollekopf wrote:
> Maybe it's just me but I prefer to keep the semantics of each value tied to the enum and neither it's int value nor it's spelling (QMetaEnum or whatnot). The translation layer allows to keep working with the enum (reordering inserting new items, renaming the enums), without breaking existing config files or wreaking havoc when loading one. So I deliberately did not use it's int value.
Ok, fine :)
- Dan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/112635/#review39733
-----------------------------------------------------------
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/
More information about the kde-pim
mailing list