Add Zoom scroll wheel option
ClosedPublic

Authored by chrissuran on Sep 28 2018, 5:50 AM.

Details

Summary

Adding a Zoom option for scrolling in the configuration

BUG: 254511
FIXED-IN: 18.12.0

Test Plan

Test scrolling in image and folder view for each configuration selection.

Diff Detail

Repository
R260 Gwenview
Branch
new-scroll
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 3415
Build 3433: arc lint + arc unit
chrissuran created this revision.Sep 28 2018, 5:50 AM
Restricted Application added a project: Gwenview. Β· View Herald TranscriptSep 28 2018, 5:50 AM
chrissuran requested review of this revision.Sep 28 2018, 5:50 AM

This would fix bug 254511

Wow, this works great! Did you use Qt Creator to change the .ui file, though? The diff is very large for such a small UI change. I'd prefer a hand-edited file so that the change is manageable to review.

That said, it all seems to work great, with both a mouse wheel and a touchpad. The zoom speed feels good. Code change seems sane to me.

Other Gwenview folks, whaddaya think?

Regardless, we need a few formatting changes to the diff here on Phabricator:

  • The title becomes the commit message, so please write it in the imperative, e.g. "Add zoom scroll wheel option"
  • Please change the Summary section to add:
BUG: 254511
FIXED-IN: 18.12.0

For information regarding why, see https://community.kde.org/Infrastructure/Phabricator#Formatting_your_patch

chrissuran updated this revision to Diff 42635.Oct 1 2018, 4:28 AM
  • Cleaned up imageviewconfigpage.ui
chrissuran retitled this revision from Adding Zoom scroll wheel option to Add Zoom scroll wheel option.Oct 1 2018, 4:45 AM
chrissuran edited the summary of this revision. (Show Details)
chrissuran removed a reviewer: Gwenview.

How do I push my latest commit to the remote branch? It seems to be replacing all my old commits with the most recent.

I tried to cleanup the imageviewconfigpage.ui but it only seemed to have replaced my diff, not added my latest commit to it.

chrissuran updated this revision to Diff 42636.Oct 1 2018, 5:01 AM

Includes all changes to the config and the code to enable the zoom option

Alright, I built a new branch and just commited all my changes to that, diff seems to look good

muhlenpfordt added a subscriber: muhlenpfordt.

Looks good to me and scrolling with Shift/Alt+wheel is still possible.
Just one thing I noticed - see inline comment.

lib/documentview/documentview.cpp
735–736

If MouseWheelBehavior::Zoom is active the zoom part is called no matter what canZoom() returns (e.g. for videos this is false). I can't see any problem ATM but maybe we should respect canZoom().

chrissuran updated this revision to Diff 42703.Oct 2 2018, 5:16 AM

Checking for canZoom() for all zoom logic

chrissuran marked an inline comment as done.Oct 2 2018, 4:39 PM

My latest change fixes the canZoom() issue

ngraham accepted this revision as: ngraham.Oct 2 2018, 4:41 PM

Excellent. Final review, @muhlenpfordt?

This revision is now accepted and ready to land.Oct 2 2018, 4:41 PM

No objections from me. πŸ™‚

This revision was automatically updated to reflect the committed changes.