Add "None" option for transparent background in Image view
ClosedPublic

Authored by huoni on Mar 24 2018, 1:57 AM.

Details

Summary

Some users may prefer viewing images with an alpha channel without a
background. Before D11629 this was the behaviour for SVGs, but is now
impossible for both SVGs and raster images. This patch adds this option,
effectively adding functionality for raster images, and getting back the
previous behaviour for SVGs.
We also make None the default.

For raster images, choosing None paints the buffer with
Qt::transparent where needed.
For SVGs, this option doesn't draw anything, which was the behaviour before
D11629.

Depends on D11629

Example (PNG on left, SVG on right):

Fullscreen:

Test Plan

Test the None option for both raster images and SVGs.
Ensure other options (checkboard and solid color) are unaffected.

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.Mar 24 2018, 1:57 AM
huoni created this revision.
huoni retitled this revision from Add "None" option for Alpha background to Add "None" option for Alpha background in Image view.Mar 24 2018, 1:59 AM

Oh goodness, I have wanted this for so long!

huoni added a comment.Mar 24 2018, 4:29 AM

Oh goodness, I have wanted this for so long!

Glad to be of service :)

huoni edited the summary of this revision. (Show Details)Mar 24 2018, 5:10 AM
huoni retitled this revision from Add "None" option for Alpha background in Image view to Add "None" option for transparent background in Image view.Mar 24 2018, 6:00 AM

@huoni @muhlenpfordt @ngraham I wonder whether we should make this option the new default? This would bring it in line with Browse mode, and also Firefox is showing a solid greyish background since some time, so users already know the behaviour from there. For those needing the checkerboard the option to change back would still exist. Thoughts?

Regarding the naming: None sounds a bit odd. Perhaps Blend in? Other ideas? Should we change Transparent background to something else too?

zzag added a subscriber: zzag.Mar 24 2018, 11:42 AM

I put my 2 cents, if you don't mind.

I wonder whether we should make this option the new default? ... Thoughts?

-1 for this. Checkerboard is de-facto standard when it comes to background for images with alpha channel. Without checkerboard it's not clear whether image has transparency.

also Firefox is showing a solid greyish background since some time

GIMP, Photoshop, Google Chrome and so on show checkerboard. Firefox is an outlier and that's [not showing checkerboard] totally wrong, IMHO.

In D11630#232793, @zzag wrote:

I put my 2 cents, if you don't mind.

We consider every input, thanks for commenting ;)

I wonder whether we should make this option the new default? ... Thoughts?

-1 for this. Checkerboard is de-facto standard when it comes to background for images with alpha channel. Without checkerboard it's not clear whether image has transparency.

That's true, but it's also more of an expert feature. We have to balance here the needs of regular users, and those of more experienced users. I felt None was the more appropriate default for Gwenview.

also Firefox is showing a solid greyish background since some time

GIMP, Photoshop, Google Chrome and so on show checkerboard. Firefox is an outlier and that's [not showing checkerboard] totally wrong, IMHO.

I just tried in Chromium, and for https://bugsfiles.kde.org/attachment.cgi?id=109324 I got a solid black background. GIMP and Photoshop are advanced editing apps, Gwenview is just a simple viewer. Also, you are still able to tell the transparency, because the background will shine through (a dark grey in windowed mode, a textured black in fullscreen mode).

Anyway, I'm not married to the idea, I just thought the checkerboard would be annoying for anyone looking primarily at SVG icons (a use case which comes up very often in Bugzilla, for which D11629 will introduce the checkerboard which is currently not there ).

Let's see what others have to say before taking a decision.

zzag added a comment.Mar 24 2018, 12:19 PM

I just tried in Chromium, and for https://bugsfiles.kde.org/attachment.cgi?id=109324 I got a solid black background.

Oh, yeah, my bad, sorry. I always thought it shows checkerboard.

I wonder whether we should make this option the new default?

I would expect to see the checkboard background in transparent areas so I think this is a good default option. It's the only pattern you can instantly imagine this is not part of the image.

Regarding the naming: None sounds a bit odd. Perhaps Blend in? Other ideas?

"Window background" or "Window color"?

