Fix several bugs in the background-painting logic#54
Conversation
Previously the background was painted or erased several times in separate parts of the code when rendering a PDF page. This was partly my own fault, as I did not fully understand the rendering logic when I first wrote the QtPDF backend (the previous Poppler backend I used as a reference also overrode significant parts of it). This new code only paints the background once, and only when necessary to do so. For example, it's not necessary or desirable to paint the background when printing. Another consequence of this fix is that it's possible now to render an image with a transparent background, for example in Frescobaldi's copy-to-image dialog. I believe this was how it worked in older versions of qpageview, and was also the intended behavior here all along.
|
See my later comment below. |
This is an optimization that eliminates the need to fill the background separately before calling drawImage(), which can cause flicker. We do this in job() rather than in render() so that it only affects the cached images used to paint the widget, not images rendered for other purposes such as printing or Frescobaldi's copy-to-image feature.
bmjcode
force-pushed
the
fix-background-painting
branch
from
7fcfce3 to
69ebd8b
Compare
|
Ignore my previous comment. There was a regression in my first commit causing a small amount of excess flicker, which I've since fixed. Whatever flicker remains was already there before, and is beyond the scope of this PR. I'm satisfied now that this fixes the problems it originally set out to solve. |
I've tried but the background is always white. How can you do it? |
The preview in the dialog always shows a white background if no color is set, but this PR makes the actual image that's saved or copied have a transparent background: 
|
|
Here's an image copied with Background unchecked. The background is white: 
I think I'm using this branch in my test, but let me double-check. |
|
No, I was not using this branch of qpageview. I tried a pipx installation that didn't work. and I get a black image if Background is unchecked, as if transparency didn't work. 
|
|
Ooops, sorry, there was the Gray option checked by mistake. 
|
|
In case you have any doubts about what I'm running: |
OK, so there's definitely a bug with the grayscale mode. My guess is it's converting the image to a format that doesn't support transparency.
I don't know why it's doing this. What Qt version are you running? At any rate, let's come back to this one after the release, since it's not performance-critical like my other recent PRs. |
|
I'm running these versions: |
Converting a transparent image as-is usually results in black-on-black, which is less than ideal for readability. Instead, we render the image with a solid background, then use some trickery to manually restore the transparent areas.
|
I found a quite clever fix for transparency in grayscale images: 
I still don't know why yours won't do transparency in color, though. |
This is my favorite optimization yet because it looks so wrong at first glance, but then it makes sense once you've thought more about it.
|
Yes, with the latest commit the Gray option is working correctly also for me: 
|
Best not to risk confusing future maintainers -- especially if one of those future maintainers is me, and therefore prone to messing with anything whose workings are not immediately obvious at first glance.
|
Excellent. (I just tweaked the grayscale code to be slightly clearer, but functionally it is still the same.) Ready to merge? |
|
Ok, merged. Should I open a new issue to remember the issue with the background not being transparent on my configuration? |
|
Yes, let's do that. |
2 participants
Previously the background was painted or erased several times in separate parts of the code when rendering a PDF page. This was partly my own fault, as I did not fully understand the rendering logic when I first wrote the QtPDF backend (the previous Poppler backend I used as a reference also overrode significant parts of it).
This new code only paints the background once, and only when necessary to do so. For example, it's not necessary or desirable to paint the background when printing.
Another consequence of this fix is that it's possible now to render an image with a transparent background, for example in Frescobaldi's copy-to-image dialog. I believe this was how it worked in older versions of qpageview, and was also the intended behavior here all along.