monitor: refactor `MonitorHandle` to store dyn object #3927

kchibisov · 2024-09-21T17:35:39Z

This also alters VideoMode to be a regular object and not reference the MonitorHandle, since it's a static data.

Given that VideoMode set may change during runtime keeping the reference as a some sort of validity may not be idea and propagating errors when changing video mode could be more reliable.

--

I'm not sure that I like how the monitor stuff looks, but I don't have a better idea. Ideally we want to have some sort of MonitorId which is more reliable in representing and split the MonitorHandle into Id and a way to get the data for the monitor, but I think I'd rather leave it for the future.

The native_id thing I've moved is already present on all the platforms, so I'd really see why not and usually platforms have some way to address the monitors and backends may build such addressing themselves.

It could make sense to change native_id() -> u64 to MonitorId(u64).

kchibisov · 2024-11-14T17:09:39Z

I've removed docsrs build for platform for now, since types are simply not present in all backends, so build fails), and platform module will be soon gone anyway.

madsmtm

Procedurally, I would prefer if we did the "refactor VideoModeHandle to VideoMode with plain data" in a separate PR first, that would make this PR easier to thoroughly review - I just cannot deal with 49 changed files at once, sorry ;). (For my own part I've been dreading reviewing this for that reason, I suspect the other maintainers feel the same).

Also, I feel it's undesirable to remove the no-op monitors, at least at this point / in this PR? I think that would allow you to keep the docsrs stuff in platform?

If you're burned out on it, I can try to do it in a few days? I.e. splitting this PR into effectively three parts:

VideoModeHandle -> VideoMode
MonitorHandle -> trait
Remove no-op monitor handles

madsmtm · 2024-12-03T16:56:26Z

src/monitor.rs

-pub struct MonitorHandle {
-    pub(crate) inner: platform_impl::MonitorHandle,
+#[derive(Debug, Clone)]
+pub struct MonitorHandle(pub(crate) Arc<dyn MonitorHandleProvider>);


Hmm, what if we instead exposed Arc<dyn MonitorHandle> to the user, and had MonitorHandle: PartialEq + Eq + Hash?

Ah, right, because PartialEq and Hash can't be dyn.

Maybe there are ways around that?

I'm not sure how it's useful, I decided to remove it for now. We can always bring those back.

So I found a way to make it possible, see this playground link.

In terms of usefulness, I think it's nicer to have the ability to just compare monitor handles vs. having to compare ids (this goes for other handles too IMO).

You can implement this Eq based on Id you store internally though? Like without a trait the way we're doing it for OwnedDisplayHandle.

You can, though that only works if monitor equality is implementable on a u64, which I don't think it is on macOS?

At the very least it needs to be u128 because it's a UUID (I think)

You can, though that only works if monitor equality is implementable on a u64, which I don't think it is on macOS?

You tell me :- ). In general, an ID is something assigned to you by the display server for that exact reason to compare them. You could have a more verbose monitor IDs.

In general, Though, I don't mind PartialEq that way given that you say that it works. Though, can send it separately, since you'll be touching other stuff along the lines.

kchibisov · 2024-12-03T17:02:20Z

I just cannot deal with 49 changed files at once, sorry ;

You need to look only into the top-level and the ones for your backend, and given that both are in the same file, you'll have 2 PRs with the 49 files changed, just twice or more.

If you feel like splitting then go ahead, I won't block on that, it's just way more work and by the time you've done it you'll review the PR. Probably not that bad in this context.

kchibisov · 2024-12-03T17:03:24Z

Also, I feel it's undesirable to remove the no-op monitors, at least at this point / in this PR? I think that would allow you to keep the docsrs stuff in platform?

That's true, I guess, the issue is that certain types don't really exist sometimes and we still try to build, so I've removed since we generally want to remove platform, so point in keeping it doesn't bring any benefit in my opinion and just creates more work, which is not great. Like once the split is done, you won't have cfg at all.

kchibisov · 2024-12-25T05:00:11Z

I've extracted video mode from it here #4060

kchibisov · 2025-02-08T11:33:56Z

Separated docsrs and monitor handle parts.

madsmtm

I've opened #4138 to ensure that we don't forget to do something about the docs.rs situation before v0.31.

Thanks again for doing this, and sorry for the long delay in reviewing it. Have approved, the only real concern here is the u64 (native) vs. u128 (unique) ID comparisons, but that can also be fixed in a follow-up.

madsmtm · 2025-02-24T01:35:52Z

src/platform_impl/orbital/event_loop.rs

    }

    fn system_theme(&self) -> Option<Theme> {
        None
    }

    fn primary_monitor(&self) -> Option<crate::monitor::MonitorHandle> {
-        Some(crate::monitor::MonitorHandle { inner: MonitorHandle })
+        None
    }


