Differential D11629 Diff 31246 lib/documentview/svgviewadapter.cpp

Changeset View

Standalone View

View Options

lib/documentview/svgviewadapter.cpp

Show All 20 Lines
21// Self 21​// Self
22#include "svgviewadapter.h" 22​#include "svgviewadapter.h"
23 23​
24// Qt 24​// Qt
25#include <QCursor> 25​#include <QCursor>
26#include <QGraphicsSvgItem> 26​#include <QGraphicsSvgItem>
27#include <QGraphicsTextItem> 27​#include <QGraphicsTextItem>
28#include <QGraphicsWidget> 28​#include <QGraphicsWidget>
29​#include <QPainter>
29#include <QSvgRenderer> 30​#include <QSvgRenderer>
30#include <QDebug> 31​#include <QDebug>
31 32​
32// KDE 33​// KDE
33 34​
34// Local 35​// Local
35#include "document/documentfactory.h" 36​#include "document/documentfactory.h"
36#include <qgraphicssceneevent.h> 37​#include <qgraphicssceneevent.h>
37#include <lib/gvdebug.h> 38​#include <lib/gvdebug.h>
38#include <lib/gwenviewconfig.h> 39​#include <lib/gwenviewconfig.h>
39 40​
40namespace Gwenview 41​namespace Gwenview
41{ 42​{
42 43​
43/// SvgImageView //// 44​/// SvgImageView ////
44SvgImageView::SvgImageView(QGraphicsItem* parent) 45​SvgImageView::SvgImageView(QGraphicsItem* parent)
45: AbstractImageView(parent) 46​: AbstractImageView(parent)
46, mSvgItem(new QGraphicsSvgItem(this)) 47​, mSvgItem(new QGraphicsSvgItem(this))
48​, mAlphaBackgroundMode(AbstractImageView::AlphaBackgroundCheckBoard)
49​, mAlphaBackgroundColor(Qt::black)
47{ 50​{
51​ // So we aren't unnecessarily drawing the background for every paint()
52​ setCacheMode(QGraphicsItem::DeviceCoordinateCache);
rkflxUnsubmitted
Done

You can initialize those member variables the same way it's done for mSvgItem above.

rkflx: You can initialize those member variables the same way it's done for `mSvgItem` above.
48} 53​}
49 54​
50void SvgImageView::loadFromDocument() 55​void SvgImageView::loadFromDocument()
51{ 56​{
52 Document::Ptr doc = document(); 57​ Document::Ptr doc = document();
53 GV_RETURN_IF_FAIL(doc); 58​ GV_RETURN_IF_FAIL(doc);
54 59​
55 if (doc->loadingState() < Document::Loaded) { 60​ if (doc->loadingState() < Document::Loaded) {
Show All 35 Lines
91void SvgImageView::onScrollPosChanged(const QPointF& /* oldPos */) 96​void SvgImageView::onScrollPosChanged(const QPointF& /* oldPos */)
92{ 97​{
93 adjustItemPos(); 98​ adjustItemPos();
94} 99​}
95 100​
96void SvgImageView::adjustItemPos() 101​void SvgImageView::adjustItemPos()
97{ 102​{
98 mSvgItem->setPos(imageOffset() - scrollPos()); 103​ mSvgItem->setPos(imageOffset() - scrollPos());
104​ update();
105​}
106​
107​void SvgImageView::setAlphaBackgroundMode(AbstractImageView::AlphaBackgroundMode mode)
108​{
109​ mAlphaBackgroundMode = mode;
110​ update();
111​}
112​
113​void SvgImageView::setAlphaBackgroundColor(const QColor& color)
114​{
115​ mAlphaBackgroundColor = color;
116​ update();
117​}
118​
119​void SvgImageView::drawAlphaBackground(QPainter* painter)
120​{
121​ const QRectF imageRect = QRectF(imageOffset(), visibleImageSize());
122​
rkflxUnsubmitted
Done

You missed a spot ;)

rkflx: You missed a spot ;)
123​ switch (mAlphaBackgroundMode) {
124​ case AbstractImageView::AlphaBackgroundCheckBoard:
125​ painter->drawTiledPixmap(imageRect, alphaBackgroundTexture(), scrollPos());
126​ break;
127​ case AbstractImageView::AlphaBackgroundSolid:
128​ painter->fillRect(imageRect, mAlphaBackgroundColor);
129​ break;
130​ default:
131​ Q_ASSERT(0);
132​ }
133​}
134​
135​void SvgImageView::paint(QPainter* painter, const QStyleOptionGraphicsItem* /*option*/, QWidget* /*widget*/)
rkflxUnsubmitted
Done

After you latest changes you can finally drop those curly braces, which are quite uncommon for switch blocks.

rkflx: After you latest changes you can finally drop those curly braces, which are quite uncommon for…
136​{
137​ drawAlphaBackground(painter);
99} 138​}
100 139​
101//// SvgViewAdapter //// 140​//// SvgViewAdapter ////
rkflxUnsubmitted
Done

In the config dialog, Apply has an immediate effect for raster images (good), but for SVGs you have to hit OK for the background to change (bad).

Possibly setAlphaBackgroundMode is called in this case, where for RasterImageView there is update, so you'd need something similar for SVGs?

rkflx: In the config dialog, {nav Apply} has an immediate effect for raster images (good), but for…
102struct SvgViewAdapterPrivate 141​struct SvgViewAdapterPrivate
103{ 142​{
104 SvgImageView* mView; 143​ SvgImageView* mView;
105}; 144​};
106 145​
107SvgViewAdapter::SvgViewAdapter() 146​SvgViewAdapter::SvgViewAdapter()
108: d(new SvgViewAdapterPrivate) 147​: d(new SvgViewAdapterPrivate)
109{ 148​{
110 d->mView = new SvgImageView; 149​ d->mView = new SvgImageView;
111 setWidget(d->mView); 150​ setWidget(d->mView);
112 connect(d->mView, &SvgImageView::zoomChanged, this, &SvgViewAdapter::zoomChanged); 151​ connect(d->mView, &SvgImageView::zoomChanged, this, &SvgViewAdapter::zoomChanged);
113 connect(d->mView, &SvgImageView::zoomToFitChanged, this, &SvgViewAdapter::zoomToFitChanged); 152​ connect(d->mView, &SvgImageView::zoomToFitChanged, this, &SvgViewAdapter::zoomToFitChanged);
114 connect(d->mView, &SvgImageView::zoomToFillChanged, this, &SvgViewAdapter::zoomToFillChanged); 153​ connect(d->mView, &SvgImageView::zoomToFillChanged, this, &SvgViewAdapter::zoomToFillChanged);
rkflxUnsubmitted
Not Done

As mentioned in T8125, I'm not sure hasAlphaChannel makes sense for SVGs. Perhaps only mention that you'll draw the background for every SVG, or even remove the whole comment.

rkflx: As mentioned in T8125, I'm not sure `hasAlphaChannel` makes sense for SVGs. Perhaps only…
rkflxUnsubmitted
Done

In theory, background is needed only for a single case, so it can be moved there. However, we won't need it at all ;)

rkflx: In theory, `background` is needed only for a single `case`, so it can be moved there. However…
115 connect(d->mView, &SvgImageView::zoomInRequested, this, &SvgViewAdapter::zoomInRequested); 154​ connect(d->mView, &SvgImageView::zoomInRequested, this, &SvgViewAdapter::zoomInRequested);
116 connect(d->mView, &SvgImageView::zoomOutRequested, this, &SvgViewAdapter::zoomOutRequested); 155​ connect(d->mView, &SvgImageView::zoomOutRequested, this, &SvgViewAdapter::zoomOutRequested);
117 connect(d->mView, &SvgImageView::scrollPosChanged, this, &SvgViewAdapter::scrollPosChanged); 156​ connect(d->mView, &SvgImageView::scrollPosChanged, this, &SvgViewAdapter::scrollPosChanged);
118 connect(d->mView, &SvgImageView::completed, this, &SvgViewAdapter::completed); 157​ connect(d->mView, &SvgImageView::completed, this, &SvgViewAdapter::completed);
rkflxUnsubmitted
Done

As you are using this variable only a single time and it is constructed by a simple function call, you could just use the function inline, saving an extra copy.

rkflx: As you are using this variable only a single time and it is constructed by a simple function…
119 connect(d->mView, &SvgImageView::previousImageRequested, this, &SvgViewAdapter::previousImageRequested); 158​ connect(d->mView, &SvgImageView::previousImageRequested, this, &SvgViewAdapter::previousImageRequested);
rkflxUnsubmitted
Done

Why write this complicated construct when you could simply write scrollPos() inline?

rkflx: Why write this complicated construct when you could simply write `scrollPos()` inline?
120 connect(d->mView, &SvgImageView::nextImageRequested, this, &SvgViewAdapter::nextImageRequested); 159​ connect(d->mView, &SvgImageView::nextImageRequested, this, &SvgViewAdapter::nextImageRequested);
121 connect(d->mView, &SvgImageView::toggleFullScreenRequested, this, &SvgViewAdapter::toggleFullScreenRequested); 160​ connect(d->mView, &SvgImageView::toggleFullScreenRequested, this, &SvgViewAdapter::toggleFullScreenRequested);
122} 161​}
rkflxUnsubmitted
Done

Why do you need the second painter? The following works just fine for me:

painter->drawTiledPixmap(imageRect, alphaBackgroundTexture(), scrollPos());
rkflx: Why do you need the second painter? The following works just fine for me: painter…
huoniAuthorUnsubmitted
Not Done

All the above was a result of copying a similar structure from I believe RasterImageView. At the time of course I'm just trying to get things to work, and not thinking too deeply. Luckily your awesome reviews catch it!
I think I'll try to review my own work so to speak before submitting in future, to hopefully catch a lot of these things.

huoni: All the above was a result of copying a similar structure from I believe `RasterImageView`. At…
123 162​
124SvgViewAdapter::~SvgViewAdapter() 163​SvgViewAdapter::~SvgViewAdapter()
125{ 164​{
126 delete d; 165​ delete d;
127} 166​}
128 167​
129QCursor SvgViewAdapter::cursor() const 168​QCursor SvgViewAdapter::cursor() const
130{ 169​{
131 return widget()->cursor(); 170​ return widget()->cursor();
132} 171​}
133 172​
rkflxUnsubmitted
Done

I'm afraid neither the compiler nor the machine running the code will care much about your comment ;)

In general for situations which "should never happen" you can Q_ASSERT(0); which kills Gwenview (at least in Debug mode) and thus makes the problem visible.

rkflx: I'm afraid neither the compiler nor the machine running the code will care much about your…
huoniAuthorUnsubmitted
Done

I thought it was good practice to always have a default clause, to communicate that you've considered all cases.

Would it be preferable to put Q_ASSERT(0) here instead, or just take out default?

huoni: I thought it was good practice to always have a `default` clause, to communicate that you've…
rkflxUnsubmitted
Done

Well, assume you add an entry to the enum. Now both the compiler and static analysis tools will see your default and assume this was a deliberate choice, i.e. you'll not get notified that probably you should add the new case here too.

When testing I expected to get a compiler warning when there were unhandled cases (i.e. when removing all but the first case in your example), but this somehow did not work for me (not sure why).

In other places in Gwenview the ASSERT is used inside the default, so I'd say just do it this way.

rkflx: Well, assume you add an entry to the `enum`. Now both the compiler and static analysis tools…
134void SvgViewAdapter::setCursor(const QCursor& cursor) 173​void SvgViewAdapter::setCursor(const QCursor& cursor)
135{ 174​{
136 widget()->setCursor(cursor); 175​ widget()->setCursor(cursor);
137} 176​}
rkflxUnsubmitted
Done

Adding qDebug(), this method and thus SvgImageView::drawAlphaBackground is called a crazy number of times compared to RasterImageView::updateFromScaler, i.e. very often when displaying the image for the first time and also each time the cursor enters the viewport.

I wonder what's done differently for raster images to prevent this (perhaps via QGraphicsView's architecture?). Also, glancing at the API docs I discovered paint and setCacheMode for QGraphicsItem, not sure if this will help.

rkflx: Adding `qDebug()`, this method and thus `SvgImageView::drawAlphaBackground` is called a crazy…
huoniAuthorUnsubmitted
Not Done

Looks like caching works here, as long as we manually call update() when the background needs changing (on zoom/pan).

Note that RasterImageView::paint is also called numerous times, but it's only drawing mCurrentBuffer. We might be able to copy that behaviour, and implement a buffer here, but I think caching is the better option.

huoni: Looks like caching works here, as long as we manually call `update()` when the background needs…
rkflxUnsubmitted
Not Done

I'd assume that drawTilePixmap is not that much more expensive than drawPixmap of a potential SvgImageView::mCurrentBuffer, so the current version with single occasional repaints should be fine. Great setCacheMode is working out for us, it was quite a long shot…

rkflx: I'd assume that `drawTilePixmap` is not that much more expensive than `drawPixmap` of a…
138 177​
139void SvgViewAdapter::setDocument(Document::Ptr doc) 178​void SvgViewAdapter::setDocument(Document::Ptr doc)
140{ 179​{
141 d->mView->setDocument(doc); 180​ d->mView->setDocument(doc);
142} 181​}
143 182​
144Document::Ptr SvgViewAdapter::document() const 183​Document::Ptr SvgViewAdapter::document() const
145{ 184​{
146 return d->mView->document(); 185​ return d->mView->document();
147} 186​}
148 187​
149void SvgViewAdapter::loadConfig() 188​void SvgViewAdapter::loadConfig()
150{ 189​{
190​ d->mView->setAlphaBackgroundMode(GwenviewConfig::alphaBackgroundMode());
191​ d->mView->setAlphaBackgroundColor(GwenviewConfig::alphaBackgroundColor());
151 d->mView->setEnlargeSmallerImages(GwenviewConfig::enlargeSmallerImages()); 192​ d->mView->setEnlargeSmallerImages(GwenviewConfig::enlargeSmallerImages());
152} 193​}
153 194​
154void SvgViewAdapter::setZoomToFit(bool on) 195​void SvgViewAdapter::setZoomToFit(bool on)
155{ 196​{
156 d->mView->setZoomToFit(on); 197​ d->mView->setZoomToFit(on);
157} 198​}
158 199​
▲ Show 20 Lines • Show All 51 Lines • Show Last 20 Lines