Kinetic scrolling in brush presets
ClosedPublic

Authored by poke1024 on Sep 27 2017, 4:29 PM.

Details

Summary

Another cosmetic thing I'd love to see in Krita.

This enables kinetic scrolling the brush presets dock/window, i.e. you can just click and drag to vertically scroll up or down your brushes. It also hides the scroll bar.

Since this is not what all users want, this probably would have to be guarded using a setting. Still, I like this very much and hope someone else of you does too :-)

Diff Detail

Repository
R37 Krita
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
poke1024 created this revision.Sep 27 2017, 4:29 PM

I think it would make more sense for this to be enabled for touch only, or maybe also tablet, but this is just my opinion.

rempt added a subscriber: rempt.Sep 28 2017, 8:11 AM

Yes, it should be an option -- I would like it on my mobile studio pro, but probably not on my desktop :-)

Though I personally really find this useful working on a Cintiq with a stylus and not having to grab the scollbar.

rempt added a comment.Sep 28 2017, 9:53 AM

Yes -- I agree with that. So, let's put a setting in the options dialog and push this :-)

dkazakov requested changes to this revision.Oct 3 2017, 2:32 PM
dkazakov added a subscriber: dkazakov.

I have asked a friend painter about that, he said the following:

  1. It should be optional
  2. Drag distance should be configurable. When we checked this patch with a tablet, in 30% of the cases the click was recognized as a drag :(

Please add the options, then we could even think about activating this option by default somehow :)

This revision now requires changes to proceed.Oct 3 2017, 2:32 PM
poke1024 updated this revision to Diff 20372.Oct 5 2017, 4:51 PM

Some ideas for having options for this:

The combo box allows for "Disabled" (classic mode with scrollbar), "On Touch Drag" (i.e. touch and drag) and "On Click Drag" (i.e. click and drag with a mouse).

Would it be too much to ask for an option to just use the default scroll behaviour without changing any parameters, which is supposed to emulate the platform behaviour (according to the Qt documentation)?

@alvinhochun That's what "disabled" should do. Or do you have something in mind that is different from the current top of tree?

No, I mean enabling the QScroller but not setting any of the QScrollerProperties explicitly.

libs/widgets/KoResourceItemChooser.cpp
549

indentation

The patch basically works fine. The only question I have is: in "On Touch Drag" mode, should the scroll bar be visible or not?

As far as I can see, in this mode, the user loses an ability to scroll the widget (unless he uses mouse wheel, not available on tablets). So, perhaps, we should still show a scroll bar in "On Touch Drag" gesture mode?

dkazakov requested changes to this revision.Oct 23 2017, 2:11 PM

I'll mark the patch as needs changes, so it would not show up in the must review list.

From my side, the only blocker of the patch is enabling the scrollbar in "On Touch Drag". Otherwise the user loses an ability to scroll at all.

And what about you, @alvinhochun?

This revision now requires changes to proceed.Oct 23 2017, 2:11 PM
poke1024 marked an inline comment as done.Oct 23 2017, 2:35 PM

The scrollbar on "On Touch Drag" is a simple change.

Alvin's "stick to default QScrollerProperties" on the other hand would need additional ui and one more bool setting that disables the custom sensibility settings. I can see the point, but it's work. And, at least on OS X, QScrollerProperties's default behavior is not native at all, it's really some QT internal implementation that feels weird to me. And yet, on some platforms it might be what one wants. It's really the ui settings stuff that needs more work if we want such a setting here.

The "using default properties" thing is moot. After checking the Qt source code (https://github.com/qt/qtbase/blob/5.9/src/widgets/util/qscrollerproperties.cpp#L55) it doesn't actually use anything platform-specific. You can leave it out then :)

I finally get to test this patch. There is one issue that I found on Windows: When set to "On Touch Drag", scrolling with touchscreen would repeatedly switch the selected preset. The scrolling is pretty sluggish too. It might have something to do with the synthesized mouse move events. It seems kind of pointless to change the selection on mouse move, so would it be a good idea to just disable this behaviour?
(Just an additional note, this also happens on Windows 1709 with pen actions which causes Windows to synthesize a touch event.)

And here are some subjective views:

  • I would prefer the scroll bar to be visible all the time even if it's set to "On Click Drag".
  • The overshoot feedback doesn't feel nice (it just go on and on with no resistance). I prefer some resistance when the scrolling reaches an end (which is also the native behaviour on Windows). (I also would like the overshoot behaviour to activate even when the list isn't scrollable, but even Windows is pretty inconsistent with this.)

Perhaps you can try setting some of the properties based on the Qt defaults linked above?

I suppose QScroller dows not support grabbing only touch and tablet events excluding mouse events? That's a bit of a shame, but I suppose it's not very often for one to use a stylus together with a mouse so it's noto really a big deal.

I think it would be sensible to enable it "On Touch Drag" by default if the issues I had on Windows can be fixed.

alvinhochun requested changes to this revision.Oct 23 2017, 5:51 PM
poke1024 updated this revision to Diff 21249.Oct 24 2017, 5:20 PM

This tries to address some of the issues brought up by Dmitry and Alvin.

There is now a checkbox called "Show scrollbar" which is on by default. But you can turn it off to hide the scrollbar, if you want (like me).

Overshoot is now always on, even if no scrolling available. Overshooting is now much less sloppy than before.

Concerning the touch/click drag conflicts Alvin reported; I fear there is nothing simple we can do. We might want to report a QT bug, as this sounds like one to me (touch events shouldn't get processed and then synthesized as mouse events, but it might be some nasty bug inside QGestureManager, where some inputs get passed through as touch and others get synthesized to mouse events, just guessing).

Anyway, QScroller only offers us QScroller::ScrollerGestureType for configuring the input type, which has only 4 options: QScroller::TouchGesture, QScroller::LeftMouseButtonGesture, QScroller::MiddleMouseButtonGesture, QScroller::RightMouseButtonGesture. That's it.

alvinhochun added a comment.EditedOct 24 2017, 5:59 PM

Concerning the touch/click drag conflicts Alvin reported; I fear there is nothing simple we can do. We might want to report a QT bug, as this sounds like one to me (touch events shouldn't get processed and then synthesized as mouse events, but it might be some nasty bug inside QGestureManager, where some inputs get passed through as touch and others get synthesized to mouse events, just guessing).

I don't think this is a bug in Qt. The mouse events are likely synthesized by Windows so Qt cannot just block them because they don't necessarily map to the touch events 1:1. But Qt can identify it and should set QMouseEvent::source() to Qt::MouseEventSynthesizedBySystem. (Why Qt doesn't just block them all and do its own mouse event synthesizing though, I'm not sure.)

Perhaps we can install an event filter to filter out any mouse events sent to KoResourceItemView when the QScroller is scrolling from touch gesture.

I have tested the patch on Linux without touch, and from my side everything works fine :)
Now it is @alvinhochun who makes the decision :)

@poke1024 Do you want me to take a look at fixing the issue on tomorrow or Friday night, or are you already up to something?

Hi @alvinhochun, if you can take a look that would be great, I don't have a Windows test machine to reproduce the effect currently.

dkazakov requested changes to this revision.Oct 30 2017, 9:20 AM

Okay, I'll mark this patch as needs changes. The changes are expected to be done by @alvinhochun

This revision now requires changes to proceed.Oct 30 2017, 9:20 AM
alvinhochun accepted this revision as: alvinhochun.Nov 9 2017, 8:18 PM

Took me more time than I should've spent but it seems unfixable... It might be a bug in Qt with PressDelayHandler used by QFlickGestureRecognizer but I haven't traced much deeper than that. I tried several hacks but none works good enough to be acceptable :/

I'm considering filing a Qt bug report, or even just dig deeper and submit a code review. I guess we can deal with the bug later and just ship this patch as-is, it's not really blocking. (But please do fix the indentation.) At least it works sort of fine... But should it be enabled by default?

dkazakov accepted this revision.Nov 14 2017, 1:16 PM

Seems like the patch is ready to land now :)

This revision is now accepted and ready to land.Nov 14 2017, 1:16 PM
poke1024 added a comment.EditedNov 17 2017, 3:14 PM

@alvinhochun I assume you mean the wrong spacing in libs/ui/forms/wdggeneralsettings.ui? I'll fix that.

I'll push it as not enabled, we can always change it later on I guess.

EDIT Actually the spacing in wdggeneralsettings.ui seems ok. @alvinhochun, can you clarify which file?

I mean these (see inline comments) should use four spaces. (Sorry, they just trigger my OCD)

libs/ui/widgets/kis_preset_chooser.cpp
208

indentation

libs/widgets/KoResourceItemChooser.cpp
573

indentation

This revision was automatically updated to reflect the committed changes.
poke1024 marked 2 inline comments as done.