Fix selection rectangle for SVG images in View comparison mode
ClosedPublic

Authored by huoni on Mar 30 2018, 3:11 AM.

Details

Summary

For SVG images, the selection rect encompassed the entire bounding
rect. This patch implements SvgViewAdapter::visibleDocumentRect
so it returns the correct rect instead of falling back to the generic
implementation in AbstractDocumentViewAdapter.

Before

After

Test Plan

Open at least one SVG in View comparison mode, and select the SVG.

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 30 2018, 3:11 AM
huoni created this revision.
huoni added a comment.Mar 30 2018, 3:16 AM
This comment was removed by huoni.
huoni updated this revision to Diff 30897.Mar 30 2018, 4:13 AM
  • Fix graphics artifacts when zooming
huoni edited the summary of this revision. (Show Details)Mar 30 2018, 4:15 AM
huoni added a reviewer: Gwenview.
huoni added inline comments.
lib/documentview/svgviewadapter.cpp
83

This is needed to get rid of artifacts of the select rect left over when zooming an SVG.

(There are other artifacts from the selection rect that are pre-existing and minor therefore I'm not addressing them here)

huoni edited the test plan for this revision. (Show Details)Mar 30 2018, 6:38 AM
ngraham accepted this revision.Mar 31 2018, 3:41 PM
ngraham added a subscriber: ngraham.

Makes sense to me. Works as advertised, and the code looks sane. No regressions seen when browsing SVGs outside of comparison mode, or when comparing non-SVGs, or when zooming either kind of image.

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

Makes sense to me. Works as advertised, and the code looks sane. No regressions seen when browsing SVGs outside of comparison mode, or when comparing non-SVGs, or when zooming either kind of image.

Thanks for reviewing! Might leave this up for a little bit before landing (Easter weekend and all).

rkflx accepted this revision.Apr 2 2018, 7:43 AM
rkflx added a subscriber: rkflx.

Thanks, this is a straightforward patch, mirroring the implementation in RasterImageView.

I thought about how to de-duplicate the code again, but given how *ViewAdapter duplicates so much anyway, I guess we'll just keep it as-is for now.


A pre-existing problem, but maybe worth a follow-up patch is the SVG overlapping the selection retangle once you zoom in and pan around:

lib/documentview/svgviewadapter.cpp
83

Nice, as far as I can see RasterImageView is doing something similar.

huoni added a comment.Apr 3 2018, 5:58 AM

I thought about how to de-duplicate the code again, but given how *ViewAdapter duplicates so much anyway, I guess we'll just keep it as-is for now.

I also considered this, but I think we'd need generic adapter for images that RasterImageViewAdapter and SvgViewAdapter both inherit.
Could be worth doing? (in a separate patch)

A pre-existing problem, but maybe worth a follow-up patch is the SVG overlapping the selection retangle once you zoom in and pan around:

Also noticed this, will work on it.

This revision was automatically updated to reflect the committed changes.
rkflx added a comment.Apr 4 2018, 11:04 PM

I thought about how to de-duplicate the code again, but given how *ViewAdapter duplicates so much anyway, I guess we'll just keep it as-is for now.

I also considered this, but I think we'd need generic adapter for images that RasterImageViewAdapter and SvgViewAdapter both inherit.
Could be worth doing? (in a separate patch)

Looked into this a bit:

It seems both classes are nearly identical, so perhaps instead of introducing yet another intermediate abstraction both classes could be merged to some kind of ImageViewAdapter, which would only need a new way to inject either a RasterImageView or an SvgImageView upon construction, as well as some changes to the *Private classes. However, this can get hairy very fast, because this may also require some refactoring in AbstractImageView to move some of the set* functions there.

I'd say understanding and streamlining some of the class hierarchies could be a good long-term goal, but currently I guess there are more interesting and more effective areas to work on, e.g. HiDPI (although I don't want to discourage you in any way if you really want to tackle this).

huoni added a comment.Apr 5 2018, 6:20 AM

A pre-existing problem, but maybe worth a follow-up patch is the SVG overlapping the selection retangle once you zoom in and pan around:

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! D11942

(Sorry for giving you so many patches to review! The downside of being such a great reviewer! :)