On Monday, 2013-08-05, Jan Kundrát wrote: > - The fact that qobject_cast doesn't work across shared library boundaries > is suspicious. Wasn't supporting *that* use case one of the reasons > (besides not requiring RTTI) for introducing qobject_cast over > dynamic_cast?
Indeed. It is the way all Qt plugins work (styles, image formats, etc)
> > - Why are TrojitaPluginJob::start/stop postponing the actual
> > operation via the event loop? I'm not saying it's bad, but I'm
> > wondering why. A comment explaining this would be great.
>
> I haven't got any response about this (commit
> 2a70055482dd90a8834fc643e41c102aef720836).
Since that was my suggestion let me comment on that :)
It is more or less a safe guard. Some jobs will be implemented in a
synchronous way, e.g. the API they are using being sychronous.
If they do this in start() then their result will be available after start(),
which can easily lead to developers using the job assume that this is always
the case (they start to treat start() like a form of exec()).
If they are testing against an implementation that does, of course.
Such code will work flawlessly for them but not for others, increasing the
test matrix.
If all start() calls are asynchronous by default, nobody will fall into the
trap of assuming start() does anything immediatley useful.
Another option is to lead by example, i.e. make sure all default shipped
plugins that do not use asynchronous API do delay their actual work method in
start().
> - commit 546581201b79d721f6bcb22bc930eb4aaca2536f:
> > + if (job) {
> > + disconnect(job, 0, 0, 0);
> > + job->stop();
> > + job->deleteLater();
> > + }
>
> This is disconnecting everything connected to any signal of that job. Did
> you mean to disconnect(job, 0, this, 0)?
I think this is to protect against stop() emitting any signals. Probably
easier to call job->disconnect();
Or avoiding sigals in stop().
> > + AddressbookJob *job = static_cast<AddressbookJob *>(sender());
>
> Kevin suggested to pass Job* as an argument; that would save you a call to
> sender() here.
True, though this is closer to Qt APIs like QNetworkReply.
My suggestion was more grounded in the idea that all jobs (actually the base
class) would have a single result signal (like KJob does).
I guess it is mostly a matter of style and I am biased there due to long
exposure to KJob based APIs :)
> - WITH_KDE cannot supported with Qt5.
Not yet :)
But yeah, that will be a while :)
Cheers,
Kevin
--
Kevin Krammer, KDE developer, xdg-utils developer
KDE user support, developer mentoring
signature.asc
Description: This is a digitally signed message part.
