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

Expected behavior of fullscreen and maximized in MacOS #460

Closed
edwin0cheng opened this issue Apr 13, 2018 · 15 comments
Closed

Expected behavior of fullscreen and maximized in MacOS #460

edwin0cheng opened this issue Apr 13, 2018 · 15 comments

Comments

@edwin0cheng
Copy link
Contributor

edwin0cheng commented Apr 13, 2018

After PR #457 is merged, I am going to implement following missing / incorrect (#66, #300) of display features in macos:

  • Window::set_fullscreen and WindowBuilder::with_fullscreen
  • Window::set_maximized and WindowBuilder::with_maximized
  • Window::set_decoration

However, i didn't know what is the expected behavior of "Full Screen" in macos :

  • Should full screen show / hide / auto-hide the menu bar ?
  • Should maximized show / hide / auto-hide the dock ?
  • If i implemented this changes, will it break others projects (e.g. alacritty @jwilm) ?
@mitchmindtree
Copy link
Contributor

mitchmindtree commented Apr 13, 2018

These are just my personal expectations, based on being a mac user for a long time (before recently switching to linux). The following also corresponds with how X11 behaves for the most part.

Should full screen show / hide / auto-hide the menu bar ?

Yes, the "inner area" of the window should take up the whole screen and no menubar or decorations should be visible. The menu bar behaviour that I would expect would be "auto-hide", where it only appears if you move your mouse cursor to the top of the screen.

One thing worth considering here is whether or not the fullscreen should occur in a new "desktop" as seems to be the behaviour of most macos apps. I'm not really sure what the right answer is here.

Should maximized show / hide / auto-hide the dock ?

I don't think hiding the dock is necessary - I would expect this to simply set the outer dimensions of the window to the size of the screen.

@edwin0cheng
Copy link
Contributor Author

edwin0cheng commented Apr 13, 2018

I don't think hiding the dock is necessary - I would expect this to simply set the outer dimensions of the window to the size of the screen.

When i clicked on "green icon" (e.g safari) , it will take up the whole screen but set the menubar and dock to auto-hide.
When i clicked on "green icon" with Option, it will not hide the dock, and just make the outer dimensions to the size of the screen which show the dock and menu bar.

So should we map set_maximized to ?:

  • green icon
  • green icon with Option

@mitchmindtree
Copy link
Contributor

Yes I think following safari's conventions is a good idea. That is, mapping set_maximized to "green icon with Option" and set_fullscreen to "green icon".

I cannot comment much further on auto-hiding the dock as I did not use the dock.

@sodiumjoe
Copy link
Contributor

Just to throw my two cents in here, I agree with following Safari's convention, which I believe is just macos convention. I imagine anyone who wants faux full-screen behavior that leaves the menu bar and dock intact would already be using a window manager that can achieve this (I use hammerspoon for this).

@edwin0cheng
Copy link
Contributor Author

edwin0cheng commented Apr 15, 2018

Indeed, i just implemented these features following standard macos convention, which is just calling NSWindow::toggleFullscreen: for fullscreen and NSWindow::zoom for maximized.

However, set_decoration and with_fullscreen are little bit tricky:

  • Calling toggleFullscreen: and zoom: on a non-resizable window (i.e set_decoration(false)) in MacOS is no-op by default, i have to do it manually (use setFrame for zoom) or set it to resizable tempority and restore it back. (for toggleFullScreen
  • Normally, the implementation of with_fullscreen is just calling toggleFullscreen after window initialization. However, when a winit app is launched from a fullscreen app (e.g. from a FullScreen VS Code Terminal), it creates a new virtual desktop and a transition animation, which takes one sec and cannot be disabled. In this animation time, all toggleFullscreen events will be failed. The PR just makes another toggleFullscreen call with delay until it succeeds.

@swiftcoder
Copy link
Contributor

Have we considered supporting two distinct fullscreen modes? One being the Safari-like mode that Mac desktop apps use at the current desktop resolution, with hidden menu bar and dock, and the other being a true fullscreen with captured display (custom resolution and no menu bar or dock at all) as games tend to use.

On Windows these equate to fullscreen borderless window, versus regular fullscreen (i.e. the choice of fullscreen modes that Blizzard presents in their games).

@edwin0cheng
Copy link
Contributor Author

edwin0cheng commented Apr 15, 2018

As i knew, in PR #270 , custom resolution are removed in X11 implementation, so i made the same change to #457. And in the MacOS side, i didn't implement resolution either.

The problem of custom resolution is, not all resolutions are supported by hardware, and there is still no api to query the supported resolution list right now. On the other hand, as stated in #270:

GPU scaling is cheap enough that apps should just scale to whatever the physical resolution is and not do any mode switching

For instance, let say we are using OpenGL as backend, it is not hard to create an framebuffer with custom resolution and then copy to screen frame buffer.

@vbo
Copy link

vbo commented Apr 17, 2018

What I would like is a fullscreen on a new "desktop" with dock and menubar hidden, maybe even cmd+tab disabled so application can handle it in a custom way. I made a pull request to the underlying cocoa library to support necessary APIs. Would be nice if we could expose this through winit.

@edwin0cheng
Copy link
Contributor Author

edwin0cheng commented Apr 17, 2018

@vbo yes, that PR is what we needed, right now i didn't touch the Fullscreen options, so the default will be used.
And we could expose this in WindowExt and WindowBuilderExt, but i think i would recommend to create another PR to address it.

@vbo
Copy link

vbo commented Apr 17, 2018

Sounds good, thanks @edwin0cheng. Let's revisit when PR is merged-in and see if I can add support in Window(Builder)Ext.

@edwin0cheng
Copy link
Contributor Author

edwin0cheng commented Apr 17, 2018

@vbo When i am trying to using NSView:: enterFullScreenMode:withOptions: , i found that NSViewFullScreenModeOptionKey is missing too, which i do not have any other options to bypass it, would you mind to bind it in your cocoa library PR too ?

@vbo
Copy link

vbo commented Apr 17, 2018

Sure, I'll take a look. That PR already merged-in though, so this is going to be separate.

@vbo
Copy link

vbo commented Apr 18, 2018

@edwin0cheng, just to clarify: you don't really need NSViewFullScreenModeOptionKey itself - this is just an alias for NSString *. Instead you want access to these constants, right?

@edwin0cheng
Copy link
Contributor Author

Yes, thanks

@edwin0cheng
Copy link
Contributor Author

As the PR #465 was merged, i would like to move the discussion to how to improve it. So i will close this issue and open #473 for improvements.

tmfink pushed a commit to tmfink/winit that referenced this issue Jan 5, 2022
…ples/canvas_webgl_minimal/www/lodash-4.17.21, r=jdm

Bump lodash from 4.17.19 to 4.17.21 in /examples/canvas_webgl_minimal/www

Bumps [lodash](https://github.com/lodash/lodash) from 4.17.19 to 4.17.21.
<details>
<summary>Commits</summary>
<ul>
<li><a href="https://github.com/lodash/lodash/commit/f299b52f39486275a9e6483b60a410e06520c538"><code>f299b52</code></a> Bump to v4.17.21</li>
<li><a href="https://github.com/lodash/lodash/commit/c4847ebe7d14540bb28a8b932a9ce1b9ecbfee1a"><code>c4847eb</code></a> Improve performance of <code>toNumber</code>, <code>trim</code> and <code>trimEnd</code> on large input strings</li>
<li><a href="https://github.com/lodash/lodash/commit/3469357cff396a26c363f8c1b5a91dde28ba4b1c"><code>3469357</code></a> Prevent command injection through <code>_.template</code>'s <code>variable</code> option</li>
<li><a href="https://github.com/lodash/lodash/commit/ded9bc66583ed0b4e3b7dc906206d40757b4a90a"><code>ded9bc6</code></a> Bump to v4.17.20.</li>
<li><a href="https://github.com/lodash/lodash/commit/63150ef7645ac07961b63a86490f419f356429aa"><code>63150ef</code></a> Documentation fixes.</li>
<li><a href="https://github.com/lodash/lodash/commit/00f0f62a979d2f5fa0287c06eae70cf9a62d8794"><code>00f0f62</code></a> test.js: Remove trailing comma.</li>
<li><a href="https://github.com/lodash/lodash/commit/846e434c7a5b5692c55ebf5715ed677b70a32389"><code>846e434</code></a> Temporarily use a custom fork of <code>lodash-cli</code>.</li>
<li><a href="https://github.com/lodash/lodash/commit/5d046f39cbd27f573914768e3b36eeefcc4f1229"><code>5d046f3</code></a> Re-enable Travis tests on <code>4.17</code> branch.</li>
<li><a href="https://github.com/lodash/lodash/commit/aa816b36d402a1ad9385142ce7188f17dae514fd"><code>aa816b3</code></a> Remove <code>/npm-package</code>.</li>
<li>See full diff in <a href="https://github.com/lodash/lodash/compare/4.17.19...4.17.21">compare view</a></li>
</ul>
</details>
<details>
<summary>Maintainer changes</summary>
<p>This version was pushed to npm by <a href="https://www.npmjs.com/~bnjmnt4n">bnjmnt4n</a>, a new releaser for lodash since your current version.</p>
</details>
<br />

[![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=lodash&package-manager=npm_and_yarn&previous-version=4.17.19&new-version=4.17.21)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores)

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot squash and merge` will squash and merge this PR after your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge and block automerging
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
- `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
- `@dependabot use these labels` will set the current labels as the default for future PRs for this repo and language
- `@dependabot use these reviewers` will set the current reviewers as the default for future PRs for this repo and language
- `@dependabot use these assignees` will set the current assignees as the default for future PRs for this repo and language
- `@dependabot use this milestone` will set the current milestone as the default for future PRs for this repo and language

You can disable automated security fix PRs for this repo from the [Security Alerts page](https://github.com/servo/pathfinder/network/alerts).

</details>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

5 participants