Changeset View
Standalone View
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 | | |||
40 | namespace Gwenview | 41 | namespace Gwenview | ||
41 | { | 42 | { | ||
42 | 43 | | |||
43 | /// SvgImageView //// | 44 | /// SvgImageView //// | ||
44 | SvgImageView::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) | ||||
50 | , mImageFullyLoaded(false) | ||||
47 | { | 51 | { | ||
52 | // So we aren't unnecessarily drawing the background for every paint() | ||||
53 | setCacheMode(QGraphicsItem::DeviceCoordinateCache); | ||||
rkflx: You can initialize those member variables the same way it's done for `mSvgItem` above. | |||||
48 | } | 54 | } | ||
49 | 55 | | |||
50 | void SvgImageView::loadFromDocument() | 56 | void SvgImageView::loadFromDocument() | ||
51 | { | 57 | { | ||
52 | Document::Ptr doc = document(); | 58 | Document::Ptr doc = document(); | ||
53 | GV_RETURN_IF_FAIL(doc); | 59 | GV_RETURN_IF_FAIL(doc); | ||
54 | 60 | | |||
55 | if (doc->loadingState() < Document::Loaded) { | 61 | if (doc->loadingState() < Document::Loaded) { | ||
Show All 13 Lines | 74 | if (zoomToFit()) { | |||
69 | setZoom(computeZoomToFit(), QPointF(-1, -1), ForceUpdate); | 75 | setZoom(computeZoomToFit(), QPointF(-1, -1), ForceUpdate); | ||
70 | } else if (zoomToFill()) { | 76 | } else if (zoomToFill()) { | ||
71 | setZoom(computeZoomToFill(), QPointF(-1, -1), ForceUpdate); | 77 | setZoom(computeZoomToFill(), QPointF(-1, -1), ForceUpdate); | ||
72 | } else { | 78 | } else { | ||
73 | mSvgItem->setScale(zoom()); | 79 | mSvgItem->setScale(zoom()); | ||
74 | } | 80 | } | ||
75 | applyPendingScrollPos(); | 81 | applyPendingScrollPos(); | ||
76 | completed(); | 82 | completed(); | ||
83 | mImageFullyLoaded = true; | ||||
77 | } | 84 | } | ||
78 | 85 | | |||
79 | void SvgImageView::onZoomChanged() | 86 | void SvgImageView::onZoomChanged() | ||
80 | { | 87 | { | ||
81 | mSvgItem->setScale(zoom()); | 88 | mSvgItem->setScale(zoom()); | ||
82 | adjustItemPos(); | 89 | adjustItemPos(); | ||
83 | update(); | 90 | update(); | ||
84 | } | 91 | } | ||
85 | 92 | | |||
86 | void SvgImageView::onImageOffsetChanged() | 93 | void SvgImageView::onImageOffsetChanged() | ||
87 | { | 94 | { | ||
88 | adjustItemPos(); | 95 | adjustItemPos(); | ||
89 | } | 96 | } | ||
90 | 97 | | |||
91 | void SvgImageView::onScrollPosChanged(const QPointF& /* oldPos */) | 98 | void SvgImageView::onScrollPosChanged(const QPointF& /* oldPos */) | ||
92 | { | 99 | { | ||
93 | adjustItemPos(); | 100 | adjustItemPos(); | ||
94 | } | 101 | } | ||
95 | 102 | | |||
96 | void SvgImageView::adjustItemPos() | 103 | void SvgImageView::adjustItemPos() | ||
97 | { | 104 | { | ||
98 | mSvgItem->setPos(imageOffset() - scrollPos()); | 105 | mSvgItem->setPos(imageOffset() - scrollPos()); | ||
106 | update(); | ||||
107 | } | ||||
108 | | ||||
109 | void SvgImageView::setAlphaBackgroundMode(AbstractImageView::AlphaBackgroundMode mode) | ||||
110 | { | ||||
111 | mAlphaBackgroundMode = mode; | ||||
112 | update(); | ||||
113 | } | ||||
114 | | ||||
115 | void SvgImageView::setAlphaBackgroundColor(const QColor& color) | ||||
116 | { | ||||
117 | mAlphaBackgroundColor = color; | ||||
118 | update(); | ||||
119 | } | ||||
120 | | ||||
121 | void SvgImageView::drawAlphaBackground(QPainter* painter) | ||||
122 | { | ||||
123 | const QRectF imageRect = QRectF(imageOffset(), visibleImageSize()); | ||||
124 | | ||||
rkflx: You missed a spot ;) | |||||
125 | switch (mAlphaBackgroundMode) { | ||||
126 | case AbstractImageView::AlphaBackgroundCheckBoard: | ||||
127 | painter->drawTiledPixmap(imageRect, alphaBackgroundTexture(), scrollPos()); | ||||
128 | break; | ||||
129 | case AbstractImageView::AlphaBackgroundSolid: | ||||
130 | painter->fillRect(imageRect, mAlphaBackgroundColor); | ||||
131 | break; | ||||
132 | default: | ||||
133 | Q_ASSERT(0); | ||||
134 | } | ||||
135 | } | ||||
136 | | ||||
137 | void SvgImageView::paint(QPainter* painter, const QStyleOptionGraphicsItem* /*option*/, QWidget* /*widget*/) | ||||
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… | |||||
138 | { | ||||
139 | if (mImageFullyLoaded) { | ||||
140 | drawAlphaBackground(painter); | ||||
141 | } | ||||
99 | } | 142 | } | ||
100 | 143 | | |||
101 | //// SvgViewAdapter //// | 144 | //// SvgViewAdapter //// | ||
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… | |||||
102 | struct SvgViewAdapterPrivate | 145 | struct SvgViewAdapterPrivate | ||
103 | { | 146 | { | ||
104 | SvgImageView* mView; | 147 | SvgImageView* mView; | ||
105 | }; | 148 | }; | ||
106 | 149 | | |||
107 | SvgViewAdapter::SvgViewAdapter() | 150 | SvgViewAdapter::SvgViewAdapter() | ||
108 | : d(new SvgViewAdapterPrivate) | 151 | : d(new SvgViewAdapterPrivate) | ||
109 | { | 152 | { | ||
110 | d->mView = new SvgImageView; | 153 | d->mView = new SvgImageView; | ||
111 | setWidget(d->mView); | 154 | setWidget(d->mView); | ||
112 | connect(d->mView, &SvgImageView::zoomChanged, this, &SvgViewAdapter::zoomChanged); | 155 | connect(d->mView, &SvgImageView::zoomChanged, this, &SvgViewAdapter::zoomChanged); | ||
113 | connect(d->mView, &SvgImageView::zoomToFitChanged, this, &SvgViewAdapter::zoomToFitChanged); | 156 | connect(d->mView, &SvgImageView::zoomToFitChanged, this, &SvgViewAdapter::zoomToFitChanged); | ||
114 | connect(d->mView, &SvgImageView::zoomToFillChanged, this, &SvgViewAdapter::zoomToFillChanged); | 157 | connect(d->mView, &SvgImageView::zoomToFillChanged, this, &SvgViewAdapter::zoomToFillChanged); | ||
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… | |||||
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); | 158 | connect(d->mView, &SvgImageView::zoomInRequested, this, &SvgViewAdapter::zoomInRequested); | ||
116 | connect(d->mView, &SvgImageView::zoomOutRequested, this, &SvgViewAdapter::zoomOutRequested); | 159 | connect(d->mView, &SvgImageView::zoomOutRequested, this, &SvgViewAdapter::zoomOutRequested); | ||
117 | connect(d->mView, &SvgImageView::scrollPosChanged, this, &SvgViewAdapter::scrollPosChanged); | 160 | connect(d->mView, &SvgImageView::scrollPosChanged, this, &SvgViewAdapter::scrollPosChanged); | ||
118 | connect(d->mView, &SvgImageView::completed, this, &SvgViewAdapter::completed); | 161 | connect(d->mView, &SvgImageView::completed, this, &SvgViewAdapter::completed); | ||
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); | 162 | connect(d->mView, &SvgImageView::previousImageRequested, this, &SvgViewAdapter::previousImageRequested); | ||
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); | 163 | connect(d->mView, &SvgImageView::nextImageRequested, this, &SvgViewAdapter::nextImageRequested); | ||
121 | connect(d->mView, &SvgImageView::toggleFullScreenRequested, this, &SvgViewAdapter::toggleFullScreenRequested); | 164 | connect(d->mView, &SvgImageView::toggleFullScreenRequested, this, &SvgViewAdapter::toggleFullScreenRequested); | ||
122 | } | 165 | } | ||
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… | |||||
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! huoni: All the above was a result of copying a similar structure from I believe `RasterImageView`. At… | |||||
123 | 166 | | |||
124 | SvgViewAdapter::~SvgViewAdapter() | 167 | SvgViewAdapter::~SvgViewAdapter() | ||
125 | { | 168 | { | ||
126 | delete d; | 169 | delete d; | ||
127 | } | 170 | } | ||
128 | 171 | | |||
129 | QCursor SvgViewAdapter::cursor() const | 172 | QCursor SvgViewAdapter::cursor() const | ||
130 | { | 173 | { | ||
131 | return widget()->cursor(); | 174 | return widget()->cursor(); | ||
132 | } | 175 | } | ||
133 | 176 | | |||
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… | |||||
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… | |||||
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… | |||||
134 | void SvgViewAdapter::setCursor(const QCursor& cursor) | 177 | void SvgViewAdapter::setCursor(const QCursor& cursor) | ||
135 | { | 178 | { | ||
136 | widget()->setCursor(cursor); | 179 | widget()->setCursor(cursor); | ||
137 | } | 180 | } | ||
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… | |||||
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… | |||||
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 | 181 | | |||
139 | void SvgViewAdapter::setDocument(Document::Ptr doc) | 182 | void SvgViewAdapter::setDocument(Document::Ptr doc) | ||
140 | { | 183 | { | ||
141 | d->mView->setDocument(doc); | 184 | d->mView->setDocument(doc); | ||
142 | } | 185 | } | ||
143 | 186 | | |||
144 | Document::Ptr SvgViewAdapter::document() const | 187 | Document::Ptr SvgViewAdapter::document() const | ||
145 | { | 188 | { | ||
146 | return d->mView->document(); | 189 | return d->mView->document(); | ||
147 | } | 190 | } | ||
148 | 191 | | |||
149 | void SvgViewAdapter::loadConfig() | 192 | void SvgViewAdapter::loadConfig() | ||
150 | { | 193 | { | ||
194 | d->mView->setAlphaBackgroundMode(GwenviewConfig::alphaBackgroundMode()); | ||||
195 | d->mView->setAlphaBackgroundColor(GwenviewConfig::alphaBackgroundColor()); | ||||
151 | d->mView->setEnlargeSmallerImages(GwenviewConfig::enlargeSmallerImages()); | 196 | d->mView->setEnlargeSmallerImages(GwenviewConfig::enlargeSmallerImages()); | ||
152 | } | 197 | } | ||
153 | 198 | | |||
154 | void SvgViewAdapter::setZoomToFit(bool on) | 199 | void SvgViewAdapter::setZoomToFit(bool on) | ||
155 | { | 200 | { | ||
156 | d->mView->setZoomToFit(on); | 201 | d->mView->setZoomToFit(on); | ||
157 | } | 202 | } | ||
158 | 203 | | |||
▲ Show 20 Lines • Show All 51 Lines • Show Last 20 Lines |
You can initialize those member variables the same way it's done for mSvgItem above.