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

Web: Implement MonitorHandle #3801

Merged
merged 2 commits into from
Jul 23, 2024
Merged

Conversation

daxpedda
Copy link
Member

@daxpedda daxpedda commented Jul 19, 2024

This is based on and requires #3805.

This implements MonitorHandle on Web by using the Window Management API, which is currently only implemented on Chrome.

This gets a bit complicated, because to access the API the user has to explicitly be asked to give permission, on top of most calls being asynchronous. So to solve this I implemented a couple of things:

  • When the EventLoop is created, the browser is queried, in the background if the user has previously given permission to use the Window Management API.
  • When the event loop is started, EventLoop::run(), it is delayed until this asynchronous query has finished. Which should really only be one tick.
  • If the user has previously given permission to use the Window Management API, we can use it.
  • If not, we fallback to Window.screen, which is implemented on all browsers. The "downside" is that it contains much less information about the current monitor and no information about primary or other monitors.

Cool things:

  • At none of these steps the user is actually asked to give permission. To explicitly ask the user for permission, ActiveEventLoop::request_detailed_monitor_permission() has to be used.
  • None of the Futures have to polled to completion, dropping them is fine. They are only there to receive errors or allow the user to "know when something has finished doing its thing".
  • MonitorHandles created with the Window Management API can be targeted with Window::set_fullscreen().
  • If the user somehow grants the site permission to use the Window Management API externally without going through Winit, we detect this and automatically upgrade the backend.
  • Everything that can be cached is cached to minimize any JS FFI calls. E.g. multiple calls to ActiveEventLoop::request_detailed_monitor_permission() do nothing, instead they share the result to minimize any JS overhead.

Not cool things:

  • There is no way to "upgrade" MonitorHandles to use the new API if they were created using the fallback.
  • VideoModeHandle is basically useless, its only there to expose bit_depth().
  • Using MonitorHandle with Window::set_fullscreen() only makes sense if it was created using the Window Management API, otherwise it doesn't work.
  • Currently the only way for users to receive errors is to pull the returned Futures to completion.
  • The whole screen orientation API has no cross-platform implementation, so its all implemented via extension traits.

Follow-up:

  • Add stuff to Browser support tracking #2875.
  • Return Option from MonitorHandle::scale_factor().
  • Return Option from MonitorHandle::position().
  • Rename Window::set_fullscreen() to Window::request_fullscreen() and maybe return an error. If not introduce an Web-only async version of it to allow users to actually handle failure.
  • Add current bit_depth() to MonitorHandle and remove VideoModeHandle from Web again. (Add MonitorHandle::current_video_mode() #3802)
  • Open tracking issue for cross-platform monitor events.
  • Open tracking issue for cross-platform screen orientation getters, locking and events.

@daxpedda daxpedda force-pushed the web-monitor branch 4 times, most recently from bd041e8 to d0b5a30 Compare July 19, 2024 12:55
@daxpedda daxpedda marked this pull request as ready for review July 19, 2024 12:55
@daxpedda daxpedda force-pushed the web-monitor branch 4 times, most recently from b7d9479 to 2c0c405 Compare July 20, 2024 15:57
@daxpedda daxpedda marked this pull request as draft July 20, 2024 16:53
@daxpedda
Copy link
Member Author

I extracted some unrelated changes to #3805 and based this PR on it.

daxpedda added a commit that referenced this pull request Jul 23, 2024
- Internal: Fix dropping `Notifier` without sending a result causing `Future`s to never complete. This should never happen anyway, but now we get a panic instead of nothing if we hit a bug.
- Internal: Remove a bunch of `unwrap()`s that aren't required when correctly using `MainThreadMarker`.
- `Window::canvas()` is now able to return a reference instead of an owned value.

Extracted from #3801.
@daxpedda daxpedda marked this pull request as ready for review July 23, 2024 14:49
@daxpedda
Copy link
Member Author

No changes so far, only rebasing on all the churn.

examples/window.rs Outdated Show resolved Hide resolved
examples/window.rs Outdated Show resolved Hide resolved
examples/window.rs Outdated Show resolved Hide resolved
examples/window.rs Outdated Show resolved Hide resolved
examples/window.rs Outdated Show resolved Hide resolved
@@ -81,3 +83,29 @@ impl<V, S: Clone + Send, E> Clone for Wrapper<V, S, E> {
}
}
}

impl<V, S: Clone + Send, E> Eq for Wrapper<V, S, E> {}
Copy link
Member

Choose a reason for hiding this comment

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

Generally Eq/Ord go after their Partial impls.

Copy link
Member Author

Choose a reason for hiding this comment

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

I generally do this alphabetically in the Web backend.
So I would like to stick to that.

I can make a follow-up PR where I align all derives and implementations in the whole crate to do PartialEq/PartialOrd before Eq/Ord and generally have the same order throughout the crate if you like.

Copy link
Member

Choose a reason for hiding this comment

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

I'd just suggest to do that for the new stuff since it's just pointless, unless we have a formatter support.

I won't block on the current order either, just noticed that it's usually other way around (you can also derive(Eq) if you have impl PartialEq).

Copy link
Member Author

@daxpedda daxpedda Jul 23, 2024

Choose a reason for hiding this comment

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

No unfortunately no formatter support ...
I will make a PR only for Web then, because Web follows alphabetical order so far.

src/platform_impl/web/async/dispatcher.rs Show resolved Hide resolved
src/platform_impl/web/monitor.rs Outdated Show resolved Hide resolved
src/platform_impl/web/monitor.rs Outdated Show resolved Hide resolved
src/platform_impl/web/web_sys/fullscreen.rs Outdated Show resolved Hide resolved
@daxpedda daxpedda force-pushed the web-monitor branch 3 times, most recently from a2739eb to 3075774 Compare July 23, 2024 16:01
Copy link
Member

@kchibisov kchibisov left a comment

Choose a reason for hiding this comment

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

  • already present comments about nesting, since the code is really hard to follow...

src/platform/web.rs Show resolved Hide resolved
src/platform_impl/web/event_loop/runner.rs Outdated Show resolved Hide resolved
@daxpedda daxpedda requested a review from kchibisov July 23, 2024 17:26
@daxpedda daxpedda merged commit a0bc3e5 into rust-windowing:master Jul 23, 2024
52 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

2 participants