Add support for configurable transparent background to SVGs
ClosedPublic

Authored by huoni on Sat, Mar 24, 1:46 AM.

Details

Summary

SVGs do not honor the "Transparent background" config option, and simply have no
background at all. This patch ensures this option is applied to SVGs as well as
raster images.

Unlike RasterImageView that uses a QPixmap buffer (mCurrentBuffer) for drawing
the background and the image, SVGs are rendered directly on top of SvgImageView
(QGraphicsWidget). Therefore we just paint the background in SvgImageView::paint
which happens before the SVG is rendered.

We move the checkboard texture code to AbstractImageView now that it's used by
both subclasses RasterImageView and SvgImageView.

This patch also configures kconf_update due to the moving of the above enum.
Since I didn't want to litter /app with files, I've moved the update files to
/kconf_update.

Fixes T8125

Before:

After:


Test Plan
  • Open an SVG with solid color config option
  • Open and SVG with checkboard background config. At a high enough zoom (where the birdseye view shows up), panning/scrolling should have the checkboard pattern fixed to the image, like raster images
  • Ensure raster images with transparent backgrounds are unaffected

kconf_update

The script gwenview-imageview-alphabackgroundmode-update.pl must be in
/usr/share/kconf_update in order for /usr/lib/kf5/kconf_update to find it.
If successfull, instances of RasterImageView::AlphaBackground* should change to
AbstractImageView::AlphaBackground*.

Diff Detail

Repository
R260 Gwenview
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
huoni requested review of this revision.Sat, Mar 24, 1:46 AM
huoni created this revision.
huoni edited the summary of this revision. (Show Details)Sat, Mar 24, 5:00 AM

(See https://secure.phabricator.com/T5132#69200 for the exact syntax to use for "Depends on" and for adding tasks. Apparently it's still an open task on upstream Phab…)

Works nicely, and even for the screenshots you selected image and colour carefully πŸ‘Œ

A couple of inline comments for now, I'll continue with the code review later.

lib/documentview/svgviewadapter.cpp
156–157

As mentioned in T8125, I'm not sure hasAlphaChannel makes sense for SVGs. Perhaps only mention that you'll draw the background for every SVG, or even remove the whole comment.

174–176

I'm afraid neither the compiler nor the machine running the code will care much about your comment ;)

In general for situations which "should never happen" you can Q_ASSERT(0); which kills Gwenview (at least in Debug mode) and thus makes the problem visible.

lib/gwenviewconfig.kcfg
121–125

If you want to rename this, you'd need to provide a kconf_update script, otherwise users will lose their old settings.

huoni added inline comments.Sun, Mar 25, 10:07 AM
lib/documentview/svgviewadapter.cpp
174–176

I thought it was good practice to always have a default clause, to communicate that you've considered all cases.

Would it be preferable to put Q_ASSERT(0) here instead, or just take out default?

lib/gwenviewconfig.kcfg
121–125

RasterImageView::AlphaBackgroundMode is still valid code, so old settings would probably still work. Would it be preferable to leave the kcfg file alone if it still works?

rkflx added inline comments.Sun, Mar 25, 10:22 AM
lib/documentview/svgviewadapter.cpp
174–176

Well, assume you add an entry to the enum. Now both the compiler and static analysis tools will see your default and assume this was a deliberate choice, i.e. you'll not get notified that probably you should add the new case here too.

When testing I expected to get a compiler warning when there were unhandled cases (i.e. when removing all but the first case in your example), but this somehow did not work for me (not sure why).

In other places in Gwenview the ASSERT is used inside the default, so I'd say just do it this way.

lib/gwenviewconfig.kcfg
121–125

Does it still work, though? Open old Gwenview, change setting, open new Gwenview: Is the change retained?

For cat ~/.config/gwenviewrc I get:

AlphaBackgroundMode=RasterImageView::AlphaBackgroundSolid

vs.

AlphaBackgroundMode=AbstractImageView::AlphaBackgroundSolid

...which in my book is a different config entry entirely ;)

Peter recently added a kconf_update script for Gwenview, I think you can piggyback on that one.

huoni updated this revision to Diff 30585.Mon, Mar 26, 4:37 AM
huoni marked 3 inline comments as done.
  • Remove unneeded comment
  • Use Q_ASSERT(0) instead of empty default: clause
huoni added a comment.Mon, Mar 26, 4:38 AM

See inline comment

lib/gwenviewconfig.kcfg
121–125

Right, so I think I know how these update scripts work.
Would I simply edit app/gwenview.upd to add my changes, under a new Id= line?

rkflx added inline comments.Mon, Mar 26, 7:43 AM
lib/gwenviewconfig.kcfg
121–125

Yup, that's how I understood it too. Konversation is doing something similar.

rkflx requested changes to this revision.Mon, Mar 26, 8:33 PM

Finally looked more in-depth at the code. There are still some improvements you could make, but we are getting there…

lib/documentview/abstractimageview.cpp
275

I believe const QPixmap& would avoid a bunch of useless copying.

lib/documentview/rasterimageview.h
52–53

AFAIK we can now assume usage of a C++11 compiler, so you could directly use override in new code.

lib/documentview/svgviewadapter.cpp
52–53

You can initialize those member variables the same way it's done for mSvgItem above.

144

In the config dialog, Apply has an immediate effect for raster images (good), but for SVGs you have to hit OK for the background to change (bad).

Possibly setAlphaBackgroundMode is called in this case, where for RasterImageView there is update, so you'd need something similar for SVGs?

157

In theory, background is needed only for a single case, so it can be moved there. However, we won't need it at all ;)

161

As you are using this variable only a single time and it is constructed by a simple function call, you could just use the function inline, saving an extra copy.

162

Why write this complicated construct when you could simply write scrollPos() inline?

163–165

Why do you need the second painter? The following works just fine for me:

painter->drawTiledPixmap(imageRect, alphaBackgroundTexture(), scrollPos());
180

Adding qDebug(), this method and thus SvgImageView::drawAlphaBackground is called a crazy number of times compared to RasterImageView::updateFromScaler, i.e. very often when displaying the image for the first time and also each time the cursor enters the viewport.

I wonder what's done differently for raster images to prevent this (perhaps via QGraphicsView's architecture?). Also, glancing at the API docs I discovered paint and setCacheMode for QGraphicsItem, not sure if this will help.

lib/documentview/svgviewadapter.h
53–54

I would make both functions private.

59

int β†’ AbstractImageView::AlphaBackgroundMode

This revision now requires changes to proceed.Mon, Mar 26, 8:33 PM
huoni updated this revision to Diff 30670.Mon, Mar 26, 10:29 PM
huoni marked 11 inline comments as done.
  • Code cleanup/improvements
  • Use graphics cache to prevent unnecessary repaints of background

Thanks again for the in depth review, I'm learning a lot!

lib/documentview/abstractimageview.cpp
275

It's hard to know when to conform to surrounding code, and when to break that consistency. In hindsight, a QPixmap is going to be more expensive to copy than QRect and the like. so this makes sense.

lib/documentview/rasterimageview.h
52–53

TIL, thanks.
In these situations, is it beneficial to submit a patch changing all the old code? In this case, essentially replace all Q_DECL_OVERRIDE with override? Or is it preferred to use the new style and leave old code as is (and end up with a mix like connect()).

lib/documentview/svgviewadapter.cpp
163–165

All the above was a result of copying a similar structure from I believe RasterImageView. At the time of course I'm just trying to get things to work, and not thinking too deeply. Luckily your awesome reviews catch it!
I think I'll try to review my own work so to speak before submitting in future, to hopefully catch a lot of these things.

180

Looks like caching works here, as long as we manually call update() when the background needs changing (on zoom/pan).

Note that RasterImageView::paint is also called numerous times, but it's only drawing mCurrentBuffer. We might be able to copy that behaviour, and implement a buffer here, but I think caching is the better option.

