Implement zooming with Ctrl+mouse wheel
ClosedPublic

Authored by yurchor on Nov 23 2018, 10:06 AM.

Details

Summary

BUG: 159772

https://bugs.kde.org/show_bug.cgi?id=159772

Adopted patch by David Benjamin + some minor documentation.

Test Plan
  1. Plot something.
  2. Use Ctrl+mouse wheel to zoom in/out.
  3. Use toolbar or menu to zoom in/out.

Diff Detail

Repository
R334 KmPlot
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
yurchor created this revision.Nov 23 2018, 10:06 AM
Restricted Application added projects: KDE Edu, Documentation. · View Herald TranscriptNov 23 2018, 10:06 AM
Restricted Application added subscribers: kde-doc-english, kde-edu. · View Herald Transcript
yurchor requested review of this revision.Nov 23 2018, 10:06 AM
tcanabrava requested changes to this revision.Nov 23 2018, 10:32 AM
tcanabrava added a subscriber: tcanabrava.
tcanabrava added inline comments.
kmplot/view.cpp
3691–3712

It seems that you are interpolating by hand, why don't you try to use something like a QEasingCurve + a QPropertyAnimation?

kmplot/view.h
637–640

What's TL and BR? Be explicit in the code.

This revision now requires changes to proceed.Nov 23 2018, 10:32 AM
yurchor marked an inline comment as done.Nov 23 2018, 11:58 AM
yurchor added inline comments.
kmplot/view.cpp
3691–3712

Because switching to QPropertyAnimation cannot be done without complete rewriting half of KmPlot classes and breaks the whole current structure. There should be implemented some property of View to hold the current viewport to tie it up with QPropertyAnimation, then it should be connected with some timer and updates, and maybe other things, I'm not a skilled Qt programmer... It is easy to use QEasingCurve and QPropertyAnimation when the widget is ready for this. View is a different thing now. It might be the stunning implementation with 6 lines, but it is impossible now.

The proposed patch just works. People wanted this for more than 10 years (21 votes).

Sorry.

kmplot/view.h
637–640

"TopLeft" and "BottomRight". Fixed in the new version.

yurchor updated this revision to Diff 46073.Nov 23 2018, 12:02 PM
yurchor marked an inline comment as done.

Rename variables in a more clear manner.

tcanabrava accepted this revision.Nov 23 2018, 3:04 PM

repaint will trigger a reppaint forcefully, if you use update() are we skipping frames?

after this and the old style connect are working I'd give a +1.
what happens if you scroll up and down too quick for the screen to repaint?

kmplot/view.cpp
3709

old style connect.

This revision is now accepted and ready to land.Nov 23 2018, 3:04 PM
yurchor marked an inline comment as done.Nov 23 2018, 5:56 PM

repaint will trigger a reppaint forcefully, if you use update() are we skipping frames?

No.

after this and the old style connect are working I'd give a +1.
what happens if you scroll up and down too quick for the screen to repaint?

Values on the axes grow exponentially. It might happen that my computer is too fast for this testing (Intel Core 2 E4700) but all I can see is a series of rectangular borders moving in/out (f(x) = x^3 -3*x^2 with its first and second derivatives + integral, default scaling).

yurchor updated this revision to Diff 46084.Nov 23 2018, 5:58 PM

New style.

yurchor updated this revision to Diff 46085.Nov 23 2018, 6:02 PM

QEasingCurve + a QPropertyAnimation version. Short, compiles but does not work at all. I'd be grateful for the answer on why it does not work.

yurchor updated this revision to Diff 46124.Nov 24 2018, 3:27 PM

Revert to the working version without QEasingCurve and QPropertyAnimation.

yurchor updated this revision to Diff 46220.Nov 25 2018, 7:01 PM

Working patch with QEasingCurve and QPropertyAnimation.