Nit: I'd prefer to keep functional changes like these out of refactoring PRs in the future, but it's fine for now.

Though maybe add a changelog entry for it?

It's not really functional iirc, since monitors are not supported.

madsmtm · 2025-02-24T01:39:53Z

src/platform/macos.rs

@@ -499,22 +500,16 @@ impl EventLoopBuilderExtMacOS for EventLoopBuilder {

 /// Additional methods on [`MonitorHandle`] that are specific to MacOS.
 pub trait MonitorHandleExtMacOS {
-    /// Returns the identifier of the monitor for Cocoa.
-    fn native_id(&self) -> u32;


Hmm, I believe there is value in keeping this (and hmonitor on Windows too).

The reason is that the native_id that is returned here is directly usable in FFI calls, since and will be two different values on macOS, UUID for comparisons and CGDirectDisplayID as the native ID.

Maybe rename MonitorHandleProvider::native_id to MonitorHandleProvider::unique_id? Or maybe just MonitorHandleProvider::id?

The thing is that we use it in ffi of some sort right now as well. Not sure how to go about it though.

I think you should keep all the native_id/hmonitor/..., and provide a new MonitorHandleProvider::id, which is a u128 whose representation isn't guaranteed.

madsmtm · 2025-02-24T01:46:47Z

src/monitor.rs

+/// Provider of the [`MonitorHandle`].
+pub trait MonitorHandleProvider: AsAny + fmt::Debug + Send + Sync {
+    /// Native platform identifier of this monitor.
+    fn native_id(&self) -> u64;


Researched a bit more, monitors are equatable on macOS, but the ID is a UUID (i.e. 128 bits), and comparing the naive ID (CGDirectDisplayID) is wrong (see

winit/src/platform_impl/apple/appkit/monitor.rs

Lines 116 to 118 in 3a39a6d

// `CGDirectDisplayID` changes on video mode change, so we cannot rely on that

// for comparisons, but we can use `CGDisplayCreateUUIDFromDisplayID` to get an

// unique identifier that persists even across system reboots

).

Suggested change

fn native_id(&self) -> u64;

fn native_id(&self) -> u128;

src/monitor.rs

src/platform_impl/apple/uikit/monitor.rs

This also alters `VideoMode` to be a regular object and not reference the `MonitorHandle`, since it's a static data. Given that `VideoMode` set may change during runtime keeping the reference as a some sort of validity may not be idea and propagating errors when changing video mode could be more reliable.

Due to casts and use of platform specific crates in those modules it's not really feasible to build docs for them. After separating crates, thus should become way easier to navigate, since backends information would be publicly available.

kchibisov requested review from daxpedda and madsmtm September 21, 2024 17:35

daxpedda mentioned this pull request Oct 11, 2024

Split Winit up into multiple crates #3433

Open

5 tasks

kchibisov force-pushed the kchibisov/dyn-monitor-handle branch from 42cbaa6 to c6e43dc Compare November 14, 2024 16:14

kchibisov marked this pull request as ready for review November 14, 2024 16:15

kchibisov requested review from jackpot51, notgull and MarijnS95 as code owners November 14, 2024 16:15

kchibisov requested review from notgull and removed request for notgull November 14, 2024 16:15

kchibisov force-pushed the kchibisov/dyn-monitor-handle branch 3 times, most recently from f2b84f4 to 84c9ae6 Compare November 14, 2024 17:08

kchibisov force-pushed the kchibisov/dyn-monitor-handle branch 5 times, most recently from 9486937 to 4c94b55 Compare November 14, 2024 17:48

madsmtm added the C - nominated Nominated for discussion in the next meeting label Dec 2, 2024

madsmtm requested changes Dec 3, 2024

View reviewed changes

kchibisov mentioned this pull request Dec 25, 2024

api: make VideoModeHandle into VideoMode #4060

Merged

kchibisov force-pushed the kchibisov/dyn-monitor-handle branch 2 times, most recently from 1436e3e to 2644af5 Compare February 8, 2025 11:31

kchibisov requested a review from madsmtm February 8, 2025 11:33

madsmtm mentioned this pull request Feb 24, 2025

Ensure platform-documentation is available on docs.rs #4138

Open

madsmtm approved these changes Feb 24, 2025

View reviewed changes

kchibisov force-pushed the kchibisov/dyn-monitor-handle branch 2 times, most recently from 99e13e8 to c8f517d Compare February 26, 2025 08:19

kchibisov added 3 commits February 26, 2025 11:30

v2

87b732a

docs: don't build platform docs for docsrs

551f496

Due to casts and use of platform specific crates in those modules it's not really feasible to build docs for them. After separating crates, thus should become way easier to navigate, since backends information would be publicly available.

kchibisov force-pushed the kchibisov/dyn-monitor-handle branch from c8f517d to 551f496 Compare February 26, 2025 08:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

monitor: refactor `MonitorHandle` to store dyn object #3927

monitor: refactor `MonitorHandle` to store dyn object #3927

kchibisov commented Sep 21, 2024

kchibisov commented Nov 14, 2024 •

edited

Loading

madsmtm left a comment

madsmtm Dec 3, 2024

madsmtm Dec 3, 2024

madsmtm Dec 3, 2024

kchibisov Dec 3, 2024

madsmtm Dec 3, 2024 •

edited

Loading

kchibisov Dec 3, 2024

madsmtm Dec 3, 2024

madsmtm Dec 3, 2024

kchibisov Dec 3, 2024

kchibisov commented Dec 3, 2024

kchibisov commented Dec 3, 2024

kchibisov commented Dec 25, 2024

kchibisov commented Feb 8, 2025

madsmtm left a comment

madsmtm Feb 24, 2025

kchibisov Feb 26, 2025

madsmtm Feb 24, 2025 •

edited

Loading

kchibisov Feb 26, 2025

madsmtm Feb 26, 2025

madsmtm Feb 24, 2025

	// `CGDirectDisplayID` changes on video mode change, so we cannot rely on that
	// for comparisons, but we can use `CGDisplayCreateUUIDFromDisplayID` to get an
	// unique identifier that persists even across system reboots

monitor: refactor MonitorHandle to store dyn object #3927

Are you sure you want to change the base?

monitor: refactor MonitorHandle to store dyn object #3927

Conversation

kchibisov commented Sep 21, 2024

kchibisov commented Nov 14, 2024 • edited Loading

madsmtm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madsmtm Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kchibisov commented Dec 3, 2024

kchibisov commented Dec 3, 2024

kchibisov commented Dec 25, 2024

kchibisov commented Feb 8, 2025

madsmtm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madsmtm Feb 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monitor: refactor `MonitorHandle` to store dyn object #3927

monitor: refactor `MonitorHandle` to store dyn object #3927

kchibisov commented Nov 14, 2024 •

edited

Loading

madsmtm Dec 3, 2024 •

edited

Loading

madsmtm Feb 24, 2025 •

edited

Loading