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

Remove non-platform-consistent mouse events from X11 #3874

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

Conversation

daxpedda
Copy link
Member

@daxpedda daxpedda commented Aug 16, 2024

Currently X11 will emit a WindowEvent::CursorMoved when the window comes into focus, even if e.g. the cursor isn't inside the window.
This is inconsistent with other platforms and can also be a bit misleading to users.
I believe in the future, we should use CursorEntered/CursorLeft on focus and unfocus on all backends to make this idea work properly. Emitting the cursor position in these events will be handled by #3833.

The other case, is X11 emitting a WindowEvent::CursorMoved on the first touch point made in a touch interaction. I believe the idea behind this is to signify the user that the cursor has jumped to this location. However, this can again be misinterpreted by users and is not consistent between platforms.
We are also unable to express this sort of pointer vs touch vs cursor with the current API, which would need its own separate discussion and motivation first.

I've encountered this while working on #3810 and #3833.

Comment on lines 1319 to 1331
// Mouse cursor position changes when touch events are received.
// Only the first concurrently active touch ID moves the mouse cursor.
if is_first_touch(&mut self.first_touch, &mut self.num_touch, id, phase) {
let event = Event::WindowEvent {
window_id,
event: WindowEvent::CursorMoved {
device_id: mkdid(util::VIRTUAL_CORE_POINTER),
position: location.cast(),
},
};
callback(&self.target, event);
}

Copy link
Member

Choose a reason for hiding this comment

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

I think the point was that the touch moves the mouse pointer? In general, I'd hold it a bit until pointer overhual since it won't break things due to things being generic.

Right now I'm not sure, so I'd suggest to research why this code is here in the first place, since it's clearly fixing some not working thing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Its this #1261.
Which has a good use-case as well.

Will remove the removal of this part.

@daxpedda daxpedda force-pushed the x11-focus-mouse-move branch from 3504422 to 1cadba0 Compare August 23, 2024 13:11
@daxpedda daxpedda added the C - nominated Nominated for discussion in the next meeting label Aug 23, 2024
Comment on lines -1248 to -1251
let event = Event::WindowEvent {
window_id,
event: WindowEvent::CursorMoved { device_id: mkdid(pointer_id as _), position },
};
Copy link
Member Author

Choose a reason for hiding this comment

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

I tracked this down to #348.

While I do think this is a valid use-case, the current implementation is incorrect, because its also emits an event if the cursor is not inside the window.

For this to be solved we need to do it across all platforms and do it correctly.
My proposition would be to emit a CursorEntered when focus is gained if the cursor is inside the window.

On the other hand this might also be addressed by #2648.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C - nominated Nominated for discussion in the next meeting DS - x11 S - platform parity Unintended platform differences
Development

Successfully merging this pull request may close these issues.

2 participants