Fix SVGs overlapping selection rect in View compare mode
ClosedPublic

Authored by huoni on Apr 5 2018, 6:14 AM.

Details

Summary

Zooming an SVG in compare mode allowed the SVG to be painted outside its
parent widget SvgImageView, effectively overlapping the selection rect
which is drawn in DocumentView::paint. This patch clips all painting
done by children of AbstractImageView to its own shape, therefore
fixing the issue.

Before:

After:

Test Plan

Open an SVG image in comparison mode.
Zoom in enough so it fills the entire document view (and therefore
can be panned).
Image should not draw over the selection rectangle.

Diff Detail

Repository
R260 Gwenview
Branch
svg-zoom-margins (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
huoni requested review of this revision.Apr 5 2018, 6:14 AM
huoni created this revision.
huoni edited the summary of this revision. (Show Details)Apr 5 2018, 6:18 AM
huoni edited the summary of this revision. (Show Details)Apr 5 2018, 6:40 AM
rkflx added a comment.Apr 5 2018, 9:50 PM

Nice, this gives a very solid feel to the light table.

I went down the proverbial rabbit hole pulling my hair out over this. Then I came back to it the next day and solved it in 5 minutes. Gotta love it!

Yup, those are the best! Thanks for working on this…


To keep the tradition of finding an opportunity for yet another follow-up patch, there is still this irritating gap between content and frame, so the background shines through:

lib/documentview/documentview.cpp
562 ↗(On Diff #31353)

On master this seems to be needed, however when rebasing this patch on top of D11795, I cannot really reproduce the problem.

If you can confirm, one update() less would be better, of course…

huoni updated this revision to Diff 31452.Apr 6 2018, 1:38 AM
huoni edited the summary of this revision. (Show Details)
  • Remove the now unnecessary update() in SvgImageView::onZoomChanged
huoni added a comment.Apr 6 2018, 1:38 AM

To keep the tradition of finding an opportunity for yet another follow-up patch, there is still this irritating gap between content and frame, so the background shines through:

I'm on it, as well as another small problem I noticed (comparison mode margin not removed when going back to single image mode).

lib/documentview/documentview.cpp
562 ↗(On Diff #31353)

I tried rebasing, but cannot confirm. I get the same artifacts shown in the commit message unless this update() is here.

However, the update() in SvgImageView::onZoomChanged() (which I added in D11796) is now unnecessary, so I've removed that instead.

rkflx added a comment.Apr 6 2018, 9:50 AM

Hm, this is really weird. I don't want to drag out this review further, but I cannot see the artifacts you are mentioning. In fact, even the update in slotZoomChanged does not seem to be necessary (see P192):

git checkout master
git pull
arc patch D11795
cd <source dir>
arc paste P192 | git apply

Perhaps I'm not testing properly. Could you upload a video where it fails for you, so I can try to replicate?

huoni added a comment.Apr 6 2018, 11:29 AM

Perhaps I'm not testing properly. Could you upload a video where it fails for you, so I can try to replicate?

Sure thing. I did:

git checkout master
arc patch D11795

I'm using Ctrl + two-finger touchpad scroll for zooming.

I followed the steps in D11942#241060 and tried different svg and png combinations but can't spot anything wrong. No artifacts as shown in the video above.
The selection rectangle always displays perfect like in this screenshot:

huoni added a comment.EditedApr 6 2018, 12:39 PM

Aha! I still had animations set to None. Changing back to Software/OpenGL fixes it.

Can you guys confirm this happens with no animations?

Can you guys confirm this happens with no animations?

That's it. ;) With config None I get random effects: no border, too narrow border, svg overlapping border while panning.

huoni added a comment.Apr 6 2018, 1:07 PM

svg overlapping border while panning.

That's actually a separate issue that happens with animations on as well. That's what setFlag(ItemClipsChildrenToShape) fixes.

I will look into this further and see if there's a better way to fix the glitches with animations off.

rkflx accepted this revision.Apr 6 2018, 1:13 PM

Ok, great. Might also be worth checking some of the other occurrences of update(). Nevertheless, if you find no better way, I'm also willing to accept the patch as-is, because with None I can reproduce now too.

This revision is now accepted and ready to land.Apr 6 2018, 1:13 PM
rkflx added a comment.Apr 6 2018, 1:15 PM

Ah, and please add a note to the commit message in that case.

huoni updated this revision to Diff 31539.Apr 7 2018, 12:21 AM
huoni retitled this revision from Fix selection rectangle artifacts in comparison mode after zooming to Fix SVGs overlapping selection rect in View compare mode.
huoni edited the summary of this revision. (Show Details)
  • Remove glitch fixes and focus on the SVG clipping

The glitches were unrelated and fixed in D11795 instead.

huoni edited the test plan for this revision. (Show Details)Apr 7 2018, 12:21 AM
huoni edited the test plan for this revision. (Show Details)Apr 7 2018, 9:56 PM
This revision was automatically updated to reflect the committed changes.
rkflx added a comment.Apr 7 2018, 10:42 PM

+1. I don't think we can make the patch any simpler :D