Have not tested both yet, but I was wondering whether/how this is related to D7744? The linked bugs suggest some relation, not sure in what direction. Maybe you could clarify that (also for future readers of the commit message, perhaps).

https://phabricator.kde.org/D7744 concerned scroll/zoom *sensitivity*. That is to say, how far your fingers have to move to generate a scroll/zoom "tick".

This patch concerns scroll/zoom speed: how much gets scrolled/zoomed per scroll tick.

Without either patch, touchpad zooming was unusable. With only https://phabricator.kde.org/D7744, it was much improved, but you still don't have much control at high zoom levels. With both patches, touchpad scrolling/zooming is a pleasure to use and offers high control. Scroll wheel zooming is improved, too.

Thanks for the explanation, makes sense and now I know what to look for. As I have no touchpad in reach, I cannot test this patch in combination with the other patch, but I hope you did test with a real touchpad?

I agree the current behaviour is not ideal, but I fear the patch makes zooming too slow for high zoom levels (only tested with a scroll wheel). Here is how others are doing it:

• Gimp: (33% )→ 50% → (66%) → 100% → (150%) → 200% → (300%) → 400% → (550%) → 800% → …
• Inkscape: (35%) → 50% → (71%) → 100% → (141%) → 200% → (283%) → 400% → (566%) → 800% → …
• Digikam: (35%) → (42%) → 50% → (59%) → (71%) → (85%) → 100% → (120%) → (144%) → (173%) → 207% → (249%) → …
• Krita: (33%) → 50% → (67%) → 100% → (141%) → 200% → (283%) → 400% → (566%) → 800% → …

I would suggest not to reinvent our own thing, but just to double the zoom level every two scroll ticks with another level in between like other graphics apps are doing (preferably like Inkscape or Krita). We could even look at their code (not sure if this needs adapting for Gwenview, though, considering our default zoom-to-fit).

For touchpads (and touchscreens), the ideal solution would be to zoom continuously like on multi-touch devices, not relying on the scroll wheel emulation. However, that's out of scope for this patch.

Just to clarify (after re-reading your comment): I'm not suggesting you have to implement continuous zooming (you can try to in addition to the improvements for scroll wheel users in this patch, of course). All I'm saying is this patch should change the zoom behaviour from a 100% increment per tick to doubling the zoom level every second (or third, if you like this better) tick, instead of just changing the 100% to a 25%.

How about the following?
Implements the zoom as Inkscape and Krita does: for more than 100% the zoom is increased by sqrt(2); for less than 100% the zoom is decreased by sqrt(0.5).

The min zoom also changes when the Gwenview window gets resized (it didn't and I can see this behavior as /buggy/). I had to override the resizeEvent for that.

diff --git a/lib/documentview/documentview.cpp b/lib/documentview/documentview.cpp
index 5e131d8..3965a28 100644
--- a/lib/documentview/documentview.cpp
+++ b/lib/documentview/documentview.cpp
@@ -263,18 +263,19 @@ struct DocumentViewPrivate
         qreal min = q->minimumZoom();
 
         mZoomSnapValues.clear();
-        if (min < 1.) {
+        for (qreal zoom = sqrt(0.5); zoom >= min; zoom *= sqrt(0.5))
+            mZoomSnapValues << zoom;
+
+        if (!mZoomSnapValues.isEmpty() && mZoomSnapValues.back() > min)
             mZoomSnapValues << min;
-            for (qreal invZoom = 16.; invZoom > 1.; invZoom /= 2.) {
-                qreal zoom = 1. / invZoom;
-                if (zoom > min) {
-                    mZoomSnapValues << zoom;
-                }
-            }
-        }
-        for (qreal zoom = 1; zoom <= MAXIMUM_ZOOM_VALUE ; zoom += 1.) {
+
+        std::reverse(mZoomSnapValues.begin(), mZoomSnapValues.end());
+
+        for (qreal zoom = 1; zoom <= MAXIMUM_ZOOM_VALUE; zoom *= sqrt(2))
             mZoomSnapValues << zoom;
-        }
+
+        if (mZoomSnapValues.back() < MAXIMUM_ZOOM_VALUE)
+            mZoomSnapValues << MAXIMUM_ZOOM_VALUE;
 
         q->minimumZoomChanged(min);
     }
@@ -572,6 +573,14 @@ qreal DocumentView::zoom() const
     return d->mAdapter->zoom();
 }
 
+void DocumentView::resizeEvent(QGraphicsSceneResizeEvent *event)
+{
+    Q_UNUSED(event);
+
+    d->resizeAdapterWidget();
+    d->updateZoomSnapValues();
+}
+
 void DocumentView::wheelEvent(QGraphicsSceneWheelEvent* event)
 {
     if (d->mAdapter->canZoom() && event->modifiers() & Qt::ControlModifier) {
diff --git a/lib/documentview/documentview.h b/lib/documentview/documentview.h
index e6ecda5..c6bac4c 100644
--- a/lib/documentview/documentview.h
+++ b/lib/documentview/documentview.h
@@ -197,6 +197,7 @@ Q_SIGNALS:
 protected:
     void paint(QPainter* painter, const QStyleOptionGraphicsItem* option, QWidget* widget = 0) Q_DECL_OVERRIDE;
 
+    void resizeEvent(QGraphicsSceneResizeEvent* event) Q_DECL_OVERRIDE;
     void wheelEvent(QGraphicsSceneWheelEvent* event) Q_DECL_OVERRIDE;
     void contextMenuEvent(QGraphicsSceneContextMenuEvent* event) Q_DECL_OVERRIDE;
     bool sceneEventFilter(QGraphicsItem*, QEvent*) Q_DECL_OVERRIDE;

That looks great, @vtasoulas. With that diff, this is basically your patch now. Wanna submit it for review?

Is there any way to update the current diff?

I tried but I was only getting the option to create a new revision.

In any case, feel free to update the current revision since this patch is related to this thread :)

@vtasoulas, since this is basically your patch, I'd like to mark you as the author. Is cyberang3l@gmail.com still a good email address for you?

Yes, this is a valid email address that you can use. Thank you :)

I see elsewhere you're using vangelis@tasoulas.net. Would you prefer that?

I have one more suggest, sqrt is a function which will be called in every iteration so just precalculate steps

const auto minStep = sqrt(0.5);
const auto maxStep = sqrt(2.0);

@ngraham yes, vangelis@tasoulas.net is my primary e-mail, but my KDE identity still uses the cyberang3l@gmail.com e-mail (which is still valid).

So if there is any need to correlate the e-mail with the KDE identiy then use the cyberang3l email. Otherwise, I prefer the vangelis@tasoulas.net.

The referenced bug is more a wishlist, so I suspect that this is more for master than the stable branch.

The referenced bug is more a wishlist

Well, the minimum value not updating is more of a bug 😛
Anyway, probably it's better not to change the zoom behaviour too much for a bugfix release. @ngraham should decide whether it works good enough for touchpad users with only D7744, or if this patch is also required immediately.

What I stumbled upon was the Toolbutton not setting its state to disabled for the max and min values.

Interim summary: There are two issues:

• Moving the slider with the mouse disables the buttons only when changing sliding direction again. – I have a patch eliminating this lag, will post it later.
• Triggering the "zoom out" action does not actually reach the minimum value (you need the mouse or the cursor keys for that). This needs more investigation and a separate patch, but I suspect this should also fix the window resizing case.

IOW, let's not care about the buttons not disabling in this patch and just remove resizeAdapterWidget for now (it would fix the issue, but only for resizing and not the general case, i.e. that would be a hack and not a fix).

Let's keep this patch focused on changing the zoom increments, and make another separate patch to address the zoom buttons not disabling properly when dragging with slider.

Once reviews are finished, if nobody objects, I'll land this on the 17.08 branch and merge to master.

In D7972#153354, @ngraham wrote:

Remove unnecessary DocumentView::resizeEvent() function

I'm afraid you removed too much (resizeEvent instead of only resizeAdapterWidget), so now updateZoomSnapValues is missing. In "Fit" mode after increasing window width, this would result in the slider showing you can zoom out further when in reality you cannot. Here is what I imagined:

void DocumentView::resizeEvent(QGraphicsSceneResizeEvent *event)
{
    d->updateZoomSnapValues();
    QGraphicsWidget::resizeEvent(event);
}

The toolbuttons not disabling are a different issue, of course.

Whoops, my bad. I didn't test the fit-then-resize case. Restored.

Excellent. @gateau?

@ngraham Apparently you can choose to "Continue anyway" when arc land complains. Given all changes Aurélien requested were implemented, I'd say this can be landed. Maybe wait until Sunday, though (to allow for a week of review time).

Sounds good.

@gateau, all good now?

Will commit on Oct 21 if I don't hear from @gateau before then, since I've addressed his remaining comments.

In D7972#153132, @ngraham wrote:

@vtasoulas, since this is basically your patch, I'd like to mark you as the author. Is cyberang3l@gmail.com still a good email address for you?

Too bad we cannot have multiple authors, as both Vangelis and Nate contributed a lot to the successful merging of this patch. Anyway, thank you to both of you, despite what the commit says now (or will if it is changed again?).

I'll commit my depending patch tomorrow.

Oh dang it, I forgot to mark @vtasoulas as the author. :-/ Yeah, I agree that joint authorship would have been ideal here, but as a consolation prize to Vangelis, since I'm marked as the author, I own it now and when it breaks, it's my fault! ;-)

@ngraham no worries about the authorship. You did at least as much for this patch, if not more when you include all the administrative work to get this patch landed :)