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

fix smooth scrolling on macOS #7904

Merged
merged 2 commits into from Jan 31, 2021
Merged

fix smooth scrolling on macOS #7904

merged 2 commits into from Jan 31, 2021

Conversation

lhietal
Copy link
Contributor

@lhietal lhietal commented Jan 17, 2021

Fixing issue on macOS with smooth scrolling events. Fixes #6068 scrolling issue by scaling the input from event->delta_x and event->delta_y.

The scaling amount feels ok on my system. It could be changed to get faster scrolling. Besides lighttable this also affects some modules. I tested module resize, retouch, histogram, zone system and bauhaus comboboxes. They seem to work correctly.

There might be better ways to handle trackpads. #6068 (comment)

@parafin
Copy link
Member

parafin commented Jan 17, 2021

Can you tell where the formula comes from or how it was created?

@johnny-bit johnny-bit added scope: OS support making darktable work on particular operating systems scope: UI user interface and interactions labels Jan 17, 2021
@lhietal
Copy link
Contributor Author

lhietal commented Jan 17, 2021

I was just trying to fit input to the expectation that scrolling happens when 1 is returned. I have no spesific information on what values from event->delta_x represent.

It is a general mapping between ranges. https://stackoverflow.com/questions/5731863/mapping-a-numeric-range-onto-another

@parafin
Copy link
Member

parafin commented Jan 17, 2021

I'm asking, because Beep6581/RawTherapee#5599 issue you linked in #6068 got resolved differently: Beep6581/RawTherapee#5607
They are basically ignoring the delta_x value altogether and just use 1 instead as value. As the source they have GTK own code for scrollbars. So I'm wondering which approach is better. Have you tried both options?

@lhietal
Copy link
Contributor Author

lhietal commented Jan 17, 2021

I didn't underestand the RawTherapee solution completely but I think they use event->delta_x somehow differently.

The way I underestand dt_gui_get_scroll_unit_deltas() is designed to work is that it sets *delta_x to 1 or -1 when multiple smaller values from event->delta_x accumulate to 1 or -1. Thumbtable is then moved by one row. On macOS event->delta_x is a lot larger than 1. So scrolling is too fast.
Setting acc_x = 1 still causes tumbtable to move every time the function is called. Which is several times per second.

@MStraeten
Copy link
Collaborator

this fix doesn't improve the scroll behaviour in lighttable filemanager on a magic mouse or trackpad. It's very hard to scroll just one row - one little movement and you've jumped over 20 rows ...

@MStraeten
Copy link
Collaborator

after adding some debug messages in the code indicates the get_scroll functions are never used for the scrollbars of the filemanager/thumbtable in lighttable

@lhietal
Copy link
Contributor Author

lhietal commented Jan 19, 2021

dt_gui_get_scroll_unit_delta() is called in src/dtgtk/thumbtable.c on line 893. Am I underestanding the functionality of _event_scroll() incorrectly?

@MStraeten
Copy link
Collaborator

gtk.c line 695:

gboolean dt_gui_get_scroll_unit_delta(const GdkEventScroll *event, int *delta)
{
  int delta_x, delta_y;
   fprintf(stderr, "dt_gui_get_scroll_unit_delta\n");
  if(dt_gui_get_scroll_unit_deltas(event, &delta_x, &delta_y))
  {
    *delta = delta_x + delta_y;
    return TRUE;
  }
  return FALSE;
}

The debug message isn't printed when having scrollbars for central view in lighttable and darkroom activated in preferences and scrolling while having them in focus.
If scrolling is done inside the central view it's called (but thats doesn't affect the scrollbars) - and in this case the pr works fine.

