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

niri msg pick-window #1031

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

niri msg pick-window #1031

wants to merge 3 commits into from

Conversation

bbb651
Copy link
Contributor

@bbb651 bbb651 commented Jan 21, 2025

recording.mp4

Originally I tried to indicate picking was occurring by turning the cursor into a crosshair, in the same way as the screenshot UI in the seat handler but that means we're forgetting what the original cursor was, I think doing it here will work but still feels a bit hacky, so I removed it for now.

Fixes #589.

@bbb651 bbb651 force-pushed the niri-msg-pick-window branch 2 times, most recently from 2a25a6d to 368d47d Compare January 22, 2025 14:36
@bbb651 bbb651 marked this pull request as draft January 22, 2025 14:37
@bbb651
Copy link
Contributor Author

bbb651 commented Jan 22, 2025

See commit message:

WIP: Currently the ipc server gets stuck waiting for niri to return the
picked window, and if the ipc client closes it doesn't realize that until
niri returns the picked window leading to "ghost" window picking.

In general having self.niri.pick_window.retain(|tx| !tx.is_closed());
to cleanup closed clients in input events feels like the wrong place,
but should work since having stale senders doesn't effect anything else.

Copy link
Owner

@YaLTeR YaLTeR left a comment

Choose a reason for hiding this comment

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

Originally I tried to indicate picking was occurring by turning the cursor into a crosshair, in the same way as the screenshot UI in the seat handler but that means we're forgetting what the original cursor was, I think doing it here will work but still feels a bit hacky, so I removed it for now.

I would say that having a crosshair cursor is more important than remembering the original cursor. So if there's no easy way to restore the original cursor, just make it crosshair with a FIXME comment.

I think some visual indication of what window you're about to pick would also be good. But I guess that can be done separately.

WIP: Currently the ipc server gets stuck waiting for niri to return the
picked window, and if the ipc client closes it doesn't realize that until
niri returns the picked window leading to "ghost" window picking.

Hmm, yeah, maybe some kind of future::select() can be used to also check for the client disconnecting?

@bbb651
Copy link
Contributor Author

bbb651 commented Jan 26, 2025

Hmm, yeah, maybe some kind of future::select() can be used to also check for the client disconnecting?

futures_util has one, the problem is that I don't know if it's possible to get a future of when the socket disconnects. I think constantly doing is_close/reads/peeks in a futures_util::poll_fn might work, perhaps with some delay.

@YaLTeR
Copy link
Owner

YaLTeR commented Jan 26, 2025

On the other hand, maybe it's not that big a deal? Picking is a quick operation either way

@bbb651 bbb651 force-pushed the niri-msg-pick-window branch 3 times, most recently from a130828 to c0ae10f Compare February 16, 2025 20:09
@bbb651 bbb651 marked this pull request as ready for review February 16, 2025 20:10
@YaLTeR YaLTeR force-pushed the niri-msg-pick-window branch from 808571b to 15bec65 Compare February 17, 2025 14:56
@YaLTeR
Copy link
Owner

YaLTeR commented Feb 17, 2025

Added some minor fixes.

However, I'm now realizing that this should be a pointer grab. Because you don't want the mouse events to go to windows while you're picking. See how spatial movement grab works for example. It also shows how to change and restore the cursor without the condition in the cursor rendering code.

@bbb651
Copy link
Contributor Author

bbb651 commented Feb 19, 2025

What should happen when there's already a grab when a request to pick a window is invoked? Should it instantly return None, or should it queue to up until the current grab is done?
I'm also not sure what to do about start data, it seems to not be optional and there's no sensible value it could be because it wasn't started by a pointer event..

@YaLTeR
Copy link
Owner

YaLTeR commented Feb 19, 2025

I think it should cancel the ongoing grab. This is stuff like popup grabs, or ongoing DnD.

I'm also not sure what to do about start data, it seems to not be optional and there's no sensible value it could be because it wasn't started by a pointer event..

Get the current location I guess.

@bbb651 bbb651 force-pushed the niri-msg-pick-window branch from 8e9b56b to 9c31798 Compare February 19, 2025 19:29
@bbb651
Copy link
Contributor Author

bbb651 commented Feb 19, 2025

I don't like that the cancellation logic is now very hidden in the ungrab handler and the code makes the assumption that self.pick_window is some iff there's an active pick window grab without checking it or encoding it in the type system..
Ideally self.pick_window would be something like Option<(Sender<...>, PickWindowGrab)> but smithay needs ownership of it, would wrapping it in a Mutex<RefCell<_>> and impl PointerGrab<State> for Mutex<RefCell<PickWindowGrab>> make senes?

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.

niri msg pick-window
2 participants