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

mac: Setting to explicitly choose pan or zoom for mouse wheel (#1339) #1344

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nanoant
Copy link
Contributor

@nanoant nanoant commented Feb 20, 2023

While auto remains the default, we now support explicit choice of doing pan or zoom when scrolling with mouse wheel or touch pad.

This can be also adapted for other (non-macOS) platforms.

…pace#1339)

While auto remains the default, we now support explicit choice of doing
pan or zoom when scrolling with mouse wheel or touch pad.

This can be also adapted for other (non-macOS) platforms.
@vespakoen
Copy link
Contributor

Wouldn't it make more sense to use something like:

        AUTO,
        MOUSE,
        TRACKPAD,

?

Or maybe I am mistaken and this is not related to #1339 but an extra feature?

@phkahler
Copy link
Member

I'm with @vespakoen on this. It also should not be platform specific. And pan with scroll wheel doesn't make sense because it only works vertically. Scroll means zoom.

@nanoant
Copy link
Contributor Author

nanoant commented Feb 22, 2023

@phkahler @vespakoen I can reiterate on this, what should be then the correct name for the setting?
Pointing device (*) Auto-detect ( ) Mouse ( ) Trackpad ?

@phkahler
Copy link
Member

I should have been clearer, but sometimes I don't want to upset contributors. I don't want scroll wheel to pan even as an option. Won't merge it.

@nanoant
Copy link
Contributor Author

nanoant commented Feb 22, 2023

@phkahler No problem for me. And yes this is a follow up for #1339 and #1093 e1b0784 which started all of this by introducing panning behavior on trackpads implemented inside of - (void)scrollWheel:(NSEvent *)nsEvent.

Please note that the event is called "scroll wheel", not "touch" or "track pad". So its purpose is to handle scrolling not anything else. This implementation broke my Apple Magic Mouse workflow that suddenly started panning instead of zooming. I tried to un-break it by extending too weak nsEvent.subtype == NSEventSubtypeTabletPoint && kind == Platform::Window::Kind::TOPLEVEL check, adding activeTrackpadTouches > 0. But apparently it stills trips on some weird Logitech mice setups. I tested #1274 cf4defc with my two Logitech mice and some 2 Chinese unbranded ones, and they all worked fine. But I don't use any Logitech customization software and I can only suspect such a software may make USB HID look like touch pad to guimac.m code for whatever reason.

So long story short, I am trying to make this software work for all macOS users, but I am not responsible for having #1339 guys complaining that wheel is panning instead of moving, because I did not contribute that panning implementation in the first place. (Un?)fortunately I was called up with my GitHub id, because I was last person trying to fix the flawed #1093 implementation.

If #1093 was implemented using gestures maybe we had no such a problem, but the original author decided to do it within scrollWheel and now we have 2 camps of touch pad users wanting to pan and mouse users wanting to zoom, both via single scrollWheel implementation.

So I hope this is clear. And I am not upset (contributor), but I want to make sure you get the whole picture that I am just an ex-macOS developer who's now doing something completely else for living and doing 3D printing for fun trying to help 🥲

@phkahler
Copy link
Member

IMHO after trying a couple versions of solvespace on my old MBA, a setting isn't appropriate. We need to use modifier keys to differentiate the mouse button behaviors when using the trackpad. Shift, Control, Super, are all there. I'd even like if one of them were used so click-drag isn't needed since it's wierd on a trackpad. But this is just one opinion ATM.

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

Successfully merging this pull request may close these issues.

None yet

4 participants