Btw. In fullscreen mode I see a solid gray background for transparent PNG images with the new None option. SVG images show the normal fullscreen background.

+1 for making "None" the default. Gwenview is a simple image viewer where IMHO aesthetics are more important than exposing advanced features. And we do have the advanced feature anyway, for those who want it.

"Simple by default, powerful when needed."

While we're at it, could we consider changing the name of the label to make this feature more obvious? "Transparent background" has always seemed a bit awkward to me. How about "Background for transparent areas"?

rkflx added a comment.Mar 24 2018, 5:30 PM

+1 for making "None" the default. Gwenview is a simple image viewer where IMHO aesthetics are more important than exposing advanced features. And we do have the advanced feature anyway, for those who want it.

I tend to agree, but Peter also has a point. Let's wait for Huon.

While we're at it, could we consider changing the name of the label to make this feature more obvious? "Transparent background" has always seemed a bit awkward to me. How about "Background for transparent areas"?

Yup, see above where I had the same idea. However, there is only so much space on the left side… Maybe we can move "background" over like so:

Transparent images:

  • Checkerboard background (is this the correct term vs "check board"?)
  • Coloured background: […]
  • Window background (good idea, Peter ;)

In fullscreen mode I see a solid gray background for transparent PNG images with the new None option. SVG images show the normal fullscreen background.

Indeed, seems like a fake None background :D We'll have to find a way to fix this…

rkflx added a comment.EditedMar 24 2018, 5:39 PM

Maybe we can move "background" over like so

Another option would be to introduce a header like Thumbnail Bar, but I find this clutters the dialog too much.

While it's technically possible to use multi-line labels, I don't think the HIG would approve this (did not find anything though, it just says "Keep labels short").

I don't think the string "transparent images" is right since that implies that whole images will be transparent, which doesn't make sense. How about:

Image transparency: O Checkerboard background
                    O Specific background color [  ]
                    O Standard background color

(Yes, the correct world is "checkerboard", in English at least)

Another thing I'm realizing: with all these radio button controls, the window is getting too tall and will always have a vertical scroll bar, which isn't ideal for settings windows. We should consider switching some of them to be comboboxes where possible (i.e. not for this one since one of the options unlocks a color picker).

rkflx added a comment.Mar 24 2018, 8:10 PM

I don't think the string "transparent images" is right since that implies that whole images will be transparent, which doesn't make sense.

Well, it depends on the image, really. Either it has an alpha channel (and then arbitrary pixels can have varying levels of transparency, e.g. a see-through red layer covering the whole image is also possible), or it hasn't (and then the background does not matter at all). We need to communicate that this only applies to a certain type of image, but "image transparency" implies that you can change all images themselves (which you cannot). BTW, you can also observe this in Browse mode, where there are different selection and hover methods for both image types.

Image transparency: O Checkerboard background
                    O Specific background color [  ]
                    O Standard background color

Hmh, what is "standard" anyway?

Another thing I'm realizing: with all these radio button controls, the window is getting too tall and will always have a vertical scroll bar, which isn't ideal for settings windows. We should consider switching some of them to be comboboxes where possible (i.e. not for this one since one of the options unlocks a color picker).

Yes, we should work on that (but in a different patch). However, I think comboboxes work better for >3 entries, for less entries radiobuttons are easier. From a high level it's simple: If there is not enough space, instead of squeezing things together we'd need to introduce a new page.

+1 for making "None" the default. Gwenview is a simple image viewer where IMHO aesthetics are more important than exposing advanced features. And we do have the advanced feature anyway, for those who want it.
I tend to agree, but Peter also has a point. Let's wait for Huon.

I can see both arguments, but given Gwenview is a simple image viewer first, I think "None" as the default is better. This was the (only) case with SVGs after all.

In fullscreen mode I see a solid gray background for transparent PNG images with the new None option. SVG images show the normal fullscreen background.

Indeed, seems like a fake None background :D We'll have to find a way to fix this…

That's embarrassing. After all my work on the fullscreen palettes...

Image transparency: O Checkerboard background
                    O Specific background color [  ]
                    O Standard background color

"Image transparency" works for me, though I can see how it could be interpreted to mean you are changing the image itself.
It might be too wordy, but what about:

Show transparency with: O Checkerboard background
                        O Solid color background   [  ]
                        O No background

Regarding the naming: None sounds a bit odd. Perhaps Blend in? Other ideas? Should we change Transparent background to something else too?

I think None implies that whatever is underneath will show through instead. In this case, underneath is the image view background color.
Perhaps better (as in my example above), "no background" is better? Something like "window background" seems more awkward to me. Another idea would be something more descriptive like "Same as window background".

ngraham added a comment.EditedMar 24 2018, 10:11 PM

It might be too wordy, but what about:

Show transparency with: O Checkerboard background
                        O Solid color background   [  ]
                        O No background

+1, that's much better. And it reads as a complete sentence too (in English at least), the way radio buttons with a label should!

"No background" is a bit confusing though; it will have a background, it's just the default one, not some special one. How can we communicate that?

"No background" is a bit confusing though; it will have a background, it's just the default one, not some special one. How can we communicate that?

To me, "no background" does in fact communicate that there's no special background, i.e. the background will match the surrounding color.

"None" as the default is better. This was the (only) case with SVGs after all.

Okay, let's make this the new default.

Show transparency with: O Checkerboard background
                        O Solid color background   [  ]

+10

Once we agreed on the naming of the last option, I'll commit this separately.


O No background

One more idea (referring to the title of the config page):

O Image View background

Yay/Nay?

One more idea (referring to the title of the config page):

O Image View background

Yay/Nay?

I like it. Yay!

huoni added a comment.Mar 25 2018, 1:22 AM

One more idea (referring to the title of the config page):

O Image View background

Yay/Nay?

I like it. Yay!

Hmm, I'm not sure. As a user, reading "Image View background", I might start looking for an "Image View background color" setting somewhere.

huoni updated this revision to Diff 30588.EditedMar 26 2018, 5:39 AM
huoni edited the summary of this revision. (Show Details)
  • Paint raster images with transparent background so now adapts to any BG (including fullscreen texture)
  • Tweak fade animation when switching images to fix ugly artifacts and jerkiness
  • Use Q_ASSERT(0) instead of empty default clause
  • Make 'None' the default
huoni added a comment.Mar 26 2018, 5:41 AM

I have fixed the issue with fullscreen, and at the same time implemented a better solution (no longer fakes the background).

Regarding the naming of the config page, I will leave that at least until we have consensus (perhaps it could go in a different patch)

huoni edited the summary of this revision. (Show Details)Mar 26 2018, 5:47 AM
huoni updated this revision to Diff 30672.Mar 26 2018, 10:42 PM
  • Rebase
  • Improve logic around painter composition mode when drawing alpha background for raster images
rkflx added a comment.Mar 27 2018, 9:07 PM

no longer fakes the background

Yay!

Regarding the naming of the config page, I will leave that at least until we have consensus (perhaps it could go in a different patch)

Yup, let's not worry about that problem in this Diff.

Tweak fade animation when switching images to fix ugly artifacts and jerkiness

Hmh, I don't really like the new fading behaviour that much. In particular, if you have two similar images, we used to fade nicely between both, but now the background is peeking through, creating a kind of flicker effect. It's even worse with the checkerboard being visible for a short time. (Marco describes essentially the same in https://phabricator.kde.org/D11726#inline-58334.) I think we have to find a different solution here…

huoni added a comment.EditedMar 27 2018, 11:45 PM

Hmh, I don't really like the new fading behaviour that much. In particular, if you have two similar images, we used to fade nicely between both, but now the background is peeking through, creating a kind of flicker effect. It's even worse with the checkerboard being visible for a short time. (Marco describes essentially the same in https://phabricator.kde.org/D11726#inline-58334.) I think we have to find a different solution here…

Hmm, I don't see a huge problem to be honest. I can't see any flicker effect fading between two wallpapers (no transparency). Can't even notice the background showing through.
Transparency is the complicating factor here.

There are really two problems here:

  1. Transparent background on raster images doesn't fade properly. This is a new issue introduced by this patch.
  2. When we are fading to/from an image with transparency, we are viewing both images at the same time. Therefore unless we fade both images, we are going to see one of the images appear/disappear instantly. This is a pre-existing issue (only affected SVGs up until this patch).

As for the fade, there are three options:

  1. Original - new image fades in on top of old image
  2. Reversed - old image fades out revealing new image
  3. Dual - fade out old image and fade in new image at the same time

#2 Reversed doesn't really fix anything, it simply reverses both problems.
#3 Dual is the only one that fixes both problems. I thought the small problem of background being a bit visible during the fade was worth getting a consistent smooth transition between any kind of image (raster, SVG, with/without transparency).

Perhaps there's a way to fix the actual fade animation itself so it isn't affected by transparency. But if there isn't, I think the dual fade is the best option overall.

Examples

Original Fade showing problem #1:

Original Fade showing problem #2:

Reversed Fade showing problem #1 (less pronounced):

Reversed Fade showing problem #2:

Dual Fade showing problem #1 and #2 (fixed):

huoni added a comment.Mar 28 2018, 5:34 AM
  1. Transparent background on raster images doesn't fade properly. This is a new issue introduced by this patch.

After further investigation, this problem actually has nothing to do with mCurrentBuffer, but is only now apparent because mCurrentBuffer has transparent pixels now.

To make this apparent, I made the following changes:

  • Prevented the old image from being deleted
  • Removed the fade and manually set the new image to .8 opacity
  • Had RasterImageView draw mCurrentBuffer at coords 0,0

All this results in the following screenshot. It shows that the section that isn't faded properly actually matches documentSize() * zoom() (indicated using red outline) and it changes when you change the zoom level.

If this is fixable (I still haven't gotten to the bottom of it), it would solve problem 1, but not problem 2. Which weakens my argument for the Dual Fade.

Anyway, I'll keep working on this. If anyone has insight, I'll gladly take it :)

rkflx added a comment.Mar 28 2018, 8:44 PM

Sorry, did not have a chance yet to look into this. However, two general comments:

  • Gwenview's primary use case is viewing photos, meaning we should not regress in that regard while working on a secondary use case like viewing transparent raster and SVG images.
  • From a high level, a good-looking transition blends in the new "scene" on top of the old "scene". Alternatively, blending out the old scene with the new scene below should give the same result. The important point here is that "scene" is defined as the final rendering of a transparent image on top of a background, including any surrounding background. IOW, instead of blending only individual images (which could be smaller than the complete viewport) the old viewport is kept and the final rendering of the new viewport is blended in.

Not sure how that maps to QGraphicsScene, though. This all sounds like it should be in a different patch anyway…

Can't even notice the background showing through.

Did you test with light images over a dark background (or vice versa)? Let me know if I should record a video of the effect.

Sorry, did not have a chance yet to look into this. However, two general comments:

  • Gwenview's primary use case is viewing photos, meaning we should not regress in that regard while working on a secondary use case like viewing transparent raster and SVG images.

Good point.

  • From a high level, a good-looking transition blends in the new "scene" on top of the old "scene". Alternatively, blending out the old scene with the new scene below should give the same result. The important point here is that "scene" is defined as the final rendering of a transparent image on top of a background, including any surrounding background. IOW, instead of blending only individual images (which could be smaller than the complete viewport) the old viewport is kept and the final rendering of the new viewport is blended in.

Agreed, this is what I'll aim for. Currently that consistent background is missing from each scene, hence our problems.

Not sure how that maps to QGraphicsScene, though. This all sounds like it should be in a different patch anyway…

Okay I'll make another dependant patch, and revert this one to the original fade. But I won't land this before fixing the transition animation.

Can't even notice the background showing through.

Did you test with light images over a dark background (or vice versa)? Let me know if I should record a video of the effect.

Yeah I can see it now.

huoni updated this revision to Diff 30819.Mar 28 2018, 11:00 PM
  • Revert transition fade animation changes (will go in separate patch)
huoni edited the summary of this revision. (Show Details)Mar 28 2018, 11:34 PM

@huoni The patch might need a rebase (arc patch does not work currently).

huoni updated this revision to Diff 30890.Mar 29 2018, 10:35 PM
  • Rebase
huoni edited the summary of this revision. (Show Details)Mar 30 2018, 7:07 AM
ngraham added inline comments.Mar 30 2018, 4:20 PM
app/imageviewconfigpage.ui
634

The correct term is "Checkerboard"

rkflx added inline comments.Mar 30 2018, 9:07 PM
app/imageviewconfigpage.ui
634

IIRC we agreed on changing the wording in a separate patch. Please give me a couple of days, busy with AFK things ATM. I'll try to make progress with the open reviews next week, sorry for the wait…

ngraham accepted this revision.Mar 31 2018, 2:31 PM

This checks out for me, modulo a style nitpick. Let's wait for @rkflx's final review before committing though.

app/imageviewconfigpage.ui
634

Ah, of course.

lib/documentview/rasterimageview.cpp
168

Style: In general, case/switch statements in KDE code don't use braces.

This revision is now accepted and ready to land.Mar 31 2018, 2:31 PM
huoni added inline comments.Mar 31 2018, 11:08 PM
lib/documentview/rasterimageview.cpp
168

textureOffset is defined in the first case. To take away the braces, I'd need to move the definition to before the switch, even though it's only used in the first case. Would that still be preferable?
Perhaps taking away the braces for the cases that don't need them, leaving them for the first case - that way it's less a style decision and more a function decision.

zzag added inline comments.Mar 31 2018, 11:49 PM
lib/documentview/rasterimageview.cpp
168

textureOffset is used only once so there is no need to declare a variable.

Also, cases don't need parentheses.

huoni updated this revision to Diff 31131.Apr 1 2018, 10:45 PM
  • Fix up switch cases
huoni added inline comments.Apr 1 2018, 10:46 PM
lib/documentview/rasterimageview.cpp
168

I removed the blocks apart from the first one, but changed the opening brace to be on its own line.
This is more like an explicit block used for restricting scope which is used in other parts of the file.

rkflx requested changes to this revision.Apr 3 2018, 11:57 PM

We have

  • decided on a default option ✅
  • optimized the wording ✅
  • fixed glitches with fading ✅

…so now we are nearly ready to go (only some minor edits left). Thanks everyone for your patience and ideas in the discussions so far :)


@huoni You've got a bunch of unrelated edits in app/imageviewconfigpage.ui, perhaps caused by Qt Designer. Would it be possible to either avoid those altogether or at least split them out in a separate patch?

A fix to this will go in a separate patch. EDIT: D11795

I always thought commit messages in Git are considered immutable for public repos :D You might want to polish the wording here a bit… (Also: "beheviour")

lib/documentview/rasterimageview.cpp
168

Good call, makes sense.

182–184

I'd put this as the first case in the switch, to mirror the enum as well as the UI.

386

Accidental whitespace removal.

This revision now requires changes to proceed.Apr 3 2018, 11:57 PM
huoni updated this revision to Diff 31247.Apr 4 2018, 12:44 AM
huoni marked 2 inline comments as done.
huoni edited the summary of this revision. (Show Details)
  • Revert and manually edit the config UI file
  • Change order of switch cases to match enum/config page
  • Put back accidentally removed whitespace
huoni added a comment.Apr 4 2018, 12:46 AM

@huoni You've got a bunch of unrelated edits in app/imageviewconfigpage.ui, perhaps caused by Qt Designer. Would it be possible to either avoid those altogether or at least split them out in a separate patch?

That is indeed was all Qt Designer's fault. I went back and made the changes manually instead.

A fix to this will go in a separate patch. EDIT: D11795

I always thought commit messages in Git are considered immutable for public repos :D You might want to polish the wording here a bit… (Also: "beheviour")

I just removed this explanation altogether.

rkflx accepted this revision.Apr 4 2018, 9:01 AM

@huoni You've got a bunch of unrelated edits in app/imageviewconfigpage.ui, perhaps caused by Qt Designer. Would it be possible to either avoid those altogether or at least split them out in a separate patch?

That is indeed was all Qt Designer's fault. I went back and made the changes manually instead.

Thanks for changing this, makes it so much easier to review…

This revision is now accepted and ready to land.Apr 4 2018, 9:01 AM
This revision was automatically updated to reflect the committed changes.