Improve Image View fade transitions
ClosedPublic

Authored by huoni on Mar 30 2018, 2:27 AM.

Details

Summary

This patch fixes three problems:

  1. After D11630, fading to a raster image with alpha channel and 'None' background resulted in the old image showing through the transparent parts of the new image, then disappearing.
  2. Switching to an SVG from a raster image resulted in the raster image not fading, then disappearing suddenly at the end of the animation. This was an issue before D11630.
  3. When OpenGL is used for animations, the full screen background texture appears during animations then disappears.

Problem 1 was due to DocumentView::setEraseBorders, which effectively
filled the background of the document view, leaving the parts within
the image transparent. This was okay before D11630 because the
background behind the image was never visible, it was always filled with
the image itself, or a solid color / checkerboard pattern.
This patch fixes the problem by always filling the background with the
default brush, not just the borders.

Problem 2 is fixed at the same time because the code that erased the
borders had no effect on SVGs because the calculated image rect was the
same size as the document view.

Problem 3 is fixed by using QGraphicsOpacityEffect instead of directoy
setting the opacity using setOpacity.

Depends on D11630

BUG: 373161
FIXED-IN: 18.08.0

Problem 1 Before

Problem 2 Before

After

(White background to show it doesn't shine through)

Test Plan

Test the transition between images in Image View, with the 'None'
transparent background option enabled. Should test between the folowing
types of images:

  • Large raster with no alpha channel, e.g. a wallpaper
  • Raster image with alpha channel
  • SVG

Animation should be smooth with no artifacts. The main view background
shouldn't be visible behind the image during the fade.

View with animations turned off should work as expected.

Full screen should show a textured background with OpenGL animations, i.e.,
no different than with Software animations.

Diff Detail

Repository
R260 Gwenview
Branch
fade-transition (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
huoni requested review of this revision.Mar 30 2018, 2:27 AM
huoni created this revision.
huoni edited the summary of this revision. (Show Details)Mar 30 2018, 2:34 AM
huoni edited the summary of this revision. (Show Details)Mar 30 2018, 7:03 AM
ngraham accepted this revision.Mar 30 2018, 3:39 PM

I like it! Much better than before. Looks good with transitions between regular images, too. And it's always nice to remove more code than you're adding. :)

This revision is now accepted and ready to land.Mar 30 2018, 3:39 PM

And it's always nice to remove more code than you're adding. :)

It feels good :)

huoni updated this revision to Diff 31132.Apr 1 2018, 10:52 PM
  • Rebase
rkflx requested changes to this revision.Apr 2 2018, 7:43 AM

This patch fixes two problems: […]

…but the newly introduced problem detailed in D11630#235665 is still there. This regression is not about SVGs or transparent images at all, it is quite apparent for regular raster images already, i.e. the primary use case: Take two nearly identical screenshots containing dark and light areas each, then fade between them with the general (!) background colour set to black or white. Instead of fading between the screenshots only, the background will shine through momentarily, which I find a bit annoying.

The main view background shouldn't be visible behind the image during the fade.

For me it is, unless I was testing the wrong patch?

This revision now requires changes to proceed.Apr 2 2018, 7:43 AM
huoni added a comment.Apr 2 2018, 8:13 AM

This patch fixes two problems: […]

…but the newly introduced problem detailed in D11630#235665 is still there. This regression is not about SVGs or transparent images at all, it is quite apparent for regular raster images already, i.e. the primary use case: Take two nearly identical screenshots containing dark and light areas each, then fade between them with the general (!) background colour set to black or white. Instead of fading between the screenshots only, the background will shine through momentarily, which I find a bit annoying.

The main view background shouldn't be visible behind the image during the fade.

For me it is, unless I was testing the wrong patch?

No, you are right. I just have trouble noticing it.

