monitor: refactor MonitorHandle to store dyn object#3927
Conversation
42cbaa6 to
c6e43dc
Compare
f2b84f4 to
84c9ae6
Compare
|
I've removed |
9486937 to
4c94b55
Compare
madsmtm
left a comment
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
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 |
|
I've extracted video mode from it here #4060 |
1436e3e to
2644af5
Compare
|
Separated docsrs and monitor handle parts. |
madsmtm
left a comment
There was a problem hiding this comment.
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.
551f496 to
d55cc22
Compare
|
@madsmtm please ensure the |
d55cc22 to
8217568
Compare
madsmtm
left a comment
There was a problem hiding this comment.
macOS rebase looks good (with comment fixed).
8217568 to
644999a
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.
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.
644999a to
8750c88
Compare
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).Part of #3433.