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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/simple fullscreen mode #10254

Merged
merged 5 commits into from Sep 14, 2017

Conversation

Projects
None yet
7 participants
@zachflower
Contributor

zachflower commented Aug 13, 2017

Modern implementations of macOS utilize desktop spaces to implement fullscreen functionality, which presents a less-than-ideal interaction for users who use multiple monitors (and prefer to keep those monitors within the same space), or prefer to have more control over their ability to multitask while using a fullscreen application.

Prior to the release of Lion, fullscreen functionality on Mac OS X acted similarly to the current fullscreen functionality on Windows and Linux. This PR provides functionality to simulate that same style of pre-Lion fullscreen on macOS for Electron applications.

Full Disclosure: This is my first attempt at writing Objective-C... ever... so any feedback would be greatly appreciated! 馃槃

Usage

Simple fullscreen support can be enabled by passing simpleFullscreen: true to the BrowserWindow constructor which, when enabled, will override the default macOS fullscreen functionality with the new simple fullscreen functionality.

Additionally, two new methods (setSimpleFullScreen() and isFullScreen()) have been added for more granular usability.

Demonstration

Here is a short animation that demonstrates what simple fullscreen support looks like (pay attention to the window, as it stays on the current space rather than moving to a new one, which is the default functionality).

Background

Because popular desktop applications like Sublime Text 3, MacVim, and iTerm2 provide support for pre-Lion fullscreen, this particular feature has been requested many times over in several popular open source Electron projects (including the Electron core itself):

@neurosnap

This comment has been minimized.

neurosnap commented Aug 14, 2017

Couldn't users use Option + Click green button?

Tip: Want a bigger window without going full screen? Maximize the window by pressing and holding the Option key while you click the green maximize button . The window expands, but the menu bar and the Dock remain visible. To return to the previous window size, Option-click the button again.

https://support.apple.com/kb/PH25072?locale=en_US

@zachflower

This comment has been minimized.

Contributor

zachflower commented Aug 14, 2017

That's a good question, and a common one in the linked issues around this particular feature, but maximizing the window isn't the same thing as making it fullscreen.

From a user perspective, fullscreen means hiding the dock, title bar, and menu bar, which provides a more immersive experience and reduces distractions (at least to users who use pre-Lion fullscreen functionality on apps like iTerm2 and Sublime Text 2), while also not dictating how users should manage their desktops and monitors (which is what post-Lion fullscreen does).

From a practical perspective, fullscreen on Linux and Windows both operate in the same way as the pre-Lion fullscreen did (and does in this PR), while also offering separate "maximize window" features.

@subhaze

This comment has been minimized.

subhaze commented Aug 16, 2017

@zachflower You're my hero for the day! This has been one of the sad points when using any electron based app on macOS and one I greatly miss when outside of Sublime Text.

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Aug 16, 2017

@kibiz0r @mattrc Let's keep PRs clean of +1 comments please guys, if you like a PR, you can add the 馃憤 reaction to the PR itself.

@electron electron deleted a comment from kibiz0r Aug 17, 2017

@electron electron deleted a comment from April-xcz Aug 17, 2017

@elliotbonneville

This comment has been minimized.

elliotbonneville commented Aug 23, 2017

Just found this PR -- it was opened on my birthday.

Best. Birthday. Ever. 馃帀 I've been waiting for this for a very long time.

What needs to happen to get it merged into the main codebase so that between Hyper and Atom my development environment can reach its final form?

* `flag` Boolean
Sets whether the window should be in simple fullscreen mode.

This comment has been minimized.

@zcbenz

zcbenz Sep 11, 2017

Contributor

The description should make it clear what "simple fullscreen mode" is.

[window setFrame:fullscreenFrame display: YES animate: YES];
// Fullscreen windows can't be resized, minimized, etc.
if (was_minimizable_) SetMinimizable(false);

This comment has been minimized.

@zcbenz

zcbenz Sep 11, 2017

Contributor

The window states are just style masks, I think you can just store the original window style masks instead of separate window states.

This comment has been minimized.

@zachflower

zachflower Sep 12, 2017

Contributor

That makes a lot of sense, thanks for the feedback!

@@ -819,7 +826,9 @@ static bool FromV8(v8::Isolate* isolate, v8::Handle<v8::Value> val,
zoom_to_page_width_(false),
fullscreen_window_title_(false),
attention_request_id_(0),
title_bar_style_(NORMAL) {
title_bar_style_(NORMAL),
simple_fullscreen_(false),

This comment has been minimized.

@zcbenz

zcbenz Sep 13, 2017

Contributor

Having both simple_fullscreen_ and is_simple_fullscreen_ is confusing, renaming simple_fullscreen_ to something like always_simple_fullscreen_ can improve code readability.

This comment has been minimized.

@zachflower

zachflower Sep 13, 2017

Contributor

Good call, I can see how the current nomenclature is ambiguous. I will update accordingly.

@zcbenz

zcbenz approved these changes Sep 14, 2017

@zcbenz zcbenz merged commit a19a229 into electron:master Sep 14, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@welcome

This comment has been minimized.

welcome bot commented Sep 14, 2017

Congrats on merging your first pull request! 馃帀馃帀馃帀

@lynaghk

This comment has been minimized.

Contributor

lynaghk commented Dec 18, 2017

If app.dock.hide() has been called, the simple fullscreened window is short by the height of the OS X menu bar.
(Tested with Electron 1.8.2-beta.3).

I'm on OS X 10.9.5, which is too old for me to build Electron from source and fix.
However, I suspect the issue is related to this part of the pull request:

NSApplicationPresentationOptions options =
    NSApplicationPresentationAutoHideDock +
    NSApplicationPresentationAutoHideMenuBar;
[NSApp setPresentationOptions:options];

was_maximizable_ = IsMaximizable();
was_movable_ = IsMovable();

NSRect fullscreenFrame = [window.screen frame];

if ( !fullscreen_window_title() ) {
  // Hide the titlebar
  SetStyleMask(false, NSTitledWindowMask);

  // Resize the window to accomodate the _entire_ screen size
  fullscreenFrame.size.height -= [[[NSApplication sharedApplication] mainMenu] menuBarHeight];

My hypothesis for the behavior is that when NSApplicationPresentationAutoHideMenuBar is set, then the menuBarHeight call returns 0, but if app.dock.hide() has been called, then NSApplicationPresentationAutoHideMenuBar has no effect and so the last line of this snippet reduces the size by the menu bar height.

If that's what's actually happening, then perhaps the fix is to simply remove this last line.
@zachflower do you recall why you added this line?

Otherwise, super thrilled to have this option in Electron, since changing OS X spaces makes me seasick = )

@zachflower

This comment has been minimized.

Contributor

zachflower commented Dec 18, 2017

Hey @lynaghk!

IIRC this snippet:

fullscreenFrame.size.height -= [[[NSApplication sharedApplication] mainMenu] menuBarHeight];

Was necessary to work around an odd issue where the Electron window would receive an amount of padding equal to the height of the menubar, which resulted in an odd ~20px blank space at the top of the window.

That said, it doesn't seem take into account the possibility of the menubar having already been hidden (and I admit I don't remember if I tested that specific circumstance or not), so that might be the issue? Either way, good catch!

I'm not sure if I'll have time to jump on this soon with the holiday coming up, but鈥攃orrect me if I'm wrong with this direction @MarshallOfSound or @zcbenz鈥攊t's probably a good idea to create a new issue outlining the problem, in case someone else has more bandwidth to put out a fix in the next week or two.

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