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

Add window.nonNativeFullscreen option for macOS #55267

Merged
merged 14 commits into from Oct 25, 2018

Conversation

Projects
None yet
3 participants
@ignu
Contributor

ignu commented Jul 27, 2018

Now that Electron 2 is merged into master, we can have simpleFullScreen support!

Adds a new configuration option window.nonNativeFullScreen (defaulted to false to preserve the existing behavior) that, when true, will use setSimpleFullScreen and not cause the creation of a new space.

Fixes #5344

@msftclas

This comment has been minimized.

msftclas commented Jul 27, 2018

CLA assistant check
All CLA requirements met.

@ignu ignu force-pushed the ignu:non-native-fullscreen branch from 1a3fed8 to 2edf933 Jul 27, 2018

@kieferrm kieferrm requested a review from bpasero Jul 28, 2018

@bpasero bpasero modified the milestones: Backlog, August 2018 Aug 6, 2018

@bpasero

@ignu this is a great change, thanks for doing it. Looking at the screenshot from iTunes, I wonder if we should flip the setting around to be enabled by default and called window.nativeFullscreen

image

I would even literally copy the description they have there below the setting to VSCode to explain when it would make sense to disable that setting.

@bpasero

This comment has been minimized.

Member

bpasero commented Aug 7, 2018

@ignu one thing I noticed is that Sublime Text no longer seems to advertise this setting because when you enter simple full screen, it is hard to get out of this mode unless you know how you entered it. We could maybe show a message informing the user how to get out of the full screen mode if the native full screen mode is disabled?

@ignu

This comment has been minimized.

Contributor

ignu commented Aug 7, 2018

@bpasero That's a good point. I was thinking the default being a double-negative could be confusing. nonNativeFullScreen: false is a bit clunky.

But as much as I dislike Apple's native fullscreen, I'd worry about making "non-native" or "simple" the default. I'm not aware of any macOS app that does that, and it'd change Code's behavior and change it in a way that's not a macOS standard. I'm sure at least a few people would be upset.

Leaving the default native also prevents the confusion of accidentally getting into simpleFullScreen, as you're intentionally opting into it, presumably because you understand and prefer that behavior.

So I'd propose I get rid of the double negative and updating the description text as your proposed.

Maybe make the option and default one of:

  1. simpleFullScreen: false
  2. useSimpleFullScreen: false (this is what Sublime uses)
  3. nativeFullScreen: true
  4. useNativeFullScreen: true
@ignu

This comment has been minimized.

Contributor

ignu commented Aug 7, 2018

@bpasero updated.

I didn't completely grab that description as it is since it's a bit out of date. (When native full screen first launched, secondary monitors were completely useless.. they were just blanked out. That's no longer the case, but they're still not ideal since you can't drag windows from them to your natively full-screened app's window.)

I also prevented the maximize button from going into native fullscreen if nativeFullscreen==false.

@bpasero

This comment has been minimized.

Member

bpasero commented Aug 8, 2018

@ignu changes look good to me, however I am seeing two bugs, that seem to be related to the fullscreen support:

  • I am no longer able to quit VSCode when in simple fullscreen. Neither Cmd+Q nor the Quit menu entry work, do you see this too?
  • Right after I enter simple fullscreen, the window does not seem to have focus anymore, to reproduce, enter simple fullscreen, type Cmd+Shift+P to open the command palette and try to type, it does not work. it will only work if you click into the window
@ignu

This comment has been minimized.

Contributor

ignu commented Aug 9, 2018

That's really weird. I think CMD+Q's not working because it's like the entire application doesn't have focus? I'm poking around now.

@bpasero

This comment has been minimized.

Member

bpasero commented Aug 9, 2018

@ignu well it does not even work after I click back into the application. Maybe something is seriously broken in this mode in Electron already when you once set simple fullscreen.

@bpasero bpasero modified the milestones: August 2018, On Deck Aug 9, 2018

@bpasero bpasero referenced this pull request Aug 9, 2018

Closed

Simple fullscreen support #5344

@ignu

This comment has been minimized.

Contributor

ignu commented Aug 9, 2018

@bpasero I didn't find it in the electron docs, but I found this snippet in an electron nvim gui

