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.
Details
- Reviewers
ngraham rkflx - Group Reviewers
Gwenview - Commits
- R260:787e4e33bd01: Fix selection rectangle for SVG images in View comparison mode
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.
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) |
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).
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. |
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.
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).
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! :)