mwolff added inline comments.
INLINE COMMENTS
> generator.cpp:407
> +{
> + request->page()->setPixmap( request->observer(), new QPixmap(
> QPixmap::fromImage( image ) ), request->normalizedRect() );
> +
is `setPixmap` old API? Why create a QPixmap on the heap? That should not be
done, pass values instead.
> generator.h:581
> + * This method can be called to trigger a partial pixmap update for
> the given request
> + * Make sure you call it in a way it's executed in the main thread.
> + * @since 1.3
aka: "Must be called from the main thread."
> generator_pdf.cpp:919
> +
> + if (payload->timer.isActive() && payload->timer.remainingTime() == 0) {
> + payload->timer.stop();
why is the timer active when there is no remaining time? when does this happen?
can you add a comment please?
> generator_pdf.cpp:929
> + auto payload = static_cast<PartialUpdatePayload *>(payloadA);
> + QMetaObject::invokeMethod(payload->generator,
> "signalPartialPixmapRequest", Qt::QueuedConnection,
> Q_ARG(Okular::PixmapRequest*, payload->request), Q_ARG(QImage, image));
> +}
sending raw pointers over a queued connection sounds scary - who owns the
pixmap request? will it really outlive everything? could you instead use a
shared pointer or similar value type to ensure it really never gets destroyed
in the middle?
REPOSITORY
R223 Okular
REVISION DETAIL
https://phabricator.kde.org/D8379
To: aacid, #okular, mlaurent
Cc: mwolff, rkflx, ngraham, michaelweghorn, mlaurent, #okular, aacid