https://github.com/igorgladkoborodov/vv/blob/60b513d4c728fe9a6d723d5b1d1808c2ef9ef634/src/renderer/nvim/features/fullScreen.js#L27

I added that to 476a89d and fixed most of the problems

There is a remaining bug... if you try to quit from simpleFullScreen, you're can't quit anymore, not through CMD+Q or even Code > Quite Visual Studio Code.

I think something must be puking trying to serialize, but I'm not totally sure. Still digging in.

@bpasero

This comment has been minimized.

Member

bpasero commented Aug 9, 2018

@ignu yeah I also thought about adding focus, but I did not try with webContents, good catch.

It could be that our very own logic prevents the quit, running with --verbose flag I see this in the console:

image

@ignu ignu force-pushed the ignu:non-native-fullscreen branch from 476a89d to a64ea32 Aug 9, 2018

@ignu ignu force-pushed the ignu:non-native-fullscreen branch from a64ea32 to 41cd80e Aug 9, 2018

@ignu

This comment has been minimized.

Contributor

ignu commented Aug 9, 2018

Yeah. I'm not fully groking everything SharedProcess does yet, and really confused as to why being in SimpleFullScreen would change its behavior.

Continued attempts to quit (even when backing out of full-screen) result in

[main 2:31:27 PM] windowsService#quit
[main 2:31:27 PM] Lifecycle#quit()
[main 2:31:27 PM] Lifecycle#quit() - a pending quit was found

but continued CMD+W's will eventually quit the app.

@bpasero

This comment has been minimized.

Member

bpasero commented Aug 10, 2018

@ignu turns out this is a known Electron bug, we will have to wait for a fix from them: electron/electron#13135

@bpasero bpasero removed this from the On Deck milestone Sep 9, 2018

@bpasero bpasero added this to the October 2018 milestone Oct 3, 2018

bpasero added some commits Oct 8, 2018

@bpasero

This comment has been minimized.

Member

bpasero commented Oct 8, 2018

@ignu I have updated master to Electron 2.0.11 and it looks like the issues are fixed (not being able to quit when in simple fullscreen).

I pushed a couple of cleanup changes to your branch and was about to merge, but then I figured out two things:

  • I see the window title visible when in simple fullscreen but would think the title should hide. maybe you could check it out and see if that is desired. to get that experience when running out of sources, change the following: comment out this line and this line

image

  • I see a NPE when switching between fullscreen and not and this seems like a new issue to me:

image

It could be that I did not see this issue before because I never tested running out of sources with the standard custom title behaviour (that needs the 2 changes I mention above).

Can you check?

@ignu

This comment has been minimized.

Contributor

ignu commented Oct 9, 2018

I'll look at that title issue tonight. I just realized it's not consistent with native fullscreen.

I've actually been using my own build of Code with these commits in it for the last month. I like the title there since my workflow (and part of why i hate native fullscreen) is having many projects open, and the title helps me identify which window I've Option-Tabbed to quickly... but it probably should be consistent with the native fullscreen version.

The other bug I have to fix that I've recently noticed is that if my resolution changes due to switching monitors Code is in simpleFullScreen but it hasn't actually updated its size to the new resolution.

@bpasero

This comment has been minimized.

Member

bpasero commented Oct 10, 2018

@ignu ok thanks. any thoughts on the exception in the layout() method?

@bpasero bpasero modified the milestones: October 2018, On Deck Oct 10, 2018

@ignu ignu force-pushed the ignu:non-native-fullscreen branch from 167a68c to 2b86094 Oct 16, 2018

@bpasero bpasero modified the milestones: On Deck, October 2018 Oct 25, 2018

@bpasero

This comment has been minimized.

Member

bpasero commented Oct 25, 2018

@ignu fyi please see 9667519 where I fixed the remaining issues. The title now correctly hides when in fullscreen mode and I added a workaround for the NPE in the layout call.

@bpasero bpasero merged commit d54b3a0 into Microsoft:master Oct 25, 2018

1 of 2 checks passed

VS Code in progress
Details
license/cla All CLA requirements met.
Details
@bpasero

This comment has been minimized.

Member

bpasero commented Oct 25, 2018

Thanks again 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment