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: return MonitorHandle in Window::fullscreen() #3861

Merged
merged 1 commit into from
Aug 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions src/changelog/unreleased.md
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,11 @@ changelog entry.
Without prompting the user for permission, only the current monitor is returned. But when
prompting and being granted permission through
`ActiveEventLoop::request_detailed_monitor_permission()`, access to all monitors and their
information is available. This "detailed monitors" can be used in `Window::set_fullscreen()` as
well.
details is available. Handles created with "detailed monitor permissions" can be used in
`Window::set_fullscreen()` as well.

Keep in mind that handles do not auto-upgrade after permissions are granted and have to be
re-created to make full use of this feature.
- Add `Touch::finger_id` with a new type `FingerId`.
- On Web and Windows, add `FingerIdExt*::is_primary()`, exposing a way to determine
the primary finger in a multi-touch interaction.
Expand Down
11 changes: 5 additions & 6 deletions src/platform/web.rs
Original file line number Diff line number Diff line change
Expand Up @@ -699,10 +699,9 @@ pub struct OrientationData {
pub orientation: Orientation,
/// [`true`] if the [`orientation`](Self::orientation) is flipped upside down.
pub flipped: bool,
/// [`true`] if the [`Orientation`] is the most natural one for the screen regardless of being
/// flipped. Computer monitors are commonly naturally landscape mode, while mobile phones
/// are commonly naturally portrait mode.
pub natural: bool,
/// The most natural orientation for the screen. Computer monitors are commonly naturally
/// landscape mode, while mobile phones are commonly naturally portrait mode.
pub natural: Orientation,
}

/// Screen orientation.
Expand All @@ -726,14 +725,14 @@ pub enum OrientationLock {
/// User is locked to landscape mode.
Landscape {
/// - [`None`]: User is locked to both upright or upside down landscape mode.
/// - [`false`]: User is locked to upright landscape mode.
/// - [`true`]: User is locked to upright landscape mode.
/// - [`false`]: User is locked to upside down landscape mode.
flipped: Option<bool>,
},
/// User is locked to portrait mode.
Portrait {
/// - [`None`]: User is locked to both upright or upside down portrait mode.
/// - [`false`]: User is locked to upright portrait mode.
/// - [`true`]: User is locked to upright portrait mode.
/// - [`false`]: User is locked to upside down portrait mode.
flipped: Option<bool>,
},
Expand Down
8 changes: 4 additions & 4 deletions src/platform_impl/web/monitor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -322,22 +322,22 @@ impl Inner {
OrientationType::LandscapePrimary => OrientationData {
orientation: Orientation::Landscape,
flipped: false,
natural: angle == 0,
natural: if angle == 0 { Orientation::Landscape } else { Orientation::Portrait },
},
OrientationType::LandscapeSecondary => OrientationData {
orientation: Orientation::Landscape,
flipped: true,
natural: angle == 180,
natural: if angle == 180 { Orientation::Landscape } else { Orientation::Portrait },
},
OrientationType::PortraitPrimary => OrientationData {
orientation: Orientation::Portrait,
flipped: false,
natural: angle == 0,
natural: if angle == 0 { Orientation::Portrait } else { Orientation::Landscape },
},
OrientationType::PortraitSecondary => OrientationData {
orientation: Orientation::Portrait,
flipped: true,
natural: angle == 180,
natural: if angle == 180 { Orientation::Portrait } else { Orientation::Landscape },
},
_ => {
unreachable!("found unrecognized orientation: {}", orientation.type_string())
Expand Down
2 changes: 1 addition & 1 deletion src/platform_impl/web/window.rs
Original file line number Diff line number Diff line change
Expand Up @@ -312,7 +312,7 @@ impl Inner {
#[inline]
pub(crate) fn fullscreen(&self) -> Option<Fullscreen> {
if self.canvas.is_fullscreen() {
Some(Fullscreen::Borderless(None))
Some(Fullscreen::Borderless(Some(self.monitor.current_monitor())))
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if this would necessarily be better? Isn't the "correct" thing to do here to track which state the user originally requested to fullscreen in (custom monitor chosen, or no monitor chosen), and then return the same here? (Such that the assertion in the sequence window.set_fullscreen(fullscreen); assert_eq!(window.fullscreen(), fullscreen) would hold).

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 was unsure as well how this would play out, but after some testing I've learned that the current screen is always the one requested to be fullscreen, regardless of what it was before.

So yes, your assertion would hold.

Copy link
Member

Choose a reason for hiding this comment

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

I mean, if I set let fullscreen = Fullscreen::Borderless(None), the assertion no longer holds.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I see!
No, in that case it won't.

I'm not entirely sure what we should do here exactly, but I'm actually in favor of the current way Web works: always return the currently correct values, regardless of user input.
E.g. what do I do if set_fullscreen() fails? Should the assert hold until I receive the error?

This is all less then ideal and should be documented thoroughly ... so I'm gonna leave a comment in #3690.

} else {
None
}
Expand Down
2 changes: 1 addition & 1 deletion src/window.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1151,7 +1151,7 @@ impl Window {
/// - **iOS:** Can only be called on the main thread.
/// - **Android / Orbital:** Will always return `None`.
/// - **Wayland:** Can return `Borderless(None)` when there are no monitors.
/// - **Web:** Can only return `None` or `Borderless(None)`.
/// - **Web:** Can only return `None` or `Borderless`.
#[inline]
pub fn fullscreen(&self) -> Option<Fullscreen> {
let _span = tracing::debug_span!("winit::Window::fullscreen",).entered();
Expand Down
Loading