use view lines for wheel scrolling, not real lines
ClosedPublic

Authored by brauch on Aug 12 2018, 8:37 AM.

Details

Summary

I don't think there is an ideal solution for this problem, this one has the disadvantage that the code handling wheel scrolling is not related to the scroll bar any more. It still seems like the simplest one. What do you think?

Diff Detail

Repository
R39 KTextEditor
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
brauch created this revision.Aug 12 2018, 8:37 AM
Restricted Application added projects: Kate, Frameworks. · View Herald TranscriptAug 12 2018, 8:37 AM
Restricted Application added subscribers: kde-frameworks-devel, kwrite-devel. · View Herald Transcript
brauch requested review of this revision.Aug 12 2018, 8:37 AM

Not yet thoroughly reviewed, but some thoughts:

  • There is a bug report for this: https://bugs.kde.org/show_bug.cgi?id=256561 Could you verify that the issues in the bug report are addressed by this patch? See also the comments by @ngraham in this bug report.
  • could you add: BUG: 256561
  • Since the commit logs now always appear in the KF5 changelog, maybe the subject could read: Fix: Scroll view lines instead of not real lines for wheel and touchpad scrolling

I will modify the commit message as suggested.

I read through the bug report, and I think this patch does fix the main issue discussed there. The patch suggested by one user there is even quite similar (if a bit less complete) than this one. The original complaint (touchpad vs. mouse wheel) to me personally sounds a bit strange and I'm not sure if that is actually the point. I'm not even sure if that is what the original reporter was in fact annoyed by (in contrast to "what he wrote he was annoyed by").

The "overly sensitive touchpad" issue seems to be missing accumulation of scroll events, so this patch to my understanding should not have it.

Thanks for the patch, Sven!

There are two issues with scrolling:

  1. Multi-line strings are inappropriately treated as a single line for the purposes of scrolling
  2. Lack of per-pixel scrolling that touchpad users expect.

This patch fixes #1, but not #2. I've untangles the Bugzilla tickets such that https://bugs.kde.org/show_bug.cgi?id=256561 now only tracks #1, so I think you're free to add BUG: 256561 to this patch. We will track pixel-by-pixel scrolling separately with https://bugs.kde.org/show_bug.cgi?id=378275.

The "overly sensitive touchpad" issue seems to be missing accumulation of scroll events, so this patch to my understanding should not have it.

Yeah, I don't think that's our bug, and we shouldn't work around it here. The reporter should file a bug against libinput.

The "overly sensitive touchpad" issue seems to be missing accumulation of scroll events, so this patch to my understanding should not have it.

Yeah, I don't think that's our bug, and we shouldn't work around it here. The reporter should file a bug against libinput.

Hmm, I understood this differently. I thought this happens when you react to each input event sent by the kernel, regardless of its delta, like e.g. the old gwenview code did which you linked: it just zooms in once per event. Thus devices which generate more events with smaller deltas zoom in faster, which is not good. And this bug is definitely at the application level, not in libinput.

dhaumann accepted this revision.Aug 13 2018, 8:23 AM

For me this works. It would be nice to get more feedback, but to be honest, I think we only get this feedback by integrating this patch and let the users test it. It's certainly not broken and fixes a usability issue we've had for a log time.

Please commit [and blog about it!].

This revision is now accepted and ready to land.Aug 13 2018, 8:23 AM

The "overly sensitive touchpad" issue seems to be missing accumulation of scroll events, so this patch to my understanding should not have it.

Yeah, I don't think that's our bug, and we shouldn't work around it here. The reporter should file a bug against libinput.

Hmm, I understood this differently. I thought this happens when you react to each input event sent by the kernel, regardless of its delta, like e.g. the old gwenview code did which you linked: it just zooms in once per event. Thus devices which generate more events with smaller deltas zoom in faster, which is not good. And this bug is definitely at the application level, not in libinput.

The Gwenview issue you're talking about manifested in touchpad scrolling feeling too fast. IMHO, touchpad scrolling in KTextEditor views feels perfect right now, so we don't have that particular issue. However yes, we should probably be taking into account the delta. But maybe that's material for another patch?

I will indeed blog about this. If you'd like, I can request that people specifically test it out and offer feedback.

I will indeed blog about this. If you'd like, I can request that people specifically test it out and offer feedback.

> I think that would be cool!

@ngraham If you blog about it, feel free to explicitly mention that this is hopefuly very useful for the LaTeX community using Kile.

+1, will do.

rjvbb added a subscriber: rjvbb.EditedAug 13 2018, 8:26 AM

I'll test the patch ASAP, but I think there's a simple guideline to observe here (apologies if it's as much an open door as I think it is):

Does scrolling work as it should when dynamic wrapping is off, with and/or without this patch (and that would include the accidental wheel-zoom protection)?

The issue related to touchpad scrolling should indeed be tracked elsewhere because it's bound to have a platform-specific aspect (while this one here shouldn't).

IMHO, touchpad scrolling in KTextEditor views feels perfect right now

I have no issues with it either - never had on Mac (still the reference for anything touchpad related in my experience ... but for that you need to have experienced the Magic Trackpad :)). On Linux ... let's say it depends on how you well you manage to calibrate the thing with synclient.

This revision was automatically updated to reflect the committed changes.