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

Add detection of CTRL modifier to mouse events #126

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

Conversation

tombh
Copy link

@tombh tombh commented Apr 26, 2016

I'm new to the codebase, so not sure if this is the correct approach. Also, could be worth adding SHIFT and META to, but they're generally already intercepted by terminals.

@nsf
Copy link
Owner

nsf commented Apr 26, 2016

Will test it out this weekend. Also winapi version needs to be implemented.

@nsf
Copy link
Owner

nsf commented Apr 30, 2016

I'm really not sure about this. I mean I don't mind adding control modifier for mouse, but it can be confusing, because keyboard input doesn't use it, it has separate consts for ctrl keys. People may get confused due to that. So, I don't know. I'll think about that a bit more.

@tombh
Copy link
Author

tombh commented Apr 30, 2016

Fair point. My only suggestion would be to use a separate const {} block to define the mouse-specific constants ModMotion and ModCtrl just to add a measure of separation of concerns. The occurrence of ModCtrl in parse_mouse_event() is pretty unambiguous though wouldn't you say?

@nsf
Copy link
Owner

nsf commented Apr 30, 2016

Hm, or maybe I can just rename the modifier to something like ModMouseCtrl

@tombh
Copy link
Author

tombh commented May 2, 2016

Perfect!

@nsf
Copy link
Owner

nsf commented May 8, 2016

Sorry for not looking into it, I'm super busy and doing a pass on my open source projects issues requires a full day. I don't have one right now.

I will add mouse modifiers sooner or later for sure, so if you really need that feature, just keep it in your own fork for now.

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

Successfully merging this pull request may close these issues.

2 participants