Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sidepanel continuous scrolling for macOS #8616

Merged
merged 18 commits into from
Apr 14, 2021

Conversation

lhietal
Copy link
Contributor

@lhietal lhietal commented Apr 5, 2021

Allow sidepanels to receive smooth scroll events on macOS. Scrolling is now continuous. Continues #8449.

I changed some iops to use dt_gui_get_scroll_unit_deltas() which also affects other OSs. This produces nicer behaviour that better maches current master. Normal scrolling should not be affected but touchpad experience might be slightly different.

Implements similar functionality as #2575.

Effectively reverses #1659. It seems that issues reported are related to issue fixed in #7904.

Testing this PR

Use both mouse scrollwheel and touchpad for testing if possible and go through functionality in the table.

Modules Expected behaviour
retouch (scales) Current scale can be set by scrolling
gradient slider (parametric mask) In and out points work with modifier keys
base curve, tone curve, rgb curve, color zones Point on the curve can be moved up and down
colorcorrection Scrolling moves slider below the widget
denoise profiled (wavelets mode), lowlight vision, monochrome, raw denoise, contrast equaliser Radius of the cursor changes smoothly
levels, rgb levels Scrolling handles
zone system Number of zones can be changed by scrolling
histogram Scrolling changes exposure, can be slow
geotagging Possible to change numbers one at a time
Other functions
side panels (lighttable and darkroom) Scroll without stepping
bauhaus slider Scrolling works with modifier keys
bauhaus combobox Possible to change values one at a time
timeline (moving and zooming) #8669 fixes zooming
darkroom second window Zooming works same as main window
filemanager and filmstrip Scroll one image at a time (there is PR #8449 to change this)

@dterrahe
Copy link
Member

dterrahe commented Apr 5, 2021

I just happened to be looking at something related today in dt_gui_get_scroll_unit_deltas. It turns out my touchpad generates both smooth scrolling events and emulated discrete events, leading to "double counting".
Adding the following, based on digging in the GDK docs,

  if(gdk_event_get_pointer_emulated((GdkEvent*)event))
    return FALSE; // avoid double counting real and emulated events when receiving smooth scrolls

seems to resolve my issue and on my system I haven't noticed averse effects, but as we know that doesn't guarantee it won't cause problem with other systems or configurations.

If it doesn't cause regressions, presumably it should be added to dt_gui_get_scroll_deltas as well.

@lhietal
Copy link
Contributor Author

lhietal commented Apr 6, 2021

I added the check for emulated events. It doesn't change anything on my system.

@dterrahe
Copy link
Member

dterrahe commented Apr 6, 2021

I added the check for emulated events. It doesn't change anything on my system.

We should try to get some serious testing on your PR (after it gets merged; from experience it is hard to get testing in the PR phase) by people on all kinds of different machines. Maybe you could add a short list of test steps so all the different cases are covered; scrolling on a widget or side panel might behave differently from ctrl-scrolling on the central view to change the mask opacity, for example. And the list should probably say what the expected / wanted behavior is, because if it has been broken on a particular system forever, the user there might not even know what to look for and report back that all is fine/no worse than before, when it actually is still broken.

Then when it lands in master we can ask in different places (irc/mailing list/pixls) to run the test script and report back here with 👍 or comments.

Otherwise we could get the usual situation after release of 3.6 where we have to run around and try to fix things for very specific configurations and that always gets messy. There might even be people that prefer the current behavior in some respect that you consider broken; better to have those discussions now.

@lhietal
Copy link
Contributor Author

lhietal commented Apr 6, 2021

If we are going to do extensive testing would it be a good idea to consider enabling GDK_SMOOTH_SCROLL_MASK on other systems as well. From #1659 it seems that macOS was the only one having issues. I have only really used darktable on macOS though. Is there any advantage to having smooth scrolling on other OSs?

@dterrahe
Copy link
Member

dterrahe commented Apr 6, 2021

Is there any advantage to having smooth scrolling on other OSs?

Shouldn't it behave similar to MacOS (i.e. less jumpy scrolling, i.e. more readable)? A quick test on my system doesn't show a difference, but it may be that this needs to be switched on system/gtk-wide as well (and I'm not using gnome).

One advantage of using a consistent setting across all platforms is that more people could notice breakage early that currently only affects MacOS.

@lhietal
Copy link
Contributor Author

lhietal commented Apr 6, 2021

I tried this PR on my "Ubuntu USB stick" (X11). With GDK_SMOOTH_SCROLL_MASK added to gui->scroll_mask in gtk.c line 1311. This produced similar touchpad behaviour to macOS. I think this could be preferrable but I don't the same intuition for scrolling in Ubuntu that I have for macOS.

@MStraeten
Copy link
Collaborator

i build it on osx and did some tests with my magic mouse - but i can't feel any difference to the behaviour of current master. What exactly should be the difference and how to test this?
the scroll behavior for the main area (scrolling using the scrollbars - enabled via preferences) that wasn't fixed with #7904 isn't affected at all.

@lhietal
Copy link
Contributor Author

lhietal commented Apr 7, 2021

Sidepanels should scroll without steps in darkroom and lighttable. Previously it was dependant on what was under the cursor. (E.g. scrolling over module headers was intermittent) Works for me with macOS 10.14.6 and macbook touchpad. I think magic mouse should work in a similar way. Traditional style scrollwheel isn't changed.

Problem with scrollbars hasn't changed for me either.

@lhietal
Copy link
Contributor Author

lhietal commented Apr 11, 2021

It doesn't look very promising that I am the only one able to see the change of this PR. I'm not really sure what the problem might be on macOS. For X11 this PR currently doesn't enable the functionality and requires the addition from previous comment.

With GDK_SMOOTH_SCROLL_MASK added to gui->scroll_mask in gtk.c line 1311.

I might add a commit for that if it makes testing easier.

@dterrahe
Copy link
Member

With GDK_SMOOTH_SCROLL_MASK added to gui->scroll_mask in gtk.c line 1311.

I might add a commit for that if it makes testing easier.

Makes sense to me; unless there is a known problem with gtk smooth scrolling on linux, we should follow gtk behavior; leave it up to the user to disable it in gtk settings if they want to.

@TurboGit as we know, this will not get serious wider testing until it gets merged. If @lhietal adds a more specific description in the OP of this PR of what the intended changes (the ones they see on MacOS; maybe using a screencast video if it is visible) are, then it can always be (partially) reverted if it does cause problems somewhere. But to me this all seems logical cleanup/consistency changes in some cases removing possibly unnecessary workarounds (that were put in "just to be safe").

@TurboGit
Copy link
Member

Ok, given @dterrahe comment, please @lhietal can you please give some more descriptions so we can merge soon. TIA.

@TurboGit TurboGit added this to the 3.6 milestone Apr 11, 2021
@TurboGit TurboGit added bugfix pull request fixing a bug priority: high core features are broken and not usable at all, software crashes labels Apr 11, 2021
Copy link
Member

@TurboGit TurboGit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm merging that. All my testing have been ok.

I have found one issue but verified that it was already in master. The touchpad on my notebook cannot be used reliably to scroll on the scrollbars in lighttable & darkroom. Any idea about what could be wrong ?

Anyway, not a regression so merging. Thanks.

@TurboGit TurboGit merged commit e7ab836 into darktable-org:master Apr 14, 2021
@lhietal
Copy link
Contributor Author

lhietal commented Apr 15, 2021

The touchpad on my notebook cannot be used reliably to scroll on the scrollbars in lighttable & darkroom. Any idea about what could be wrong ?

What is the exact behaviour that you are getting? This might be something related to #7904 (comment) where scrolling scrollbars is too fast on macOS. I could look into that issue again but there doesn't seem to be any easy solution.

@TurboGit
Copy link
Member

@lhietal : the behavior is erratic, the scrollbars do not move then there are moving fast in loop up/down (just a bit) making the display shake/flicker... Really strange! I don't have this issue with mouse scroll of course.

@lhietal
Copy link
Contributor Author

lhietal commented Apr 15, 2021

Seems that it is not related to the issue I mentioned. I haven't seen that kind of behaviour in any of my tests.

@dtorop
Copy link
Contributor

dtorop commented Apr 25, 2021

The concept of darktable.gui->scroll_mask was to allow per-system control of whether smooth scrolling was enabled. As this PR is able to revert #1659, if it works in further testing, a next step could be to eliminate scroll_mask. Essentially:

s/darktable.gui->scroll_mask/GDK_SCROLL_MASK | GDK_SMOOTH_SCROLL_MASK/

@lhietal
Copy link
Contributor Author

lhietal commented Apr 25, 2021

That could be a reasonable next step. Could there be a need for using other masks with different systems in the future? E.g. GDK_TOUCH_MASK or GDK_TOUCHPAD_GESTURE_MASK. I don't have anything planned with those.

@dtorop
Copy link
Contributor

dtorop commented Apr 25, 2021

Could there be a need for using other masks with different systems in the future? E.g. GDK_TOUCH_MASK or GDK_TOUCHPAD_GESTURE_MASK.

I don't know. It'd be worth checking/following #8078, as that's where intense development is going on.

The scroll_mask showed up as I wanted to make smooth scrolling work on Wayland (where scrolling was broken w/out GDK_SMOOTH_SCROLL_MASK), but wasn't able to test on MacOS (where scrolling via GDK_SMOOTH_SCROLL_MASK was broken until this PR) and hence it was a quick hack rather than a plan for future extension.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix pull request fixing a bug priority: high core features are broken and not usable at all, software crashes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants