Fixes #4754: report other mouse buttons #7773
Conversation
alacritty/src/input/mod.rs
Outdated
| @@ -946,7 +956,7 @@ impl<T: EventListener, A: ActionContext<T>> Processor<T, A> { | |||
| MouseButton::Left => self.ctx.mouse_mut().left_button_state = state, | |||
| MouseButton::Middle => self.ctx.mouse_mut().middle_button_state = state, | |||
| MouseButton::Right => self.ctx.mouse_mut().right_button_state = state, | |||
| _ => (), | |||
| _ => self.ctx.mouse_mut().other_button_state = state, | |||
There was a problem hiding this comment.
How would this ever work? You're conflating all other buttons into a single state that can't possibly be accurate.
There was a problem hiding this comment.
The issue is still exactly the same?
There was a problem hiding this comment.
hi, it was specified here to only map Back and Forward to 8 and 9 & to report other buttons according to xterm.
this was why only MouseButton::Back & MouseButton::Forward was handled in the match statement. and the default case was to handle the other buttons as is.
hence, we check for the buttons pressed on #on_mouse_release, add the respective values & report them as is.
please correct me if I am missing something here, thanks! :)
There was a problem hiding this comment.
and the default case was to handle the other buttons as is.
That is not what your code does.
alacritty/src/input/mod.rs
Outdated
| @@ -946,7 +956,7 @@ impl<T: EventListener, A: ActionContext<T>> Processor<T, A> { | |||
| MouseButton::Left => self.ctx.mouse_mut().left_button_state = state, | |||
| MouseButton::Middle => self.ctx.mouse_mut().middle_button_state = state, | |||
| MouseButton::Right => self.ctx.mouse_mut().right_button_state = state, | |||
| _ => (), | |||
| _ => self.ctx.mouse_mut().other_button_state = state, | |||
There was a problem hiding this comment.
The issue is still exactly the same?
| MouseButton::Other(x) => match x { | ||
| 6 | 7 => (x + 64) as u8, | ||
| 10 | 11 => (x + 128) as u8, |
There was a problem hiding this comment.
Why did you add this? What is it based on? X11 button IDs? How about cross-platform compatibility?
There was a problem hiding this comment.
hi
Why did you add this? What is it based on? X11 button IDs?
it is based on XTerm control sequences (as mentioned here: #6957 (comment)).
How about cross-platform compatibility?
I think cross-platform compatibility is already being handled by winit (ref: rust-windowing/winit#2770)
There was a problem hiding this comment.
I think cross-platform compatibility is already being handled by winit (ref: rust-windowing/winit#2770)
This is irrelevant here, because winit guarantee only for Named values, but you're mapping Other and Other is undefined. And that's what was the question about here, why you translate 6 | 7 to x + 64 where on wayland or macOS the buttons are just completely different, thus you kind of implies X11 because xterm is X11.
There was a problem hiding this comment.
I think cross-platform compatibility is already being handled by winit (ref: rust-windowing/winit#2770)
This is irrelevant here, because winit guarantee only for
Namedvalues, but you're mappingOtherandOtheris undefined. And that's what was the question about here, why you translate6 | 7tox + 64where on wayland or macOS the buttons are just completely different, thus you kind of implies X11 because xterm is X11.
alright, I will look into this & update the PR soon.
Supersedes #6957