Scale down MacOS scrolling from event controllers#20710
Open
dtorop wants to merge 3 commits intodarktable-org:masterfrom
Open
Scale down MacOS scrolling from event controllers#20710dtorop wants to merge 3 commits intodarktable-org:masterfrom
dtorop wants to merge 3 commits intodarktable-org:masterfrom
Conversation
MacOS trackpad scroll events appear to be of much greater magnitude than those on other systems. To make them usable, we need to scale down the scroll deltas. For traditional GTK 3 code, dt_gui_get_scroll_unit_deltas() took care of this. Unfortunately, this code depends on the "scroll-event" signal. To make the code GTK 4 ready, we need to handle the "scroll" signal generated by a GtkEventControllerScroll. For discrete scrolls, we can handle this transparently via the dt_gui_connect_scroll_discrete() setup function. This new function is similar to dt_gui_connect_scroll(). There is no need to call it with the GTK_EVENT_CONTROLLER_SCROLL_DISCRETE flag, as it adds this automatically. If darktable is compiled for MacOS, a proxy function will scale down the scroll events. Note that this commit doesn't do anything to help scale down scroll events from a non-discrete GtkEventControllerScroll. Also: In bauhaus _widget_scroll, actually warn if called an a non-scroll event or no event rather than having a useless NOP conditional. Don't free the event in unlikely case we can't retrieve it. Also: Update/remove a couple comments. In GTK4 it seems preferable to target events directly to widgets (as gtk_propagate_event and gtk_event_controller_handle_event are gone) so don't apologize in comments for using gtk_widget_event(). Fixes: darktable-org#20698
Collaborator
|
no decrease of sensitivity on sliders and also scope (color harmony rotation) with this fix |
Contributor
Author
|
@MStraeten: Drat! So it doesn't fix anything? Would you have time to add a bit of diagnostic code in and send the output when scrolling a slider? Something like: modified src/gui/gtk.c
@@ -4699,6 +4699,7 @@ static void _scroll_discrete_proxy(GtkEventControllerScroll* self,
gdouble dy,
_scroll_handler_proxy_t *h)
{
+ dt_print_ext("proxy dx %f dy %f cur_dx %f cur_dy %f", dx, dy, h->cur_dx, h->cur_dy);
// Even though the event controller has already accumulated smooth
// scroll events to discrete dx/dy, MacOS smooth scrolling events
// are too fast so scale them down via another accumulating stage.
@@ -4722,6 +4723,7 @@ static void _scroll_discrete_proxy(GtkEventControllerScroll* self,
h->cur_dy -= steps;
dy = steps;
}
+ dt_print_ext("smooth dx %f dy %f cur_dx %f cur_dy %f", dx, dy, h->cur_dx, h->cur_dy);
}
if(dx != 0.0 || dy != 0.0)
h->scroll_callback_real(self, dx, dy, h->scroll_data_real);I also realize there might be a slightly nicer way to implement this (only accumulate scrolling at the darktable level, not at the GTK level), but not material unless this actually works. |
Collaborator
|
7*.* --> rotating color harmonies in scope |
Contributor
Author
|
@MStraeten: Thank you for debug output. It looks like the If there is no difference or otherwise trouble with this code, I'd be interested to see diagnostics via this diff: modified src/gui/gtk.c
@@ -4678,8 +4678,10 @@ static void _scroll_discrete_proxy(GtkEventControllerScroll* controller,
gdouble dy,
gpointer user_data)
{
+ dt_print_ext("proxy dx %f dy %f cur_dx %f cur_dy %f", dx, dy, _scroll_discrete_dx, _scroll_discrete_dy);
GdkEvent *event = gtk_get_current_event();
if(!event) return;
+ dt_print_ext("event type %d dir %d emulated %d", gdk_event_get_event_type(event), event->scroll.direction, gdk_event_get_pointer_emulated(event));
if(gdk_event_get_event_type(event) == GDK_SCROLL
&& event->scroll.direction == GDK_SCROLL_SMOOTH
// avoid double counting real and emulated events when receiving smooth scrolls
@@ -4699,6 +4701,7 @@ static void _scroll_discrete_proxy(GtkEventControllerScroll* controller,
#endif
_scroll_discrete_dx += copysign(pow(fabs(dx), compression), dx * scale);
_scroll_discrete_dy += copysign(pow(fabs(dy), compression), dy * scale);
+ dt_print_ext("scaled dx %f dy %f", copysign(pow(fabs(dx), compression), dx * scale), copysign(pow(fabs(dy), compression), dy * scale));
dx = dy = 0.0;
if(fabs(_scroll_discrete_dx) >= 1.0)
{
@@ -4712,6 +4715,7 @@ static void _scroll_discrete_proxy(GtkEventControllerScroll* controller,
_scroll_discrete_dy -= steps;
dy = steps;
}
+ dt_print_ext("smooth stop %d dx %f dy %f cur_dx %f cur_dy %f", event->scroll.is_stop, dx, dy, _scroll_discrete_dx, _scroll_discrete_dy);
}
if(dx != 0.0 || dy != 0.0)
{ |
Attenuate smooth scroll delta by calculating delta ** compression, which will reduce large deltas (apparently found in momentum-based scrolling on MacOS). On MacOS, instead of scaling down smooth scroll by DT_UI_SCROLL_SMOOTH_DELTA_SCALE, scale down by 0.25. It appears that for MacOS, we do not receive "scroll-begin" and "scroll-end" events for trackpad events, so instead exam the GdkEvent, which brings this code closer to extant working dt_gui_get_scroll_unit_deltas(). Don't use GTK_EVENT_CONTROLLER_SCROLL_DISCRETE flag, regardless of OS, as _scroll_discrete_proxy() will accumulate smooth scroll events. Don't bother to handle scroll stop events as we won't receive them from GTK and it is OK to not clear accumulated scroll on stop. Simplify storage for proxy discrete scroll handler. Don't use an allocated struct. We can keep dx/dy globally, as we do not expect multiple simultaneous scrolls of multiple widgets. Store real scroll handler as GObject data. Note that the exponential attenuation math is on recommendation from ChatGPT, based on very little actual data from MacOS.
dtorop
force-pushed
the
scroll-controller-fixes-macos
branch
from
f45d2be to
e8af2e6
Compare
Collaborator
|
diagnostic log 20710.txt - need to play around with both parameters - starting point isn't significant better btw: the scope is just sensitive to up/down movements, the slider for up/down and left/right movements. |
Fix error in formula which didn't actually multiply by scale. Reduce the scale for MacOS. Rationale: From a sample of 431 MacOS smooth scroll events, median delta is 1. Breakdown of deltas by probability: 1: 45% 2: 27% 3: 12% 4: 5% 5: 6% 6: 2% 7: 1% 8: 1% 9: 0.2% 10: 0.2% Previously we used a scale of 0.02 and a compression of 1. Make scale/compression which give equivalent attenuated results for an average sample of 1.5. Also: - Handle horizontal scrolling for histogram - Update bauhaus copyright date
Contributor
Author
|
I had the math wrong and was dropping the scale parameter. Should be fixed in the latest commit. The whole idea of using pow() to attenuate scale may not be worth it. The alternative would be to set scale to 0.02 and compression to 1 for MacOS, which should match the GTK3 behavior. And thank you for pointing out about horizontal scroll on scope. Had been on todo, should work now. Maybe this version works better? If not, debugging code would be this: modified src/gui/gtk.c
@@ -4678,8 +4678,10 @@ static void _scroll_discrete_proxy(GtkEventControllerScroll* controller,
gdouble dy,
gpointer user_data)
{
+ dt_print_ext("proxy dx %f dy %f cur_dx %f cur_dy %f", dx, dy, _scroll_discrete_dx, _scroll_discrete_dy);
GdkEvent *event = gtk_get_current_event();
if(!event) return;
+ dt_print_ext("event type %d dir %d emulated %d", gdk_event_get_event_type(event), event->scroll.direction, gdk_event_get_pointer_emulated(event));
if(gdk_event_get_event_type(event) == GDK_SCROLL
&& event->scroll.direction == GDK_SCROLL_SMOOTH
// avoid double counting real and emulated events when receiving smooth scrolls
@@ -4701,6 +4703,7 @@ static void _scroll_discrete_proxy(GtkEventControllerScroll* controller,
dy = scale * copysign(pow(fabs(dy), compression), dy);
_scroll_discrete_dx += dx;
_scroll_discrete_dy += dy;
+ dt_print_ext("scaled dx %f dy %f cur_dx %f cur_dy %f", dx, dy, _scroll_discrete_dx, _scroll_discrete_dy);
dx = dy = 0.0;
if(fabs(_scroll_discrete_dx) >= 1.0)
{
@@ -4714,6 +4717,7 @@ static void _scroll_discrete_proxy(GtkEventControllerScroll* controller,
_scroll_discrete_dy -= steps;
dy = steps;
}
+ dt_print_ext("smooth stop %d dx %f dy %f cur_dx %f cur_dy %f", event->scroll.is_stop, dx, dy, _scroll_discrete_dx, _scroll_discrete_dy);
}
if(dx != 0.0 || dy != 0.0)
{ |
Collaborator
|
that fix was successful. thanks for the effort |
Contributor
Author
|
@MStraeten: Great! Thank you for so many rounds of tests. It's helpful to see the MacOS scroll delta #'s, to understand a bit more why it requires special case code. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Attenuate discrete scrolling deltas via a proxy handler set up in
dt_gui_connect_scroll_discrete().Usage:
The
handler_funcwill receive discrete (integral) scroll deltas. The proxy will transparently accumulate smooth scroll (trackpad/touchpad) fractional scrolling as needed to provide integral steps. It will attenuate smooth scroll events, with slight attenuation on non-MacOS systems and substantial attenuation when compiled for MacOS..To make a cross-plaform smooth scroll handler, we need to scale down the MacOS scroll deltas. For traditional GTK 3 code,
dt_gui_get_scroll_unit_deltas()took care of this. This PR allows for using the GTK4-readyscrollsignal generated by aGtkEventControllerScroll.Notes:
dt_gui_connect_scroll_discrete()should eventually replace all uses ofdt_gui_get_scroll_unit_deltas()anddt_gui_get_scroll_unit_delta(). This will be follow-up work to make GTK4-ready code. There are about 18 of places to change this. Until this work is done, the feel of trackpad scrolling will be slightly different depending on whether it is handled by the old or new code.dt_gui_connect_scroll_discrete(), the flags do not need to includeGTK_EVENT_CONTROLLER_SCROLL_DISCRETEbut should include one ofGTK_EVENT_CONTROLLER_SCROLL_VERTICAL,GTK_EVENT_CONTROLLER_SCROLL_HORIZONTALorGTK_EVENT_CONTROLLER_SCROLL_DISCRETE.Also: In bauhaus
_widget_scroll(), warn in the unlikely case called with a non-scroll or null event, rather than ignoring this via a NOP conditional. Don't free a null event.Also: Update/remove a couple comments. In GTK4 it seems preferable to target events directly to widgets (as
gtk_propagate_eventandgtk_event_controller_handle_eventare gone) so don't apologize in comments for usinggtk_widget_event().Fixes: #20698
See #20701 for some other discussion.