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

fix: setMaximizable to be true if window is resizable & maximizable #14648

Merged
merged 14 commits into from Sep 26, 2018

Conversation

Projects
None yet
5 participants
@Kthulu120
Contributor

Kthulu120 commented Sep 17, 2018

Description of Change

Fixes #13373

Currently on Windows when users have attempt to call setResizable(false) then setResizable(true) on Windows it disables the ability to be maximized again even though it is resizable. This PR adds this functionality.

Checklist
  • PR description included and stakeholders cc'd
  • npm test passes
  • tests are changed or added
  • relevant documentation is changed or added
  • PR title follows semantic commit guidelines

Notes: Allows the window to be maximization status to be restored to previous value upon being resizable again.

@Kthulu120 Kthulu120 requested a review from codebytere Sep 17, 2018

@Kthulu120 Kthulu120 requested a review from electron/reviewers as a code owner Sep 17, 2018

@Kthulu120 Kthulu120 changed the title from [wip] setMaximizable to be true if window is resizable & maximizable to [wip] fix: setMaximizable to be true if window is resizable & maximizable Sep 17, 2018

@codebytere

This comment has been minimized.

Show comment
Hide comment
@codebytere

codebytere Sep 17, 2018

Member

Could you check into a test for this fix to ensure it doesn't regress?

Member

codebytere commented Sep 17, 2018

Could you check into a test for this fix to ensure it doesn't regress?

@ckerr

This comment has been minimized.

Show comment
Hide comment
@ckerr

ckerr Sep 18, 2018

Member

Is this broken in 3.0.x as well? If so, it ought to have a target/3-0-x label too

Member

ckerr commented Sep 18, 2018

Is this broken in 3.0.x as well? If so, it ought to have a target/3-0-x label too

@Kthulu120

This comment has been minimized.

Show comment
Hide comment
@Kthulu120

Kthulu120 Sep 18, 2018

Contributor

This issue was reproduced in our official 3.0.0 version. Will be updating the setResizable(true) method to make the window maximizable by default, for now since its expected that a window that is now resizable also can be maximized.

Contributor

Kthulu120 commented Sep 18, 2018

This issue was reproduced in our official 3.0.0 version. Will be updating the setResizable(true) method to make the window maximizable by default, for now since its expected that a window that is now resizable also can be maximized.

@nornagon

This comment has been minimized.

Show comment
Hide comment
@nornagon

nornagon Sep 18, 2018

Contributor

hmm, i think that setResizable shouldn’t change the value of setMaximizable, i.e. if you have a window and you say win.setMaximizible(false); win.setResizable(true) then the window should be resizable but not maximizable.

if you can get a resizable non-maximizable window by running setResizable(true); setMaximizable(false) then i think the reverse order should also work, i.e. setMaximizable(false); setResizable(true) should do the same thing

Contributor

nornagon commented Sep 18, 2018

hmm, i think that setResizable shouldn’t change the value of setMaximizable, i.e. if you have a window and you say win.setMaximizible(false); win.setResizable(true) then the window should be resizable but not maximizable.

if you can get a resizable non-maximizable window by running setResizable(true); setMaximizable(false) then i think the reverse order should also work, i.e. setMaximizable(false); setResizable(true) should do the same thing

@Kthulu120

This comment has been minimized.

Show comment
Hide comment
@Kthulu120

Kthulu120 Sep 19, 2018

Contributor

@nornagon so my PR has changed a bit, its take the point you brought up into account. setResizable now resizes a window and sets the maxmizability of window to stay with what the initial value was before calling setResizable and allows isMaximizable() to return the correct value.

Contributor

Kthulu120 commented Sep 19, 2018

@nornagon so my PR has changed a bit, its take the point you brought up into account. setResizable now resizes a window and sets the maxmizability of window to stay with what the initial value was before calling setResizable and allows isMaximizable() to return the correct value.

@Kthulu120 Kthulu120 changed the title from [wip] fix: setMaximizable to be true if window is resizable & maximizable to fix: setMaximizable to be true if window is resizable & maximizable Sep 19, 2018

@codebytere

This comment has been minimized.

Show comment
Hide comment
@codebytere

codebytere Sep 20, 2018

Member

@Kthulu120 the following is failing on both win builds:

not ok 254 BrowserWindow module window states (excluding Linux) maximizable state (Windows only) is set to false when resizable state is set to false
  AssertionError [ERR_ASSERTION]: Input A expected to strictly equal input B:
  + expected - actual
  
  - true
  + false
      at Context.it (C:\projects\src\electron\spec\api-browser-window-spec.js:2618:16)
      at callFn (C:\projects\src\electron\spec\node_modules\mocha\lib\runnable.js:372:21)
      at Test.Runnable.run (C:\projects\src\electron\spec\node_modules\mocha\lib\runnable.js:364:7)
      at Runner.runTest (C:\projects\src\electron\spec\node_modules\mocha\lib\runner.js:455:10)
      at C:\projects\src\electron\spec\node_modules\mocha\lib\runner.js:573:12
      at next (C:\projects\src\electron\spec\node_modules\mocha\lib\runner.js:369:14)
      at C:\projects\src\electron\spec\node_modules\mocha\lib\runner.js:379:7
      at next (C:\projects\src\electron\spec\node_modules\mocha\lib\runner.js:303:14)
      at Immediate._onImmediate (C:\projects\src\electron\spec\node_modules\mocha\lib\runner.js:347:5)
      at runCallback (timers.js:696:18)
      at tryOnImmediate (timers.js:667:5)
      at processImmediate (timers.js:649:5)
