Enlarge small SVGs too
ClosedPublic

Authored by rkflx on Feb 28 2018, 7:51 PM.

Details

Summary

For images smaller than the viewport at 100% zoom, Gwenview allows to
scale up the image to Fit the window if Enlarge smaller images
is checked in the configuration dialog. So far this only worked for
raster-based images, but not for SVGs.

mEnlargeSmallerImages is meant to be handled by the abstract base
class AbstractImageView, however when f00cef3c0 moved code over there
from RasterImageView, it was missed to also adapt SvgImageView. This
can be fixed by implementing the missing function.

In addition, a left-over bool is removed.

BUG: 364822
FIXED-IN: 17.12.3

Test Plan
  • Open icons/hisc-apps-gwenview.svgz, press Fit: Small icon shown.
  • Check Enlarge smaller images, press Fit again: Icon scaled up to fit window.
  • Enlarging raster-based images still works fine.

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.
rkflx requested review of this revision.Feb 28 2018, 7:51 PM
rkflx created this revision.
rkflx added a subscriber: huoni.

@huoni BTW, setAlphaBackgroundColor and setAlphaBackgroundMode are also broken for SVGs, but not sure how easy that is to fix.

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

Patch looks good! No issues found.

@huoni BTW, setAlphaBackgroundColor and setAlphaBackgroundMode are also broken for SVGs, but not sure how easy that is to fix.

Could we implement RasterImageView::drawAlphaBackground in SvgImageView?

rkflx added a comment.Mar 1 2018, 12:35 AM

@huoni BTW, setAlphaBackgroundColor and setAlphaBackgroundMode are also broken for SVGs, but not sure how easy that is to fix.

Could we implement RasterImageView::drawAlphaBackground in SvgImageView?

Maybe, but it's probably more work than copy-and-paste. That's because for SVGs most of the work is done by QSvgRenderer, while drawAlphaBackground operates on QPainter level.

huoni accepted this revision.Mar 1 2018, 1:52 AM

Maybe, but it's probably more work than copy-and-paste. That's because for SVGs most of the work is done by QSvgRenderer, while drawAlphaBackground operates on QPainter level.

I figured there'd be something different about SVGs. I probably don't understand it properly, but could we just use a QPainter to draw the background behind the image? Currently the viewport background is visible through the transparent parts of an SVG, therefore it seems reasonable to me that it would be possible to draw another rectangle above the viewport but below the image.

This revision is now accepted and ready to land.Mar 1 2018, 1:52 AM
muhlenpfordt accepted this revision.Mar 1 2018, 8:31 AM
muhlenpfordt added a subscriber: muhlenpfordt.

Works as it should and adjust display immediately after changing the option.
I shortly thought about moving it back to AbstractImageView but there may be future derived views where it makes sense to ignore/separate this option - so it's fine here. :)

ngraham accepted this revision.Mar 1 2018, 2:38 PM
ngraham added a subscriber: ngraham.

Looks and works great! This always bugged me.

ngraham edited the summary of this revision. (Show Details)Mar 1 2018, 2:39 PM

Thanks for testing everyone, I don't think I ever had three green lights on a Gwenview patch before!

Works as it should and adjust display immediately after changing the option.
I shortly thought about moving it back to AbstractImageView but there may be future derived views where it makes sense to ignore/separate this option - so it's fine here. :)

I almost did that before submitting the patch, until I discovered EmptyAdapter.


QSvgRenderer

Discussion continues in T8125: Add support for configurable transparent background to SVGs.

This revision was automatically updated to reflect the committed changes.