monitor: refactor MonitorHandle to store dyn object
#3927
Conversation
kchibisov
force-pushed
the
kchibisov/dyn-monitor-handle
branch
from
42cbaa6 to
c6e43dc
Compare
kchibisov
requested review from
jackpot51,
notgull and
MarijnS95
as code owners
kchibisov
force-pushed
the
kchibisov/dyn-monitor-handle
branch
3 times, most recently
from
f2b84f4 to
84c9ae6
Compare
|
I've removed |
kchibisov
force-pushed
the
kchibisov/dyn-monitor-handle
branch
4 times, most recently
from
8565120 to
9486937
Compare
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.
kchibisov
force-pushed
the
kchibisov/dyn-monitor-handle
branch
from
9486937 to
4c94b55
Compare
There was a problem hiding this comment.
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->VideoModeMonitorHandle-> trait- Remove no-op monitor handles
| pub struct MonitorHandle { | ||
| pub(crate) inner: platform_impl::MonitorHandle, | ||
| #[derive(Debug, Clone)] | ||
| pub struct MonitorHandle(pub(crate) Arc<dyn MonitorHandleProvider>); |
There was a problem hiding this comment.
Hmm, what if we instead exposed Arc<dyn MonitorHandle> to the user, and had MonitorHandle: PartialEq + Eq + Hash?
There was a problem hiding this comment.
Ah, right, because PartialEq and Hash can't be dyn.
There was a problem hiding this comment.
Maybe there are ways around that?
There was a problem hiding this comment.
I'm not sure how it's useful, I decided to remove it for now. We can always bring those back.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
You can implement this Eq based on Id you store internally though? Like without a trait the way we're doing it for OwnedDisplayHandle.
There was a problem hiding this comment.
You can, though that only works if monitor equality is implementable on a u64, which I don't think it is on macOS?
There was a problem hiding this comment.
At the very least it needs to be u128 because it's a UUID (I think)
There was a problem hiding this comment.
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.
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. |
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 |
This also alters
VideoModeto be a regular object and not reference theMonitorHandle, since it's a static data.Given that
VideoModeset 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
MonitorIdwhich is more reliable in representing and split theMonitorHandleintoIdand a way to get the data for the monitor, but I think I'd rather leave it for the future.The
native_idthing 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() -> u64toMonitorId(u64).