Member

codebytere commented Sep 20, 2018

@Kthulu120 the following is failing on both win builds:

not ok 254 BrowserWindow module window states (excluding Linux) maximizable state (Windows only) is set to false when resizable state is set to false
  AssertionError [ERR_ASSERTION]: Input A expected to strictly equal input B:
  + expected - actual
  
  - true
  + false
      at Context.it (C:\projects\src\electron\spec\api-browser-window-spec.js:2618:16)
      at callFn (C:\projects\src\electron\spec\node_modules\mocha\lib\runnable.js:372:21)
      at Test.Runnable.run (C:\projects\src\electron\spec\node_modules\mocha\lib\runnable.js:364:7)
      at Runner.runTest (C:\projects\src\electron\spec\node_modules\mocha\lib\runner.js:455:10)
      at C:\projects\src\electron\spec\node_modules\mocha\lib\runner.js:573:12
      at next (C:\projects\src\electron\spec\node_modules\mocha\lib\runner.js:369:14)
      at C:\projects\src\electron\spec\node_modules\mocha\lib\runner.js:379:7
      at next (C:\projects\src\electron\spec\node_modules\mocha\lib\runner.js:303:14)
      at Immediate._onImmediate (C:\projects\src\electron\spec\node_modules\mocha\lib\runner.js:347:5)
      at runCallback (timers.js:696:18)
      at tryOnImmediate (timers.js:667:5)
      at processImmediate (timers.js:649:5)
@ckerr

ckerr approved these changes Sep 21, 2018

Looks like it'll need to be backported by hand, but that's not a blocker.

@Kthulu120

This comment has been minimized.

Show comment
Hide comment
@Kthulu120

Kthulu120 Sep 21, 2018

Contributor

Currently there are options when addressing this PR, we can allow the status quo of !resizable implies !maximizable while fixing resizable toggling maximizable or we can change the behavior of setResziable which will cause developers to have to explicitly call setMaximizable(false). This may be significant for some apps. But my suggestion is that in the end this provides more functionality that the developer controls that may allow for unforseen use cases. So I think the PR as is serves my suggestion as is, though I believe both options are right. If anyone has any objections feel free to comment otherwise we can get this merged.

Contributor

Kthulu120 commented Sep 21, 2018

Currently there are options when addressing this PR, we can allow the status quo of !resizable implies !maximizable while fixing resizable toggling maximizable or we can change the behavior of setResziable which will cause developers to have to explicitly call setMaximizable(false). This may be significant for some apps. But my suggestion is that in the end this provides more functionality that the developer controls that may allow for unforseen use cases. So I think the PR as is serves my suggestion as is, though I believe both options are right. If anyone has any objections feel free to comment otherwise we can get this merged.

@nornagon

This comment has been minimized.

Show comment
Hide comment
@nornagon

nornagon Sep 21, 2018

Contributor

I agree that being allowed to make a window that's maximizable but not resizable is more flexibility, but I'm not clear on that being something that's useful? I'm struggling to think of a use case for that combination of flags. I'd support maintaining the current behaviour that !resizable implies !maximizable.

Contributor

nornagon commented Sep 21, 2018

I agree that being allowed to make a window that's maximizable but not resizable is more flexibility, but I'm not clear on that being something that's useful? I'm struggling to think of a use case for that combination of flags. I'd support maintaining the current behaviour that !resizable implies !maximizable.

Kthulu120 added some commits Sep 24, 2018

@MarshallOfSound MarshallOfSound merged commit 560b1c1 into master Sep 26, 2018

17 checks passed

Semantic Pull Request ready to be squashed
Details
WIP ready for review
Details
appveyor: win-ia32-debug AppVeyor build succeeded
Details
appveyor: win-ia32-testing AppVeyor build succeeded
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-debug AppVeyor build succeeded
Details
appveyor: win-x64-testing AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
ci/circleci: electron-linux-arm-debug Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm-testing Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64-debug Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64-testing Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32-debug Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32-testing Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64-debug Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64-testing Your tests passed on CircleCI!
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Show comment
Hide comment
@release-clerk

release-clerk bot Sep 26, 2018

Release Notes Persisted

Allows the window to be maximization status to be restored to previous value upon being resizable again.

release-clerk bot commented Sep 26, 2018

Release Notes Persisted

Allows the window to be maximization status to be restored to previous value upon being resizable again.

@MarshallOfSound MarshallOfSound deleted the kthulu120/resizable branch Sep 26, 2018

Kthulu120 added a commit that referenced this pull request Oct 8, 2018

@Kthulu120 Kthulu120 referenced this pull request Oct 8, 2018

Merged

fix: Backport of #14648 #15025

4 of 4 tasks complete

ckerr added a commit that referenced this pull request Oct 9, 2018

trop-bot pushed a commit to trop-bot/electron that referenced this pull request Oct 9, 2018

jkleinsc added a commit that referenced this pull request Oct 9, 2018

@ckerr ckerr referenced this pull request Oct 12, 2018

Open

3.0.x auto-generated release notes are difficult to read #15116

0 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment