D8379: PDF: Support the new poppler renderToImage with update callback

[Milian Wolff]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/okular-devel/attachments/20171101/0612b688/attachment-0001.html>