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

Conversation

daxpedda
Copy link
Member

@daxpedda daxpedda commented Aug 13, 2024

  • Return MonitorHandle in Window::fullscreen()
  • Change OrientationData::natural type from bool to Orientation, just telling the user what the natural orientation is instead of letting the user "do the math".
  • Fix and improve some monitor related documentation.

Follow-up to #3801.

- Change `OrientationData::natural` type from `bool` to `Orientation`, just telling the user what the natural orientation is.
- Fix and improve some monitor related documentation.
@@ -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.

@daxpedda daxpedda merged commit a96491f into rust-windowing:master Aug 13, 2024
54 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