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

[OSX] NSScrollWheel event handling for 2D scrolling should use scrollingDeltaX/Y #6800

Closed
alexanderweiss opened this issue May 27, 2018 · 8 comments

Comments

@alexanderweiss
Copy link
Contributor

@alexanderweiss alexanderweiss commented May 27, 2018

Apple recommends using scrollingDeltaX/Y (https://developer.apple.com/documentation/appkit/nsevent/1524505-scrollingdeltax?language=objc#discussion) instead of deltaX/Y for scrolling events , which are currently being used:

/* Set the scroll count for scrollwheel scrolling */
_cursor.h_wheel -= (int)([ event deltaX ] * 5 * _settings_client.gui.scrollwheel_multiplier);
_cursor.v_wheel -= (int)([ event deltaY ] * 5 * _settings_client.gui.scrollwheel_multiplier);

I tried this out quickly and it makes scrolling the viewport using 2D scrolling significantly smoother: on par with dragging the viewport. I'd add it to #6793 or open a new pull request, but it's only supported since OS X 10.7 so it requires some conditional compilation and/or using selectors.

Using respondsToSelector and performSelector would be nicest, but performSelector doesn't work with primitives, so I've now combined it with conditional compilation, but it looks a little ugly. Using an invocation doesn't seem much better though.

(Note: I've changed the multiplier for scrollingDeltaX to keep the speed similar)

	/* Set the scroll count for scrollwheel scrolling */
#if (MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_X_VERSION_10_7)
	if ([event respondsToSelector:@selector(scrollingDeltaX)]) {
		_cursor.h_wheel -= (int)([ event scrollingDeltaX ] / 5 * _settings_client.gui.scrollwheel_multiplier);
		_cursor.v_wheel -= (int)([ event scrollingDeltaY ] / 5 * _settings_client.gui.scrollwheel_multiplier);
	} else {
#endif
		_cursor.h_wheel -= (int)([ event deltaX ] * 5 * _settings_client.gui.scrollwheel_multiplier);
		_cursor.v_wheel -= (int)([ event deltaY ] * 5 * _settings_client.gui.scrollwheel_multiplier);
#if (MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_X_VERSION_10_7)
	}
#endif

Any thoughts?

@alexanderweiss alexanderweiss changed the title NSScrollWheel event handling for 2D scrolling should use scrollingDeltaX/Y [OSX] NSScrollWheel event handling for 2D scrolling should use scrollingDeltaX/Y May 27, 2018
@michicc
Copy link
Member

@michicc michicc commented May 27, 2018

Untested (i.e. just written from memory) and not really pretty either, but an alternative would be:
CGFloat deltaX = ((CGFloat(*)(id, SEL))[ event methodForSelector:@selector(scrollingDeltaX) ])(event, @selector(scrollingDeltaX));
CGFloat deltaY = ((CGFloat(*)(id, SEL))[ event methodForSelector:@selector(scrollingDeltaY) ])(event, @selector(scrollingDeltaY));

But conditional compilation isn't that bad either. I'd recommend breaking coding style here though to loose the second #if. Look at other OS X code for how.

@joelekstrom
Copy link

@joelekstrom joelekstrom commented May 31, 2018

I don't quite understand why the respondsToSelector-check is needed since you already check system version - but here's another way to do it which avoids runtime checks and may or may not be nicer:

/* Set the scroll count for scrollwheel scrolling */
#if (MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_X_VERSION_10_7)
#define deltaSelector(AXIS) scrollingDelta##AXIS
#else
#define deltaSelector(AXIS) delta##AXIS
#endif
_cursor.h_wheel -= (int)([ event deltaSelector(X) ] / 5 * _settings_client.gui.scrollwheel_multiplier);
_cursor.v_wheel -= (int)([ event deltaSelector(Y) ] / 5 * _settings_client.gui.scrollwheel_multiplier);
#undef deltaSelector

@michicc
Copy link
Member

@michicc michicc commented May 31, 2018

respondsToSelector has three uses. First it allows the compiled app to run on any system version, second it avoids ugly #ifs in the code, and last the functionality of the compiled app doesn't depend on the SDK version it was build with.

@joelekstrom
Copy link

@joelekstrom joelekstrom commented May 31, 2018

Right - my question was regarding the original post. I wondered why it was needed when the compile time check is there anyway - my example omits that because if you have a compile time check there's no point in also doing a run-time check, unless I'm mistaken.

@michicc
Copy link
Member

@michicc michicc commented May 31, 2018

The compile time check/define itself doesn't prevent the app to run on an older system. Unless you stop that somewhere else, you still always need a runtime check or the user will get "unexplainable" errors.

@joelekstrom
Copy link

@joelekstrom joelekstrom commented May 31, 2018

Right you are - I foolishly assumed that everyone would compile on the computer they're playing on. 🤦‍♂️

Then another suggestion is to use @available instead, to skip #ifs and selector checks:

if (@available(macOS 10.7, *)) {
    _cursor.h_wheel -= (int)([ event scrollingDeltaX ] / 5 * _settings_client.gui.scrollwheel_multiplier);
    _cursor.v_wheel -= (int)([ event scrollingDeltaY ] / 5 * _settings_client.gui.scrollwheel_multiplier);
} else {
    _cursor.h_wheel -= (int)([ event deltaX ] * 5 * _settings_client.gui.scrollwheel_multiplier);
    _cursor.v_wheel -= (int)([ event deltaY ] * 5 * _settings_client.gui.scrollwheel_multiplier);
}

More info: https://clang.llvm.org/docs/LanguageExtensions.html#objective-c-available

@stale
Copy link

@stale stale bot commented Jan 24, 2019

This issue has been automatically marked as stale because it has not had any activity in the last two months.
If you believe the issue is still relevant, please test on the latest nightly and report back.
It will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

@stale stale bot added the stale label Jan 24, 2019
@alexanderweiss
Copy link
Contributor Author

@alexanderweiss alexanderweiss commented Jan 26, 2019

Completely forgot about this but thanks for the input! The function pointer suggestion works well, but with conditional compilation it's indeed not necessary (I got confused there).

@available looks nice! It was only introduced in 2017 though. That means that with @available any build on a macOS version below 10.12 would not be able to use the scrollingDeltaX/Y properties even though they were introduced in 10.7. There's probably not very many people doing that, but I'm not sure about the official stance on how well older platforms should be supported?

In any case, I'll get a PR in today using respondsToSelector.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants