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

Improvements in BrowserWindow #4156

Merged
merged 7 commits into from Jan 23, 2016
Merged

Conversation

evgenyzinoviev
Copy link

  • movable option supported on Windows
  • added methods setMovable, isMovable
  • added options minimizable, closable and corresponding methods setMinimizable, isMinimizable, setClosable, isClosable. Implemented on Windows and OS X. This options controls whether specific window button and feature should be disabled or enabled.
  • On Windows implemented similar to OS X behaviour when fullscreen option is set to false. In this case the maximize button will be disabled.
  • Fixed a bug on OS X when resizable is set to false but the zoom button was not disabled when window is created
  • Fixed a bug on OS X when if you do win.setResizable(false); win.setResizable(true); win.setResizable(false), the zoom button was not disabled after the second setResizable(false) call

@sbruchmann
Copy link

I’m not a Windows user, but I don’t think that disabling fullscreen should disable maximizing windows.

@evgenyzinoviev
Copy link
Author

@sbruchmann Well, I agree that this is a bit strange. It's also a little strange to me how fullscreen option works on OS X. Another way is to add maximizable option for Windows.

@xywz
Copy link

xywz commented Jan 20, 2016

Could you add setFullscreenable and isFullscreenable?

@evgenyzinoviev
Copy link
Author

I'm afraid that fullscreenable will be easily confused with fullscreen. Moreover, "fullscreen" and "maximize" are different concepts. I think that adding maximizable option and setMaximizable and isMaximizable methods would be better solution. We could also improve API for OS X, make it more logical and clear.

Who are the maintainers here? I only know that @zcbenz is. What does he think?

@xywz
Copy link

xywz commented Jan 20, 2016

The reason for fullscreenable or a different name is to separate entering/exiting fullscreen from enabling/disabling fullscreen. It will improve the fullscreen API, making it more consistent, logical, and clear. This is unrelated to maximize.

@evgenyzinoviev
Copy link
Author

Now I understand. You suggest this for OS X, right?

@xywz
Copy link

xywz commented Jan 20, 2016

Right.

@zcbenz
Copy link
Member

zcbenz commented Jan 21, 2016

@evgenyzinoviev Adding fullscreenable and maximizable looks good to me, this PR is awesome!

.SetMethod("setMinimizable", &Window::SetMinimizable)
.SetMethod("isMinimizable", &Window::IsMinimizable)
.SetMethod("setClosable", &Window::SetClosable)
.SetMethod("isClosable", &Window::IsClosable)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little unsure whether closable or closeable is the right word here.

From a little googling, it looks like Java uses closeable

Copy link
Member

Choose a reason for hiding this comment

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

Cocoa uses closable: there is a NSClosableWindowMask flag.

Probably just use the shorter one.

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably just use the shorter one.

Sounds good 👍

@kevinsawicki
Copy link
Contributor

@evgenyzinoviev @zcbenz This will fix #2970 right?

@evgenyzinoviev
Copy link
Author

@kevinsawicki Yes. I'm gonna finish this tomorrow.

@evgenyzinoviev
Copy link
Author

Added the fullscreenable and maximizable options and methods.

@zcbenz
Copy link
Member

zcbenz commented Jan 23, 2016

Thanks!

zcbenz added a commit that referenced this pull request Jan 23, 2016
@zcbenz zcbenz merged commit 8cabe0f into electron:master Jan 23, 2016
@evgenyzinoviev evgenyzinoviev deleted the windows-pr branch January 24, 2016 17:17
@petercunha
Copy link

Thank you for adding this, much appreciated ❤️

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

Successfully merging this pull request may close these issues.

None yet

7 participants