Add `cursor_position` getter API #2648

amrbashir · 2023-01-24T16:59:03Z

Tested on all platforms changed
- Windows
- macOS
- X11
Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
Created or updated an example program if it would help users understand this functionality
Updated feature matrix, if new features were added or implemented

filnet · 2023-01-24T17:22:59Z

What's the motivation for getting the cursor position with the new function over getting it via an event ?

amrbashir · 2023-01-24T17:29:54Z

This is useful for positioning the window on a particular monitor when creating the window, for example, when making a spotlight-like app, where you want to show the window on the monitor the has the cursor, then I would need to:

get the cursor location
get the monitor based on the cursor location
center the app on the monitor

This PR is for 1 and I will be opening another PR for 2, later today or tomorrow.

kchibisov · 2023-01-24T19:34:45Z

get the cursor location
get the monitor based on the cursor location

This won't work on macOS/Wayland for sure?

amrbashir · 2023-01-24T19:43:01Z

Wayland yes, macOS should be possible, no?

amrbashir · 2023-01-24T19:45:17Z

for macOS there is no direct API to get a monitor based on a point but we can iterate over available monitors and check if the point is contained within one of them

kchibisov · 2023-01-24T19:51:08Z

Wayland yes, macOS should be possible, no?

On Wayland certainly not. Since you know only about the cursor over your window. You don't even have control were to open a window, so use-case you're describing is very specific and can be achieved on X11 and maybe Windows.

kchibisov · 2023-01-24T19:52:05Z

And in my experience normal window managers will open a new window where your cursor is if you don't explicitly specify the output you want(like on Wayland it's done by majority).

amrbashir · 2023-01-24T19:55:04Z

yeah mostly you don't need to manually position your window but there is use-cases where you want to control which monitor to spawn the window on, for example: spotlight-like apps, widgets, overlays...etc

kchibisov · 2023-01-24T20:06:46Z

But you already have all in-place for that? You could just report current_monitor on the event_loop or such?

ids1024 · 2023-01-24T20:26:35Z

On Wayland certainly not. Since you know only about the cursor over your window. You don't even have control were to open a window, so use-case you're describing is very specific and can be achieved on X11 and maybe Windows.

You should be able to implement a centered "spotlight-like" app like this on Wayland with layer-shell (in compositors that support it), but yeah, you couldn't implement it this way.

madsmtm

macOS impl approved, unsure about desired API, I'll leave that up for you to decide on.

src/event_loop.rs

src/window.rs

src/platform_impl/macos/util/mod.rs

filnet · 2023-01-30T16:44:24Z

This is useful for positioning the window on a particular monitor when creating the window, for example, when making a spotlight-like app, where you want to show the window on the monitor the has the cursor, then I would need to:
1. get the cursor location

2. get the monitor based on the cursor location

3. center the app on the monitor
This PR is for 1 and I will be opening another PR for 2, later today or tomorrow.

May be you can keep 1 private for 2 ?

amrbashir · 2023-01-30T16:59:21Z

May be you can keep 1 private for 2 ?

2 has been rejected in #2649 so this API is no longer connected to 2 directly

ids1024 · 2023-01-30T17:22:09Z

As I mentioned earlier, the "spotlight-like app" use case on Wayland would be served by layer shell (#2582). I think what would be ideal for that is if either winit or an extension crate could provide an API that can use layer shell, or position things more manually on X/win32/etc. to provide as portable a solution as possible for this particular use case.

I would generally agree with the authors of the base Wayland protocol that applications placing windows at specific spots on screen is generally a bad practice outside of specialized things like this (though Winit already has set_outer_position).

#2160 is relevant, wrt including things in winit vs a separate crate.

…init into feat/cursor-position

daxpedda

We just decided in the meeting that we are happy to move forward with this.
The coordinate system used here should be the same as for all other APIs covering the cursor position.

There was also a concern raised about multiple pointers, which is something that we decided we will handle separately but will most likely include adding a device ID parameter.

daxpedda

LGTM.
Can't review the relevant backends though and the CI needs to pass.

src/changelog/unreleased.md

Co-authored-by: daxpedda <daxpedda@gmail.com>

amrbashir · 2024-03-26T14:38:12Z

Seems like a couple of tests are failing on nightly but should be unrelated to this PR

daxpedda

Thanks!

CI should be fixed after #3604.

madsmtm · 2024-03-28T17:41:00Z

src/event_loop.rs

@@ -490,6 +491,16 @@ impl ActiveEventLoop {
            platform: self.p.owned_display_handle(),
        }
    }
+
+    /// Returns the current cursor position in screen coordinates.


Referencing #2648 (review).

I think we thought this was on Window, and hence expected things to be in window coordinates, just like the coordinates for set_cursor_position (such that window.set_cursor_position(window.cursor_position()?)? is (roughly) a no-op). But we decided against Window::cursor_position in https://github.com/rust-windowing/winit/pull/2648/files#r1090072945, this API is ActiveEventLoop::cursor_position.

In that case, I'd expect the coordinates to be the same as those from Window::outer_position, is that correct? If so, could you refer to that for discussion about the coordinate space, just like Window::set_outer_position does?

(If you think that this interpretation is correct, I'll test the macOS implementation myself to ensure that it matches Window::outer_position).

In that case, I'd expect the coordinates to be the same as those from Window::outer_position, is that correct?

Yeah

If so, could you refer to that for discussion about the coordinate space, just like Window::set_outer_position does?

like this amrbashir@1f9a3df ?

Yup.

And I've tested it now, and pushed a fix to return positions with the correct scaling and such (it's still a little broken, but that's a more general issue, see #3615).

src/event_loop.rs

madsmtm · 2024-03-29T16:24:43Z

src/event_loop.rs

+    ///
+    /// - **iOS / Android / Wayland / Orbital / Web**: Unsupported.
+    #[inline]
+    pub fn cursor_position(&self) -> Result<PhysicalPosition<f64>, ExternalError> {


It confused me that the return type of this is not PhysicalPosition<i32> like the return type of Window::outer_position, but I guess it matches the CursorMoved event - apparently cursors can be positioned on a sub-pixel scale, didn't know that!

We needed to flip and scale the coordinates, as is usual for macOS' screen coordinates.

Add cursor_position getter API

7f55366

amrbashir requested review from jackpot51, msiglreith, madsmtm and kchibisov as code owners January 24, 2023 16:59

amrbashir mentioned this pull request Jan 24, 2023

Tracking issue for different implementation between winit tauri-apps/tao#470

Open

22 tasks

amrbashir added 3 commits January 24, 2023 19:00

Restore examples/window.rs

04aec3c

update docs

7495b2f

fix build

d2bd7af

madsmtm requested changes Jan 29, 2023

View reviewed changes

src/event_loop.rs Show resolved Hide resolved

src/window.rs Outdated Show resolved Hide resolved

src/window.rs Outdated Show resolved Hide resolved

src/platform_impl/macos/util/mod.rs Outdated Show resolved Hide resolved

Update window.rs

5b34692

amrbashir added 3 commits February 7, 2023 20:47

remove Window::cursor_position

5555119

Merge branch 'feat/cursor-position' of https://github.com/amrbashir/w…

71985a8

…init into feat/cursor-position

Merge branch 'master' into feat/cursor-position

e08d28d

daxpedda mentioned this pull request Sep 23, 2023

Added the show system menu method to window #3109

Merged

5 tasks

amrbashir requested review from daxpedda and notgull as code owners February 27, 2024 19:28

daxpedda added DS - macos DS - windows DS - x11 labels Feb 27, 2024

daxpedda reviewed Mar 15, 2024

View reviewed changes

daxpedda removed the C - nominated Nominated for discussion in the next meeting label Mar 15, 2024

amrbashir added 6 commits March 26, 2024 05:22

Merge branch 'master' into feat/cursor-position

64f7e38

fix a few missed imports after merge

4d6d78d

Discard changes to dpi/LICENSE

86506db

Discard changes to dpi/LICENSE

a6a7636

revert license file change

f976f2f

Update unreleased.md

5f811f8

amrbashir requested a review from daxpedda March 26, 2024 03:33

daxpedda reviewed Mar 26, 2024

View reviewed changes

src/changelog/unreleased.md Outdated Show resolved Hide resolved

amrbashir and others added 6 commits March 26, 2024 15:40

try fix build

e48ec28

Update src/changelog/unreleased.md

37dbcc7

Co-authored-by: daxpedda <daxpedda@gmail.com>

duplicate import

9ee1fd6

fix x11 impl

261a709

unsafe

76ba0b6

Merge branch 'master' into feat/cursor-position

61db908

daxpedda approved these changes Mar 27, 2024

View reviewed changes

madsmtm reviewed Mar 28, 2024

View reviewed changes

reference Window::outer_position

1f9a3df

kchibisov requested changes Mar 29, 2024

View reviewed changes

src/event_loop.rs Show resolved Hide resolved

kchibisov added the C - nominated Nominated for discussion in the next meeting label Mar 29, 2024

kchibisov self-requested a review March 29, 2024 15:42

madsmtm reviewed Mar 29, 2024

View reviewed changes

macOS: Fix cursor position calculation

cd6b857

We needed to flip and scale the coordinates, as is usual for macOS' screen coordinates.

madsmtm approved these changes Mar 29, 2024

View reviewed changes

daxpedda removed the C - nominated Nominated for discussion in the next meeting label Apr 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `cursor_position` getter API #2648

Add `cursor_position` getter API #2648

amrbashir commented Jan 24, 2023 •

edited by madsmtm

filnet commented Jan 24, 2023

amrbashir commented Jan 24, 2023 •

edited

kchibisov commented Jan 24, 2023

amrbashir commented Jan 24, 2023

amrbashir commented Jan 24, 2023

kchibisov commented Jan 24, 2023

kchibisov commented Jan 24, 2023

amrbashir commented Jan 24, 2023

kchibisov commented Jan 24, 2023

ids1024 commented Jan 24, 2023

madsmtm left a comment

filnet commented Jan 30, 2023 •

edited

amrbashir commented Jan 30, 2023 •

edited

ids1024 commented Jan 30, 2023

daxpedda left a comment

daxpedda left a comment

amrbashir commented Mar 26, 2024

daxpedda left a comment

madsmtm Mar 28, 2024

madsmtm Mar 28, 2024

amrbashir Mar 29, 2024

madsmtm Mar 29, 2024

madsmtm Mar 29, 2024

Add cursor_position getter API #2648

Are you sure you want to change the base?

Add cursor_position getter API #2648

Conversation

amrbashir commented Jan 24, 2023 • edited by madsmtm

filnet commented Jan 24, 2023

amrbashir commented Jan 24, 2023 • edited

kchibisov commented Jan 24, 2023

amrbashir commented Jan 24, 2023

amrbashir commented Jan 24, 2023

kchibisov commented Jan 24, 2023

kchibisov commented Jan 24, 2023

amrbashir commented Jan 24, 2023

kchibisov commented Jan 24, 2023

ids1024 commented Jan 24, 2023

madsmtm left a comment

Choose a reason for hiding this comment

filnet commented Jan 30, 2023 • edited

amrbashir commented Jan 30, 2023 • edited

ids1024 commented Jan 30, 2023

daxpedda left a comment

Choose a reason for hiding this comment

daxpedda left a comment

Choose a reason for hiding this comment

amrbashir commented Mar 26, 2024

daxpedda left a comment

Choose a reason for hiding this comment

madsmtm Mar 28, 2024

Choose a reason for hiding this comment

madsmtm Mar 28, 2024

Choose a reason for hiding this comment

amrbashir Mar 29, 2024

Choose a reason for hiding this comment

madsmtm Mar 29, 2024

Choose a reason for hiding this comment

madsmtm Mar 29, 2024

Choose a reason for hiding this comment

Add `cursor_position` getter API #2648

Add `cursor_position` getter API #2648

amrbashir commented Jan 24, 2023 •

edited by madsmtm

amrbashir commented Jan 24, 2023 •

edited

filnet commented Jan 30, 2023 •

edited

amrbashir commented Jan 30, 2023 •

edited