src/gui/gtk.c Outdated
@@ -654,8 +654,13 @@ gboolean dt_gui_get_scroll_unit_deltas(const GdkEventScroll *event, int *delta_x
// accumulate trackpad/touch scrolls until they make a unit
// scroll, and only then tell caller that there is a scroll to
// handle
#ifdef GDK_WINDOWING_QUARTZ // on macOS deltas need to be scaled
acc_x += (2.0 / 300.0) * (event->delta_x + 150) - 1;
Copy link
Member

Choose a reason for hiding this comment

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

Is there a drawback to simplifying this to
acc_x += event->delta_x / 150.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. The range should be symmetrical around 0 so simplifying works.

Copy link
Collaborator

Choose a reason for hiding this comment

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

after using this a couple of days if find the divisor 150 makes the ui very laggy - i found a value of 50 being more responsive but not too fast (at least for my old magic mouse)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. It is better to use the smaller value. It is still possible to scroll by one row.

@lhietal
Copy link
Contributor Author

lhietal commented Jan 23, 2021

The debug message isn't printed when having scrollbars for central view in lighttable and darkroom activated in preferences and scrolling while having them in focus.

In this case it seems that scroll distance comes directly from GtkScrollbar widget in scrollbar_changed() (gtk.c line 871). Scroll events are not handled directly so same fix is not possible.

@MStraeten
Copy link
Collaborator

to get this stuff done:
@lhieta please change the code as suggested, so @parafin can check and merge.
tweaking the behaviour of the scrollbar widget is a different story ...

@parafin
Copy link
Member

parafin commented Jan 30, 2021

I would still like to know how well rawtherapee solution (copied from GTK) works. Which is it seems just replacing event->delta_x / 50 with 1 or some other constant.
But if current version of PR is confirmed to make things better I'm fine with merging it as is.

@jonastr
Copy link
Contributor

jonastr commented Jan 30, 2021

@parafin I'm not sure I understand your question, so maybe my answer is not what you're looking for: There are no scrolling issues in RawTherapee on MacOS.
Or is your question rather to try and copy the same bits to see if it works with Darktable as well?

@MStraeten
Copy link
Collaborator

It quite easy to see the effect when scrolling the thumbtable in lighttable mode with magic mouse or trackpad since the slider widgets aren’t affected.
using the slider a little swipe scrolls over 20 lines. Just changing the angle the finger is touching the mouse can cause scrolling over a couple of rows.
If the mousepointer is on the thumbtable it’s now possible to scroll by row as it is done when using a real mousewheel.
So even it doesn‘t affect the scrollbarwidget its a real improvement.

@parafin
Copy link
Member

parafin commented Jan 31, 2021

@parafin I'm not sure I understand your question, so maybe my answer is not what you're looking for: There are no scrolling issues in RawTherapee on MacOS.
Or is your question rather to try and copy the same bits to see if it works with Darktable as well?

Yes, there are no issues, because they fixed them, see my previous comment: #7904 (comment)

So my questions is why not use an already tested solution?

@lhietal
Copy link
Contributor Author

lhietal commented Jan 31, 2021

I haven't spend a lot of time trying to underestend the RawTherapee solution. Simply changing event->delta_x / 50 to 1 produces similar behavior than before the fix. Changing it to 0.1 makes scrolling slower but doesn't allow scrolling speed to change. So that scrolling is always slow.

@parafin
Copy link
Member

parafin commented Jan 31, 2021

OK, so let's re-iterate - current PR improves the situation by fixing some scrolling behaviour in darktable, but not all of them (specifically lighttable scrollbar)?

@MStraeten
Copy link
Collaborator

yes, because the handlers for scroll events in gtk.c aren't triggered if the thumbtable scrollbar widget is used.

@parafin parafin merged commit c672080 into darktable-org:master Jan 31, 2021
parafin pushed a commit that referenced this pull request Jan 31, 2021
@parafin
Copy link
Member

parafin commented Jan 31, 2021

Merged to master and 3.4 branch

@lhietal lhietal deleted the lhietal-patch-1 branch January 31, 2021 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: OS support making darktable work on particular operating systems scope: UI user interface and interactions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

macOS: lighttable scrolling nearly unusable and preview very slow since 3.2.1
6 participants