This revision was automatically updated to reflect the committed changes.
Closed by commit R223:a2f5560c0076: PDF: Support the poppler 0.62 renderToImage
with update callback (authored by aacid).
CHANGED PRIOR TO COMMIT
https://phabricator.kde.org/D8379?vs=21826=22125#toc
REPOSITORY
R223
aacid added a comment.
In https://phabricator.kde.org/D8379#164630, @rkflx wrote:
> In https://phabricator.kde.org/D8379#15, @ngraham wrote:
>
> > Can we add "BUG: 344081" to the Summary?
>
>
> It's still not showing up for me in the summary on Phab. Note you'll have
to use
mlaurent added a comment.
@rkflx void signalPartialPixmapRequest( Okular::PixmapRequest * request,
const QImage ); it seems that it's not fixed no ? still space no ?
But after fixing it it's ok for me
REPOSITORY
R223 Okular
REVISION DETAIL
https://phabricator.kde.org/D8379
To:
rkflx accepted this revision.
rkflx added a comment.
In https://phabricator.kde.org/D8379#15, @ngraham wrote:
> Can we add "BUG: 344081" to the Summary?
It's still not showing up for me in the summary on Phab. Note you'll have to
use something like `arc diff --edit
aacid updated this revision to Diff 21826.
aacid added a comment.
rebase
REPOSITORY
R223 Okular
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D8379?vs=21790=21826
BRANCH
updateCallback (branched from master)
REVISION DETAIL
https://phabricator.kde.org/D8379
AFFECTED FILES
rkflx added a comment.
In https://phabricator.kde.org/D8379#163666, @mwolff wrote:
> FWIW: I ran it here, and it worked fine after the crash fix. Only tested it
on the dublin map though.
Thanks, good to know :)
REPOSITORY
R223 Okular
REVISION DETAIL
mwolff added a comment.
In https://phabricator.kde.org/D8379#163631, @rkflx wrote:
> @aacid In general I notice the changes you are making, sorry I'm not always
able test immediately. I'll try to have another look in the next couple of days.
>
> (That said, more people testing by
rkflx added a comment.
@aacid In general I notice the changes you are making, sorry I'm not always
able test immediately. I'll try to have another look in the next couple of days.
(That said, more people testing by actually running the code with the newest
Poppler would be great.)
aacid added a comment.
@rkflx i rebased this patch on top of master that includes
https://cgit.kde.org/okular.git/commit/?id=662fa69a2dcc0c1403b1773262368943d5cacd52
This fixes for me the crashes on rotation jobs, the bug wasn't on this
branch, it's just that now that codepath was
aacid updated this revision to Diff 21790.
aacid added a comment.
rebased
REPOSITORY
R223 Okular
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D8379?vs=21756=21790
BRANCH
updateCallback (branched from master)
REVISION DETAIL
https://phabricator.kde.org/D8379
AFFECTED FILES
mwolff accepted this revision.
mwolff added a comment.
lgtm from my side
INLINE COMMENTS
> generator_pdf.cpp:920
> +
> +// Since the timer lives in a thread without even loop we need to stop
> it ourselves
> +// when the remaining time has reached 0
typo: "without _an_ even_t_
aacid updated this revision to Diff 21756.
aacid added a comment.
Add comment about why we need to stop the timer manually
REPOSITORY
R223 Okular
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D8379?vs=21754=21756
BRANCH
updateCallback (branched from master)
REVISION DETAIL
aacid marked an inline comment as done.
aacid added inline comments.
INLINE COMMENTS
> mwolff wrote in generator_pdf.cpp:919
> That's quite surprising to me. I thought a singleshot timer gets inactivated
> automatically after the first shot... Is that really not the case?
So both you and I are
mwolff added inline comments.
INLINE COMMENTS
> aacid wrote in generator.h:581
> Are you asking for a reword? i see both my and your sentence basically say
> the same?
well, mine is shorted. Your's probably refers to going through the event loop
or something? It boils down to the same thing.
aacid marked an inline comment as done.
aacid added inline comments.
INLINE COMMENTS
> mwolff wrote in generator.cpp:407
> is `setPixmap` old API? Why create a QPixmap on the heap? That should not be
> done, pass values instead.
Yes, it's existing (not old :P) API
> mwolff wrote in
aacid updated this revision to Diff 21753.
aacid added a comment.
Update code for new poppler patch that uses a QVariant for the payload
REPOSITORY
R223 Okular
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D8379?vs=21609=21753
BRANCH
updateCallback (branched from master)
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
aacid added a comment.
In https://phabricator.kde.org/D8379#160970, @rkflx wrote:
> The latest version is much better and I could not reproduce my issues from
above anymore. Still some oddities (tested using `dublin-center-street.pdf`):
>
> - With Fit Page, first the main view
aacid updated this revision to Diff 21609.
aacid added a comment.
rebase
REPOSITORY
R223 Okular
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D8379?vs=21607=21609
BRANCH
updateCallback (branched from master)
REVISION DETAIL
https://phabricator.kde.org/D8379
AFFECTED FILES
aacid updated this revision to Diff 21607.
aacid added a comment.
Don't ask for incremental updates if the render is not async
Fixes crash when opening the presentation view
REPOSITORY
R223 Okular
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D8379?vs=21372=21607
BRANCH
rkflx added a comment.
The latest version is much better and I could not reproduce my issues from
above anymore. Still some oddities (tested using `dublin-center-street.pdf`):
- With Fit Page, first the main view renders and after finishing the
thumbnail should be the only thing left to
rkflx added a comment.
In https://phabricator.kde.org/D8379#158669, @aacid wrote:
> Fix pixmaps not getting updated when the tile manager kicks in
>
> Also make the tile request be partially updated if that's what the request
wants
Thanks for updating, I'll try to re-test
aacid marked an inline comment as done.
REPOSITORY
R223 Okular
REVISION DETAIL
https://phabricator.kde.org/D8379
To: aacid, #okular, mlaurent
Cc: rkflx, ngraham, michaelweghorn, mlaurent, #okular, aacid
aacid updated this revision to Diff 21372.
aacid added a comment.
Update coding style
Though the coding style is not really very set on this file, there's a mix of
spaces and not spaces, but let's make Laurent happy :)
REPOSITORY
R223 Okular
CHANGES SINCE LAST UPDATE
mlaurent requested changes to this revision.
mlaurent added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> generator.h:584
> + */
> +void signalPartialPixmapRequest( Okular::PixmapRequest * request,
> const QImage );
> +
Coding style:
void
aacid added a comment.
In https://phabricator.kde.org/D8379#157771, @rkflx wrote:
> That's a really great feature Okular's user will surely love. Does this
solve https://bugs.kde.org/show_bug.cgi?id=344081?
Yes
> When testing (with https://phabricator.kde.org/D8378 and
aacid updated this revision to Diff 21161.
aacid added a comment.
Add BUGS
REPOSITORY
R223 Okular
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D8379?vs=21160=21161
BRANCH
updateCallback (branched from master)
REVISION DETAIL
https://phabricator.kde.org/D8379
AFFECTED
aacid updated this revision to Diff 21160.
aacid added a comment.
Fix pixmaps not getting updated when the tile manager kicks in
Also make the tile request be partially updated if that's what the request
wants
REPOSITORY
R223 Okular
CHANGES SINCE LAST UPDATE
ngraham added a comment.
This definitely looks like it implements that feature request. Can we add
"BUG: 344081" to the Summary?
REPOSITORY
R223 Okular
REVISION DETAIL
https://phabricator.kde.org/D8379
To: aacid, #okular
Cc: rkflx, ngraham, michaelweghorn, mlaurent, #okular, aacid
rkflx added a comment.
That's a really great feature Okular's user will surely love. Does this solve
https://bugs.kde.org/show_bug.cgi?id=344081?
When testing (with https://phabricator.kde.org/D8378 and
https://phabricator.kde.org/D8379 both applied at the same time – sorry for
that –,
aacid updated this revision to Diff 21008.
aacid added a comment.
Normalized Q_ARG param signature
REPOSITORY
R223 Okular
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D8379?vs=21001=21008
BRANCH
updateCallback (branched from master)
REVISION DETAIL
aacid marked an inline comment as done.
REPOSITORY
R223 Okular
REVISION DETAIL
https://phabricator.kde.org/D8379
To: aacid, #okular
Cc: mlaurent, #okular, aacid
mlaurent added inline comments.
INLINE COMMENTS
> generator_pdf.cpp:929
> +auto payload = static_cast(payloadA);
> +QMetaObject::invokeMethod(payload->generator,
> "signalPartialPixmapRequest", Qt::QueuedConnection,
> Q_ARG(Okular::PixmapRequest *, payload->request), Q_ARG(QImage,
aacid added a comment.
You can see a video of the difference here
https://www.youtube.com/watch?v=NaUbWL6800Y
REPOSITORY
R223 Okular
REVISION DETAIL
https://phabricator.kde.org/D8379
To: aacid, #okular
Cc: #okular, aacid
aacid added a comment.
One PDF that benefits from this is
https://scalablemaps.com/download-request/dublin-center-street/pdf (even though
this is not very slow it shows the difference)
REPOSITORY
R223 Okular
REVISION DETAIL
https://phabricator.kde.org/D8379
To: aacid, #okular
Cc:
aacid added a reviewer: Okular.
aacid added a comment.
Upstream poppler patch needed:
https://bugs.freedesktop.org/show_bug.cgi?id=103372
It's not mandatory but used with https://phabricator.kde.org/D8378 gives
better results since that way we ensure that the pixmap generation takes
aacid created this revision.
Restricted Application added a subscriber: Okular.
Restricted Application added a project: Okular.
REVISION SUMMARY
This way pages that take more than 500ms to render get updated every so often
so that the
user can see that the program didn't hang, it's just that
37 matches
Mail list logo