huoni updated this revision to Diff 30688.Tue, Mar 27, 5:54 AM
huoni edited the summary of this revision. (Show Details)
huoni edited the test plan for this revision. (Show Details)
  • Add kconf_update script
rkflx added a comment.Tue, Mar 27, 9:06 PM

Nearly there, so I can (hopefully) move to Peter's and your other reviews next…

CMakeLists.txt
157

Why not name the directory kconf_update?

lib/documentview/abstractimageview.cpp
140

That's probably not needed anymore.

lib/documentview/rasterimageview.h
52–53

At some point we could run tools like Clazy and clang-tidy over the whole code base and apply their automated fix-ups (and replace Q_DECL_OVERRIDE manually as well). Of course this makes git blame harder to use, which is also the reason to prefer the new style for new code righ away or when a line of code is touched anyway.

lib/documentview/svgviewadapter.cpp
124

You missed a spot ;)

126–137

After you latest changes you can finally drop those curly braces, which are quite uncommon for switch blocks.

180

I'd assume that drawTilePixmap is not that much more expensive than drawPixmap of a potential SvgImageView::mCurrentBuffer, so the current version with single occasional repaints should be fine. Great setCacheMode is working out for us, it was quite a long shot…

updaters/gwenview-imageview-alphabackgroundmode-update.pl
6 β†—(On Diff #30688)

FYI: Unless someone manually edited the config file, the default value will not be specified explicitly so your condition will never trigger. This trick will allow us to change the default to a different value, BTW.

huoni updated this revision to Diff 30767.Tue, Mar 27, 10:37 PM
huoni marked 5 inline comments as done.
  • Rename updaters/ -> kconf_update/
  • Code cleanup
CMakeLists.txt
157

Because Konversation used updaters (was looking at other examples), but kconf_update is better I agree.

rkflx accepted this revision.Tue, Mar 27, 10:43 PM

LGTM ;)

This revision is now accepted and ready to land.Tue, Mar 27, 10:43 PM

LGTM ;)

Thanks!

huoni edited the summary of this revision. (Show Details)Tue, Mar 27, 11:58 PM
ngraham added inline comments.Fri, Mar 30, 4:03 PM
kconf_update/gwenview-imageview-alphabackgroundmode-update.pl
2

Is there ever going to be an environment where this could run that doesn't have Perl? I'm not one of those Perl haters, but it seems like this could be done more easily and in a more guaranteed-bulletproof manner with sed in a shell script instead.

rkflx added inline comments.Fri, Mar 30, 9:03 PM
kconf_update/gwenview-imageview-alphabackgroundmode-update.pl
2

The docs say the following on that topic:

stick with interpreted scripts like shell or perl. To make your scripts compatible with Windows, you should not use shell scripts.

Also, lots of similar kconf_update scripts are written in Perl, so I guess this is fine.

huoni added inline comments.Fri, Mar 30, 10:10 PM
kconf_update/gwenview-imageview-alphabackgroundmode-update.pl
2

Thanks for answering for me :)
Those are indeed the reasons I chose Perl.

ngraham accepted this revision.Fri, Mar 30, 10:24 PM
rkflx added a comment.Tue, Apr 3, 11:57 PM

One more thing…

kconf_update/gwenview-imageview-alphabackgroundmode-update.pl
2

This path does not exist on some systems I tested. Konversation uses /usr/bin/perl, but I guess the "official" way would be #!/usr/bin/env perl.

huoni updated this revision to Diff 31246.Wed, Apr 4, 12:12 AM
huoni marked an inline comment as done.
  • Rebase
  • Change update script shebang to use env
huoni updated this revision to Diff 31442.Fri, Apr 6, 12:36 AM
  • Fix SVG background being drawn in wrong position for split second
rkflx accepted this revision.Fri, Apr 6, 9:57 AM

Still LGTM ;)

This revision was automatically updated to reflect the committed changes.