Differential D11629 Diff 30767 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 19 Lines69{
75 applyPendingScrollPos(); 80​ applyPendingScrollPos();
76 completed(); 81​ completed();
77} 82​}
78 83​
79void SvgImageView::onZoomChanged() 84​void SvgImageView::onZoomChanged()
80{ 85​{
81 mSvgItem->setScale(zoom()); 86​ mSvgItem->setScale(zoom());
82 adjustItemPos(); 87​ adjustItemPos();
88​ update();
83} 89​}
84 90​
85void SvgImageView::onImageOffsetChanged() 91​void SvgImageView::onImageOffsetChanged()
86{ 92​{
87 adjustItemPos(); 93​ adjustItemPos();
88} 94​}
89 95​
90void SvgImageView::onScrollPosChanged(const QPointF& /* oldPos */) 96​void SvgImageView::onScrollPosChanged(const QPointF& /* oldPos */)
91{ 97​{
92 adjustItemPos(); 98​ adjustItemPos();
93} 99​}
94 100​
95void SvgImageView::adjustItemPos() 101​void SvgImageView::adjustItemPos()
96{ 102​{
97 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);
98} 138​}
99 139​
100//// 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…
101struct SvgViewAdapterPrivate 141​struct SvgViewAdapterPrivate
102{ 142​{
103 SvgImageView* mView; 143​ SvgImageView* mView;
104}; 144​};
105 145​
106SvgViewAdapter::SvgViewAdapter() 146​SvgViewAdapter::SvgViewAdapter()
107: d(new SvgViewAdapterPrivate) 147​: d(new SvgViewAdapterPrivate)
108{ 148​{
109 d->mView = new SvgImageView; 149​ d->mView = new SvgImageView;
110 setWidget(d->mView); 150​ setWidget(d->mView);
111 connect(d->mView, &SvgImageView::zoomChanged, this, &SvgViewAdapter::zoomChanged); 151​ connect(d->mView, &SvgImageView::zoomChanged, this, &SvgViewAdapter::zoomChanged);
112 connect(d->mView, &SvgImageView::zoomToFitChanged, this, &SvgViewAdapter::zoomToFitChanged); 152​ connect(d->mView, &SvgImageView::zoomToFitChanged, this, &SvgViewAdapter::zoomToFitChanged);
113 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…
114 connect(d->mView, &SvgImageView::zoomInRequested, this, &SvgViewAdapter::zoomInRequested); 154​ connect(d->mView, &SvgImageView::zoomInRequested, this, &SvgViewAdapter::zoomInRequested);
115 connect(d->mView, &SvgImageView::zoomOutRequested, this, &SvgViewAdapter::zoomOutRequested); 155​ connect(d->mView, &SvgImageView::zoomOutRequested, this, &SvgViewAdapter::zoomOutRequested);
116 connect(d->mView, &SvgImageView::scrollPosChanged, this, &SvgViewAdapter::scrollPosChanged); 156​ connect(d->mView, &SvgImageView::scrollPosChanged, this, &SvgViewAdapter::scrollPosChanged);
117 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…
118 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?
119 connect(d->mView, &SvgImageView::nextImageRequested, this, &SvgViewAdapter::nextImageRequested); 159​ connect(d->mView, &SvgImageView::nextImageRequested, this, &SvgViewAdapter::nextImageRequested);
120 connect(d->mView, &SvgImageView::toggleFullScreenRequested, this, &SvgViewAdapter::toggleFullScreenRequested); 160​ connect(d->mView, &SvgImageView::toggleFullScreenRequested, this, &SvgViewAdapter::toggleFullScreenRequested);
121} 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…
122 162​
123SvgViewAdapter::~SvgViewAdapter() 163​SvgViewAdapter::~SvgViewAdapter()
124{ 164​{
125 delete d; 165​ delete d;
126} 166​}
127 167​
128QCursor SvgViewAdapter::cursor() const 168​QCursor SvgViewAdapter::cursor() const
129{ 169​{
130 return widget()->cursor(); 170​ return widget()->cursor();
131} 171​}
132 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…
133void SvgViewAdapter::setCursor(const QCursor& cursor) 173​void SvgViewAdapter::setCursor(const QCursor& cursor)
134{ 174​{
135 widget()->setCursor(cursor); 175​ widget()->setCursor(cursor);
136} 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…
137 177​
138void SvgViewAdapter::setDocument(Document::Ptr doc) 178​void SvgViewAdapter::setDocument(Document::Ptr doc)
139{ 179​{
140 d->mView->setDocument(doc); 180​ d->mView->setDocument(doc);
141} 181​}
142 182​
143Document::Ptr SvgViewAdapter::document() const 183​Document::Ptr SvgViewAdapter::document() const
144{ 184​{
145 return d->mView->document(); 185​ return d->mView->document();
146} 186​}
147 187​
148void SvgViewAdapter::loadConfig() 188​void SvgViewAdapter::loadConfig()
149{ 189​{
190​ d->mView->setAlphaBackgroundMode(GwenviewConfig::alphaBackgroundMode());
191​ d->mView->setAlphaBackgroundColor(GwenviewConfig::alphaBackgroundColor());
150 d->mView->setEnlargeSmallerImages(GwenviewConfig::enlargeSmallerImages()); 192​ d->mView->setEnlargeSmallerImages(GwenviewConfig::enlargeSmallerImages());
151} 193​}
152 194​
153void SvgViewAdapter::setZoomToFit(bool on) 195​void SvgViewAdapter::setZoomToFit(bool on)
154{ 196​{
155 d->mView->setZoomToFit(on); 197​ d->mView->setZoomToFit(on);
156} 198​}
157 199​
▲ Show 20 Lines • Show All 46 Lines • Show Last 20 Lines