Sorry for these constant changes. It seems that I'm not a very fast student. :'(

Can someone tell which of the proposed ways to fix the bug is better?

Thanks in advance for your advice.

aacid added a subscriber: aacid.Nov 26 2018, 10:59 PM
aacid added inline comments.
kmplot/view.cpp
3646

You either need to delete the existing one or not create one every single time otherwise we end up with lots of qpropertyanimations when we don't really need that many, no?

yurchor updated this revision to Diff 46301.Nov 27 2018, 6:58 AM

Do not create a new QPropertyAnimation every time when the animation is needed.

aacid added inline comments.Nov 28 2018, 10:49 PM
kmplot/view.cpp
3401

You may want to accumulate the deltas, some touchpads send lots of events with very small deltas, so just doing >0 and <0 can make it very difficult to control, see https://cgit.kde.org/okular.git/commit/?id=7a50ce0edfc9be8bd23441e52a4f3a0c60f7e60f

kmplot/view.h
86

What is this propety and the new functions for? They are never called, no?

cfeck added a subscriber: cfeck.Nov 28 2018, 10:55 PM
cfeck added inline comments.
kmplot/view.h
86

They are required for QPropertyAnimation

yurchor marked 3 inline comments as done.Nov 29 2018, 2:19 PM
  1. The property and functions are for QPropertyAnimation (proposed by Tomaz Canabrava to replace a good bunch of hand-written code by KmPlot authors), as it was already mentioned by Christoph Feck (thanks).
  1. I have tried to add QWheelEvent::DefaultDeltasPerStep threshold to mitigate potential unwanted behavior.
yurchor updated this revision to Diff 46472.Nov 29 2018, 2:21 PM

Add threshold to get rid of potential unwanted zooming on very sensitive touchpads.

aacid added a comment.Dec 1 2018, 11:11 PM

Add threshold to get rid of potential unwanted zooming on very sensitive touchpads.

You need to accumulate them, otherwise people where all the deltas are small will never get it to work.

Add threshold to get rid of potential unwanted zooming on very sensitive touchpads.

You need to accumulate them, otherwise people where all the deltas are small will never get it to work.

I have no idea how to do this. The example is not informative. Please advise.

yurchor updated this revision to Diff 46665.Dec 2 2018, 6:14 AM

Something along the lines, at least works with my mouse and does not break things on the common machines.

yurchor updated this revision to Diff 46823.Dec 4 2018, 5:12 AM

Trying to mimic zeroing of the accumulated scrolling with mouse button clicking in Okular.

aacid added a comment.Dec 4 2018, 11:07 PM

I *have not tried* it.

I think the code is in good enough shape now.

If you're confident with your testing is good enough, for me you can commit it (with the const fix)

If you want someone else to give it a test say so and i'll test it a bit.

kmplot/view.cpp
3689

this function should probably be const

yurchor marked an inline comment as done.Dec 5 2018, 5:33 AM

Many thanks for all reviews.

It would be great if someone can test the patch on a machine with touchpad. Regretfully, I have no notebook to test.

Should there be no testers and no objections I will try to push the patch to the repo in a week.

Many thanks in advance for your help.

yurchor updated this revision to Diff 46878.Dec 5 2018, 5:35 AM

Add const to getViewport().

aacid added a comment.Dec 5 2018, 11:31 PM

Many thanks for all reviews.

It would be great if someone can test the patch on a machine with touchpad. Regretfully, I have no notebook to test.

Dam something has changed and now mousemoveevent is also being called. It seems a Qt regression to me, but who's going to fix Qt... :D

Meh, that means that okular is also broken too (ok, not anymore, just fixed it in https://cgit.kde.org/okular.git/commit/?id=ba1c2beb70f0ebe3eed2ddda6709eabb2422e7ea )

If you apply https://paste.kde.org/p0s0avu28 it works fine for me using my laptop touchpad.

Should there be no testers and no objections I will try to push the patch to the repo in a week.

Many thanks in advance for your help.

yurchor updated this revision to Diff 46933.Dec 6 2018, 4:33 AM

Fixes by Albert (many thanks to him for testing).

This revision was automatically updated to reflect the committed changes.