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

Maximization and Windowed Fullscreen #130

Merged
merged 8 commits into from
Aug 29, 2017

Conversation

pedrocr
Copy link
Contributor

@pedrocr pedrocr commented Jan 31, 2017

As suggested in #129 implement the ability to set the window to maximized and fullscreen. So far only implemented for X11 as that's the only thing I have access to.


#[inline]
pub fn set_fullscreen_windowed(&self, fullscreen: bool) {
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps these unimplemented methods should invoke unimplemented!() for now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought of that but it seemed to me it would make no sense to panic instead of noop for these things.

@tomaka
Copy link
Contributor

tomaka commented Feb 1, 2017

I'd like to rework the fullscreen/dimensions system.
Right now the way the state in the window builder and the state of the window are both very confusing.

@pedrocr
Copy link
Contributor Author

pedrocr commented Feb 1, 2017

@tomaka does that mean this needs to wait for that or do you have a sketch of what that rework would mean that you'd like a PR for?

@tomaka
Copy link
Contributor

tomaka commented Feb 1, 2017

@pedrocr Basically I'd like to have an API that can't be in an invalid state.
For example right now there's already the dimensions and monitor parameters that interact in weird ways. If you add fullscreen_windowed, that's yet another parameter.

In other words it should be impossible for the user to pass parameters that conflict with each other.
If we can't do otherwise, precisely document the behavior.

@pedrocr
Copy link
Contributor Author

pedrocr commented Feb 1, 2017

@tomaka ok, so I just made with_fullscreen() and with_fullscreen_windowed() mutually exclusive by disabling one when the other is set. It shouldn't really happen as users should choose one or the other, and at least in X11 setting both is harmless but it's better be safe than sorry. After creation fullscreen_windowed already interacts properly with the window sizes so I don't see any other bad interactions added by this.

@pedrocr
Copy link
Contributor Author

pedrocr commented Feb 4, 2017

@tomaka any opinion on this?

@tomaka
Copy link
Contributor

tomaka commented Feb 4, 2017

Ah forgot to come back here.

I didn't mean just methods. The methods are only helpers. What I want is an enum of some sort in the WindowAttributes struct.

EDIT: Be aware that the current API already doesn't respect that, which is why I wanted a small rework.

@pedrocr
Copy link
Contributor Author

pedrocr commented Feb 5, 2017

@tomaka, moved the fullscreen stuff into its own enum. See if that's what you meant.

@tomaka
Copy link
Contributor

tomaka commented Feb 5, 2017

That's a good start!
The dimensions (normal, min and max) should also be moved inside the Windowed variant.

Unfortunately now the problem is that the backends are broken.

@pedrocr
Copy link
Contributor Author

pedrocr commented Feb 5, 2017

Can't do much about the backends, I was just able to make sure X11 was working and Wayland should be too. Not sure what you mean about moving stuff into Windowed. At least on X11 the Windowed Fullscreen is just a hint you pass to the window manager who will resize the window for you. It will also remember the original window size and take you back to those dimensions when you unset the hint. So these fullscreen states and the dimensions are independent of each other.

@pedrocr
Copy link
Contributor Author

pedrocr commented May 17, 2017

This now has conflicts with the existing code. Is it still worth pursuing?

src/lib.rs Outdated
pub enum FullScreenState {
None,
Windowed,
Total(platform::MonitorId),
Copy link
Contributor

Choose a reason for hiding this comment

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

A more conventional name for this case would be Exclusive

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably, I don't have much experience with this kind of API to know what's usually used.

@pedrocr
Copy link
Contributor Author

pedrocr commented Aug 27, 2017

@tomaka cleaned up the commits and made them apply to current master. I discussed this with Ralith on IRC and we didn't see why it would make sense to move the dimensions into the fullscreen enum. This is also a pretty basic functionality so it would be important to expose it. Checks currently fail because I've only implemented X11 support and because glutin also needs a minor update to use the new fullscreen enum.

Implement a simple API to set a window to maximized. Implement it
only for the X11 backend.
@pedrocr pedrocr force-pushed the maximization branch 3 times, most recently from 3dfe855 to 1c7ddce Compare August 28, 2017 01:02
@pedrocr
Copy link
Contributor Author

pedrocr commented Aug 28, 2017

@tomaka @Ralith got all platforms to at least compile and be a noop for these settings. To get them to actually work would need access to the platforms themselves. I assume these don't make sense on iOS and Android so implementations are needed for MacOS, Windows and Wayland.

There are two kinds of fullscreen. One where you take over the whole
output the other where you just set the window size to the screen
size and get rid of decorations. The first one already existed,
implement the second which is more common for normal desktop apps.
Use an enum to consolidate all the fullscreen states.
@tomaka
Copy link
Contributor

tomaka commented Aug 28, 2017

One problem is that Wayland doesn't have a distinction between exclusive and windowed fullscreen. It's up to the server to decide.

@tomaka
Copy link
Contributor

tomaka commented Aug 28, 2017

And I think this API is full of corner cases. For example what if I create a window in exclusive fullscreen and call set_maximized(false)? (please don't answer this question in particular, this is just an example).

IMO there should be a clear state when it comes to windowed/fullscreen.

@pedrocr
Copy link
Contributor Author

pedrocr commented Aug 28, 2017

Wouldn't the solution just be to move maximized into the enum as well to make all the states mutually exclusive? And if on some platforms some states don't exist that's fine two, the given backend should just ignore the distinction, no?

@pedrocr
Copy link
Contributor Author

pedrocr commented Aug 28, 2017

On second thought I think the current API is already ok:

  • The two fullscreen states exist because on at least a few platforms they are different, backends are free to implement them the same way. Maybe there should be a set_fullscreen(bool) method or a more generalized set_fullscreen(Enum) one.
  • Maximization and Fullscreen are different concepts and all the combinations of states make sense. Setting both maximized and fullscreen just means "this window will turn into a maximized window with decorations if fullscreen is turned off".

@elinorbgr
Copy link
Contributor

Implementing this properly in Wayland will require some work on wayland-window to expose the missing methods, but I don't see any large difficulty in terms of exposing the functionality.

Now, it'll likely require some internal state-tracking, as the way these things are handled in wayland is rather that a window can be either regular, minimized, maximized or fullscreen. There is no notion of "both fullscreen and maximized".

@tomaka
Copy link
Contributor

tomaka commented Aug 28, 2017

I still don't understand how maximized and fullscreen are orthogonal. What would a fullscreen non-maximized window be?

@pedrocr
Copy link
Contributor Author

pedrocr commented Aug 28, 2017

So here's how most apps I've seen at least on X11 tend to work around maximized/fullscreen. They keep three types of state:

  • The window dimensions
  • Is the window maximized (X11 splits these into horizontal and vertical)?
  • Is the window fullscreen?

Then the following behavior happens:

  1. Start the app and set 800x600 size. Window shows up with that size plus decorations
  2. Call maximize (either by the app or the WM causing it). Window resizes to take over the whole screen minus decorations and any system bars
  3. Call windowed fullscreen or complete fullscreen. Window takes over the screen completely. In windowed fullscreen alt-tab still shifts to other apps. In complete fullscreen the app owns your display.
  4. Disable fullscreen. Window goes back to maximized
  5. Disable maximized. Window goes back to 800x600

If you consider maximized as not orthogonal to fullscreen step 4 and 5 get merged into one. You don't keep the previous state around, on every action you reset everything. This API also allows you to do that (just make the two calls) but provides the normal interaction where you get to unset only fullscreen or only maximized and have that state be tracked.

@tomaka
Copy link
Contributor

tomaka commented Aug 28, 2017

Ok that makes more sense. I had the same mindset as wayland, which is that a window is either maximized or fullscreen.

I guess I could just r+ this, and the API can always change later.

@pedrocr
Copy link
Contributor Author

pedrocr commented Aug 28, 2017

I think it makes sense to make the api .set_fullscreen(FullScreenState) and .with_fullscreen(FullScreenState).

Use the enum to make a single fullscreen API that's much more
consistent. Both set_fullscreen() and with_fullscreen() take the
same enum and support all the variations so you can build the window
however you want and switch between the modes at runtime.
@pedrocr
Copy link
Contributor Author

pedrocr commented Aug 29, 2017

Ok, pushed a commit to unify .with_fullscreen() and .with/.set_fullscreen_windowed(). The API looks much better now and the X11 code is a bit cleaner too.

@pedrocr pedrocr force-pushed the maximization branch 4 times, most recently from 2ab0b9f to 2d4365b Compare August 29, 2017 01:45
@tomaka
Copy link
Contributor

tomaka commented Aug 29, 2017

:shipit:

@tomaka tomaka merged commit f7a8bcd into rust-windowing:master Aug 29, 2017
@pedrocr
Copy link
Contributor Author

pedrocr commented Aug 29, 2017

😃

tmfink pushed a commit to tmfink/winit that referenced this pull request Jan 5, 2022
…t-no-floor

Don't draw the floor if the background is transparent
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants