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.

Sign up for GitHub

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

Jump to bottom

if natural-scroll is enabled, invert scroll inputs #113

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

if natural-scroll is enabled, invert scroll inputs #113

wants to merge 2 commits into from

Conversation

cg505
Copy link

@cg505 cg505 commented Nov 23, 2024

When natural scrolling setting is enabled, the scroll inputs are essentially the opposite of what's actually physically happening. This is fine for scrolling down a page, but feels very weird for things like volume control.

Invert natural scrolling inputs so scrolling up actually increases the volume, and vice versa.

Fixes #112.

@cg505
if natural-scroll is enabled, invert scroll inputs
2639075 
When natural scrolling setting is enabled, the scroll inputs are essentially the
opposite of what's actually physically happening. This is fine for scrolling
down a page, but feels very weird for things like volume control.

Invert natural scrolling inputs so scrolling up actually increases the volume,
and vice versa.

Fixes Moon-0xff#112.
cg505
cg505 commented Nov 23, 2024
View reviewed changes
extension.js Outdated
Comment on lines 174 to 176
if (event.is_pointer_emulated())
return Clutter.EVENT_PROPAGATE;

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure why this was initially added. It seems like my touchpad has a lot of scroll events that are considered emulated but otherwise look normal. This was causing problems for me because I have another extension with scroll event handling on the entire menu bar. A bunch of scroll events would hit this and propagate through, even though I just wanted to interact with the mpris label.
So that's why I removed it. Could also just make it return EVENT_STOP instead.

Copy link
Owner

Choose a reason for hiding this comment

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

I'm not sure why this was initially added.

I don't remember, though gnome-shell's code does the same.

This was causing problems for me because I have another extension with scroll event handling on the entire menu bar.

We found the same issue you are describing with Dash-to-panel (#47).

So that's why I removed it. Could also just make it return EVENT_STOP instead.

I'm not sure if we want "emulated scroll events" to trigger the scrolling action, this would probably change the volume increasing/decreasing speed in some hardware (can you test if this is the case in yours?). The idea is to follow gnome's volume widget on behaviour, though we are changing scroll direction here... speed, so I'm more inclined to change EVENT_PROPAGATE to EVENT_STOP.

Copy link
Author

Choose a reason for hiding this comment

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

I found this bugzilla thread: https://bugzilla.gnome.org/show_bug.cgi?id=687573, and these commits: https://gitlab.gnome.org/GNOME/gnome-shell/-/commit/7d4e14f38455977ffb103acd85239ed6b1a9d816, https://gitlab.gnome.org/GNOME/gnome-shell/-/commit/c9741ae3d5f661b2aee352c50ce21fab16694fbe (the last one is somewhat confusing to me since I definitely observe events with is_pointer_emulated() true).
It seems that some events are essentially duplicated, one emulated and one not emulated... I don't have the historical reasoning for why this is the case but my impression is that it has to do with retrofitting touchpad support into X11.

I suppose ideally other extensions should also be checking this, but in practice it doesn't make a lot of sense to propagate the events instead of just ignoring them completely.

Upshot: happy to use EVENT_STOP, makes sense.

@Moon-0xff
Copy link
Owner

While this might be out of scope, it makes sense that if we decide to fix the direction of the volume slider, we should do the same for the built-in global slider. This can indeed be done, though I'm not sure how.
I believe the volume widget is somewhere inside Main.panel and can be referenced (Main.panel...), if we can grab the volume slider object, then perhaps we can redefine the scroll function.

@cg505
Copy link
Author

cg505 commented Nov 24, 2024

While this might be out of scope

IMO, it is out of scope of the extension, but definitely this PR.

Options:

  1. Monkeypatch volume control (and maybe other sliders like brightness?) in the settings panel.
    • Note: I think if we're gonna do this, we need a solution that works for all the sliders, not just volume. Seems quite confusing if the volume slider works one way but the brightness slider works differently.
    • It's pretty unintuitive that installing this extension would modify the behavior of random UI elements that already exist in the shell, even if it sort of makes sense why we would want that.
  2. Try to get GNOME to fix this specifically for the panel sliders by using a similar technique to here. (Uninvert scroll events using awareness of natural scrolling setting value.)
  3. Get GTK/Clutter/whoever to fix this upstream so that we can somehow differentiate the physical scroll direction from the intended content movement direction. (Ideal, but complicated and requires much deeper changes.)
  4. Do nothing.

I'm inclined to do 4 because I'm lazy. Not really a fan of 1 for the reasons mentioned. 2 and 3 outside the scope of the project.

@Moon-0xff
Copy link
Owner

I do believe it would be of value to also flip the volume slider, and just the volume slider,
Though, I've tried before of modifying elements of GNOME this way, and while it's possible, it's not straightforward, and it sometimes breaks things in unexpected ways.

So option 4 it is!

@Moon-0xff
Copy link
Owner

Now, this change seems controversial, and also some people would prefer it the other way around as in (#89). So I think this work should be adapted to fit all kinds of bar.

I mean, being able to choose between:

  • GNOME's default scrolling direction
  • Reversed direction if "Natural scrolling" is enabled
  • Always reverse the scrolling direction

As reference, this commit, prefs.js and the schema file. and gjs guide.
You need to write the schema, compile it and add the function calls where you seem they fit.
I'll do this if you are done with the PR. Great work!

@cg505
Copy link
Author

cg505 commented Nov 25, 2024

Always reverse the scrolling direction

Although I see that some people would want the behavior to be consistent with the GNOME behavior, it doesn't seem likely that anyone would want this "always". That is, even if it is inconsistently with GNOME behavior.

I think it should be sufficient to just wrap the code I added in an if statement and add a setting called "respect natural scrolling" or something.

@Moon-0xff
Copy link
Owner

just ... add a setting called "respect natural scrolling"

Thing is, I also felt the direction reversed when testing it for the first time using the "Traditional" setting, so making this change accessible to "Traditional" users makes sense to me.
On a UI/UX point of view though, a single boolean setting for this is way better.

Also, sorry for not having a follow-up on this yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Moon-0xff Moon-0xff Moon-0xff left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Invert scroll direction if "natural scolling" is used
2 participants
@cg505 @Moon-0xff