Back to the drawing board I guess. I must be missing something. This change should implement your high level description (D11630#236076), where each 'scene' (document view) is rendered with a background and an image, and then faded in.

huoni updated this revision to Diff 31190.Apr 3 2018, 4:48 AM
  • Fix background showing through while fading (I mean it this time)
huoni edited the summary of this revision. (Show Details)Apr 3 2018, 4:51 AM
huoni added inline comments.Apr 3 2018, 4:54 AM
lib/documentview/documentview.cpp
642

eraseRect() no longer worked after implementing QGraphicsOpacityEffect because painter->background() (which is the brush eraseRect uses) was returning pure white until after the fade animation had finished.

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

Yay, animations themselves work great now and this even gets rid of some pre-existing artifacts. Meanwhile I also stumbled upon a068901770dd, which confirms that fading to the background between images was never wanted behaviour.

Found a problem with the palettes though: Switching directly from normal Browse to fullscreen View (e.g. via ViewStart Slideshow or by clicking on the "Fullscreen" hover button) does not update the background colour properly.

Also, setting AnimationsNone is now slightly broken, i.e. no image is shown at all anymore.

lib/documentview/documentview.cpp
644

Why not move this inside the if?

lib/documentview/documentviewcontainer.cpp
337

Please don't use Q_FOREACH for new code, see https://doc.qt.io/qt-5/qtglobal.html#Q_FOREACH.

This revision now requires changes to proceed.Apr 3 2018, 11:57 PM
huoni updated this revision to Diff 31248.Apr 4 2018, 1:16 AM
huoni marked an inline comment as done.
  • Fix images not displaying if animations disabled
  • Fix background not changing when going from Browse -> fullscreen
  • Minor code/comment improvement
huoni marked an inline comment as done.Apr 4 2018, 1:18 AM
huoni edited the test plan for this revision. (Show Details)
huoni updated this revision to Diff 31253.Apr 4 2018, 4:20 AM
  • Combine for loops
rkflx added a comment.Apr 4 2018, 9:01 AM

Now that images display again with animations disabled, I observed another problem: When switching from a transparent raster image to an SVG, for a short moment the background is drawn at an incorrect location:

lib/documentview/documentview.cpp
818–821

Do you have further plans with this function? If not, the code could be written inline.

lib/documentview/documentviewcontainer.cpp
337

Would this also work without (…).toList()?

rkflx added a comment.EditedApr 4 2018, 7:28 PM

In my testing, your patch solves https://bugs.kde.org/show_bug.cgi?id=373161. If you can confirm, you might want to add a BUG: 373161 to your summary (and a FIXED-IN, to make Nate happy ;)

huoni updated this revision to Diff 31346.Apr 5 2018, 1:31 AM
huoni marked 2 inline comments as done.
huoni edited the summary of this revision. (Show Details)
  • Fix SVG background being drawn in the wrong location for a split second
  • Remove setOpacityEffectEnabled()
  • Remove unnecessary toList()
huoni added a comment.Apr 5 2018, 1:36 AM

Now that images display again with animations disabled, I observed another problem: When switching from a transparent raster image to an SVG, for a short moment the background is drawn at an incorrect location:

Well spotted, this was actually introduced in D11629.

In my testing, your patch solves https://bugs.kde.org/show_bug.cgi?id=373161. If you can confirm, you might want to add a BUG: 373161 to your summary (and a FIXED-IN, to make Nate happy ;)

How on earth did you find that bug. It does it fact fix it for me. I hope I got the FIXED-IN version correct.

lib/documentview/documentview.cpp
818–821

My thinking was since setGraphicsEffect is generic, I wanted to make it clearer that we're dealing with the opacity.

It now directly calls setGraphicsEffect(0) from DocumentViewContainer::updateLayout, and I've put a comment to explain that it's the opacity. Is this what you meant?

Perhaps it would be preferred to add a function that effectivly replaces setOpacity, and acts on the graphics effect?

huoni edited the summary of this revision. (Show Details)Apr 5 2018, 1:36 AM
huoni edited the summary of this revision. (Show Details)Apr 5 2018, 6:26 AM

Awesome, can confirm this fixes things.


Well spotted, this was actually introduced in D11629.

…then you should fix it over there, and not here (sorry for not noticing where this was introduced in the first place).

How on earth did you find that bug. It does it fact fix it for me.

Oh, I just typed "bko gwenview animation" or "bko gwenview opengl" into my browser (can't remember anymore), which would then issue a quicksearch on Bugzilla. Then triaged a couple of related bugs which came up for that, and one of them matched your patch.

I hope I got the FIXED-IN version correct.

Yup.

lib/documentview/documentview.cpp
818–821

While it is true that an appropriately named function can help document the code, adding one-liners everywhere for that exact purpose feels counter-productive to me, because it increases cognitive costs for the reader of the code.

I'd say the comment is just fine in this case.

lib/documentview/documentviewcontainer.cpp
337

…and without the braces? (At least to me the operator precedence it pretty clear here.)

huoni updated this revision to Diff 31443.Apr 6 2018, 12:41 AM
  • Revert SVG BG fix (move to D11629), and rebase
  • Remove brackets from for loop
huoni marked an inline comment as done.Apr 6 2018, 12:43 AM

…then you should fix it over there, and not here (sorry for not noticing where this was introduced in the first place).

No that's okay, I should have realised this.

How on earth did you find that bug. It does it fact fix it for me.

Oh, I just typed "bko gwenview animation" or "bko gwenview opengl" into my browser (can't remember anymore), which would then issue a quicksearch on Bugzilla. Then triaged a couple of related bugs which came up for that, and one of them matched your patch.

Right, I thought you might have been searching specifically for this patch :)

lib/documentview/documentviewcontainer.cpp
337

I usually prefer being explicit than relying on operator precedence, but in this case there's really no other way to interpret it!

rkflx accepted this revision.Apr 6 2018, 9:52 AM
This revision is now accepted and ready to land.Apr 6 2018, 9:52 AM
huoni updated this revision to Diff 31537.Apr 7 2018, 12:11 AM
huoni marked an inline comment as done.
  • Don't remove graphics opacity effect when animations off

This change fixes some glitches in compare mode when animations were turned off.

huoni added inline comments.Apr 7 2018, 12:25 AM
lib/documentview/documentview.h
137

I judged adding this function was better than including QGraphicsOpacityEffect in DocumentViewContainer and having to static_cast to set the opacity.

rkflx accepted this revision.Apr 7 2018, 10:42 PM

Cool, not drawing the artifacts in the first place is of course the best solution…

This patch fixes two problems:

I looked some more into 373161, I think the flickering is caused by the texture appearing and disappearing again. After your patch the texture is not missing anymore for OpenGL. Might be worth adding that to the summary (because the two problems you are describing currently do not directly pertain to the bug, confusing future readers…).

lib/documentview/documentview.cpp
361

Why not use setGraphicsEffectOpacity here too?

818–821

After your latest update, the function call makes sense, because you actually need to access the d-pointer.

huoni added inline comments.Apr 7 2018, 11:17 PM
lib/documentview/documentview.cpp
361

Yes I suppose so. Seems a bit odd calling a local function here when we're setting things up though? I can't think of a situation is change the function without touching this code.

Perhaps a better solution would be to put all this code in setGraphicsEffectOpacity, creating and applying the effect the first time it's called.

rkflx added inline comments.Apr 7 2018, 11:31 PM
lib/documentview/documentview.cpp
361

My thinking was that in case later someone adds code to setGraphicsEffectOpacity, it would be easy to miss adding the code here too.

Anyway, you could also just keep it as it is now. No need to overcomplicate this… Also, keeping the init code in the constructor is not too bad either.

huoni added inline comments.Apr 8 2018, 11:38 PM
lib/documentview/documentview.cpp
361

My thinking was that in case later someone adds code to setGraphicsEffectOpacity, it would be easy to miss adding the code here too.

I see the logic, but it is just a basic setter. I think calling setters in the constructor is generally not favourable.
Some arguments regarding this: https://stackoverflow.com/questions/4893558/calling-setters-from-a-constructor#4893604

I think I will leave it as is, if that's okay :)

huoni edited the summary of this revision. (Show Details)Apr 8 2018, 11:41 PM
huoni edited the test plan for this revision. (Show Details)
This revision was automatically updated to reflect the committed changes.
rkflx added inline comments.Apr 9 2018, 8:54 AM
lib/documentview/documentview.cpp
361

Sure thing ;)


However, I don't find linking to a Java question on SO very convincing, in particular because there are differences regarding handling of virtual vs. C++.

Also, I would not call this a "setter", as it's not operating on its own private member, but calling a (arbitrary from out PoV) function on an object.

Still, there are probably other reason why this is good in one situation and bad in another, so as I said, it's fine the way you landed it…