From 67f33cdb60fa09e9a1373419df4ec8a5e7fe089b Mon Sep 17 00:00:00 2001 From: Samuel Attard Date: Tue, 22 Nov 2016 15:49:47 +1100 Subject: [PATCH 1/9] Add failing spec for window size after restore --- spec/api-browser-window-spec.js | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/spec/api-browser-window-spec.js b/spec/api-browser-window-spec.js index a0425d7ffc63..83a9ed8d49a5 100644 --- a/spec/api-browser-window-spec.js +++ b/spec/api-browser-window-spec.js @@ -1754,6 +1754,25 @@ describe('browser-window module', function () { }) }) }) + + describe.only('minWidth', function () { + beforeEach(function () { + if (w != null) w.destroy() + w = new BrowserWindow({ + minWidth: 800, + width: 800 + }) + }) + + it('should persist when restoring the window', function (done) { + w.minimize(); + setTimeout(() => { + w.restore(); + assert.equal(w.getSize()[0], 800); + done(); + }, 200); + }) + }) }) const assertBoundsEqual = (actual, expect) => { From c65033a13b7ec349e10cf54f47a81f12997db052 Mon Sep 17 00:00:00 2001 From: Samuel Attard Date: Tue, 22 Nov 2016 15:49:56 +1100 Subject: [PATCH 2/9] Revert "incorrect position when restored from maximize-on-top-drag under Windows #7630" This reverts commit a2b3abbf47f2adc8f945f3e12ce6f756670d03ca. --- atom/browser/native_window_views.cc | 1 + atom/browser/native_window_views.h | 16 ++++++++ atom/browser/native_window_views_win.cc | 51 ++++++++++++++++++------- 3 files changed, 55 insertions(+), 13 deletions(-) diff --git a/atom/browser/native_window_views.cc b/atom/browser/native_window_views.cc index 4a8bc16ef6f2..2ae36bc36b3a 100644 --- a/atom/browser/native_window_views.cc +++ b/atom/browser/native_window_views.cc @@ -327,6 +327,7 @@ NativeWindowViews::NativeWindowViews( last_window_state_ = ui::SHOW_STATE_FULLSCREEN; else last_window_state_ = ui::SHOW_STATE_NORMAL; + last_normal_bounds_ = GetBounds(); #endif } diff --git a/atom/browser/native_window_views.h b/atom/browser/native_window_views.h index 379dc4ceaaf3..caaf64ba7134 100644 --- a/atom/browser/native_window_views.h +++ b/atom/browser/native_window_views.h @@ -211,6 +211,22 @@ class NativeWindowViews : public NativeWindow, ui::WindowShowState last_window_state_; + // There's an issue with restore on Windows, that sometimes causes the Window + // to receive the wrong size (#2498). To circumvent that, we keep tabs on the + // size of the window while in the normal state (not maximized, minimized or + // fullscreen), so we restore it correctly. + gfx::Rect last_normal_bounds_; + + // last_normal_bounds_ may or may not require update on WM_MOVE. When a + // window is maximized, it is moved (WM_MOVE) to maximum size first and then + // sized (WM_SIZE). In this case, last_normal_bounds_ should not update. We + // keep last_normal_bounds_candidate_ as a candidate which will become valid + // last_normal_bounds_ if the moves are consecutive with no WM_SIZE event in + // between. + gfx::Rect last_normal_bounds_candidate_; + + bool consecutive_moves_; + // In charge of running taskbar related APIs. TaskbarHost taskbar_host_; diff --git a/atom/browser/native_window_views_win.cc b/atom/browser/native_window_views_win.cc index d6f57b6aead8..85230addb0c1 100644 --- a/atom/browser/native_window_views_win.cc +++ b/atom/browser/native_window_views_win.cc @@ -125,6 +125,7 @@ bool NativeWindowViews::PreHandleMSG( return taskbar_host_.HandleThumbarButtonEvent(LOWORD(w_param)); return false; case WM_SIZE: { + consecutive_moves_ = false; // Handle window state change. HandleSizeEvent(w_param, l_param); return false; @@ -134,6 +135,15 @@ bool NativeWindowViews::PreHandleMSG( ::GetWindowRect(GetAcceleratedWidget(), (LPRECT)l_param); return false; } + case WM_MOVE: { + if (last_window_state_ == ui::SHOW_STATE_NORMAL) { + if (consecutive_moves_) + last_normal_bounds_ = last_normal_bounds_candidate_; + last_normal_bounds_candidate_ = GetBounds(); + consecutive_moves_ = true; + } + return false; + } default: return false; } @@ -152,20 +162,35 @@ void NativeWindowViews::HandleSizeEvent(WPARAM w_param, LPARAM l_param) { NotifyWindowMinimize(); break; case SIZE_RESTORED: - switch (last_window_state_) { - case ui::SHOW_STATE_MAXIMIZED: - last_window_state_ = ui::SHOW_STATE_NORMAL; - NotifyWindowUnmaximize(); - break; - case ui::SHOW_STATE_MINIMIZED: - if (IsFullscreen()) { - last_window_state_ = ui::SHOW_STATE_FULLSCREEN; - NotifyWindowEnterFullScreen(); - } else { + if (last_window_state_ == ui::SHOW_STATE_NORMAL) { + // Window was resized so we save it's new size. + last_normal_bounds_ = GetBounds(); + } else { + switch (last_window_state_) { + case ui::SHOW_STATE_MAXIMIZED: last_window_state_ = ui::SHOW_STATE_NORMAL; - NotifyWindowRestore(); - } - break; + + // When the window is restored we resize it to the previous known + // normal size. + SetBounds(last_normal_bounds_, false); + + NotifyWindowUnmaximize(); + break; + case ui::SHOW_STATE_MINIMIZED: + if (IsFullscreen()) { + last_window_state_ = ui::SHOW_STATE_FULLSCREEN; + NotifyWindowEnterFullScreen(); + } else { + last_window_state_ = ui::SHOW_STATE_NORMAL; + + // When the window is restored we resize it to the previous known + // normal size. + SetBounds(last_normal_bounds_, false); + + NotifyWindowRestore(); + } + break; + } } break; } From 621a934160ecabb99f6e667930978c009ba0928b Mon Sep 17 00:00:00 2001 From: Samuel Attard Date: Tue, 22 Nov 2016 16:07:05 +1100 Subject: [PATCH 3/9] Fix maximize --> unmaximize positioning issue --- atom/browser/native_window_views.h | 1 + atom/browser/native_window_views_win.cc | 14 ++++++++++---- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/atom/browser/native_window_views.h b/atom/browser/native_window_views.h index caaf64ba7134..110a4e9d4373 100644 --- a/atom/browser/native_window_views.h +++ b/atom/browser/native_window_views.h @@ -216,6 +216,7 @@ class NativeWindowViews : public NativeWindow, // size of the window while in the normal state (not maximized, minimized or // fullscreen), so we restore it correctly. gfx::Rect last_normal_bounds_; + gfx::Rect last_normal_bounds_before_move_; // last_normal_bounds_ may or may not require update on WM_MOVE. When a // window is maximized, it is moved (WM_MOVE) to maximum size first and then diff --git a/atom/browser/native_window_views_win.cc b/atom/browser/native_window_views_win.cc index 85230addb0c1..1b523e90b804 100644 --- a/atom/browser/native_window_views_win.cc +++ b/atom/browser/native_window_views_win.cc @@ -125,9 +125,12 @@ bool NativeWindowViews::PreHandleMSG( return taskbar_host_.HandleThumbarButtonEvent(LOWORD(w_param)); return false; case WM_SIZE: { - consecutive_moves_ = false; // Handle window state change. HandleSizeEvent(w_param, l_param); + + consecutive_moves_ = false; + last_normal_bounds_before_move_ = last_normal_bounds_; + return false; } case WM_MOVING: { @@ -155,6 +158,9 @@ void NativeWindowViews::HandleSizeEvent(WPARAM w_param, LPARAM l_param) { switch (w_param) { case SIZE_MAXIMIZED: last_window_state_ = ui::SHOW_STATE_MAXIMIZED; + if (consecutive_moves_) { + last_normal_bounds_ = last_normal_bounds_before_move_; + } NotifyWindowMaximize(); break; case SIZE_MINIMIZED: @@ -165,14 +171,14 @@ void NativeWindowViews::HandleSizeEvent(WPARAM w_param, LPARAM l_param) { if (last_window_state_ == ui::SHOW_STATE_NORMAL) { // Window was resized so we save it's new size. last_normal_bounds_ = GetBounds(); + last_normal_bounds_before_move_ = last_normal_bounds_; } else { switch (last_window_state_) { case ui::SHOW_STATE_MAXIMIZED: last_window_state_ = ui::SHOW_STATE_NORMAL; - // When the window is restored we resize it to the previous known - // normal size. - SetBounds(last_normal_bounds_, false); + // Don't force out last known bounds onto the window as Windows + // actually gets these correct NotifyWindowUnmaximize(); break; From 926cabec784743d1d560753c90f9be08df64dd40 Mon Sep 17 00:00:00 2001 From: Samuel Attard Date: Tue, 22 Nov 2016 16:07:19 +1100 Subject: [PATCH 4/9] Add spec for maximize restore positioning --- spec/api-browser-window-spec.js | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/spec/api-browser-window-spec.js b/spec/api-browser-window-spec.js index 83a9ed8d49a5..b44e2a5093bb 100644 --- a/spec/api-browser-window-spec.js +++ b/spec/api-browser-window-spec.js @@ -1755,7 +1755,7 @@ describe('browser-window module', function () { }) }) - describe.only('minWidth', function () { + describe('minWidth', function () { beforeEach(function () { if (w != null) w.destroy() w = new BrowserWindow({ @@ -1764,7 +1764,7 @@ describe('browser-window module', function () { }) }) - it('should persist when restoring the window', function (done) { + it('should not affect the bounds when restoring the window', function (done) { w.minimize(); setTimeout(() => { w.restore(); @@ -1773,6 +1773,23 @@ describe('browser-window module', function () { }, 200); }) }) + + describe.only('window position', function () { + beforeEach(function () { + if (w != null) w.destroy() + w = new BrowserWindow() + }) + + it('should not affect the bounds when restoring the window', function (done) { + const originalPos = w.getPosition(); + w.maximize(); + setTimeout(() => { + w.unmaximize(); + assertBoundsEqual(originalPos, w.getPosition()); + done(); + }, 200); + }) + }) }) const assertBoundsEqual = (actual, expect) => { From 97b4f121121f76d9ff2d5fe944c1d37d943e4572 Mon Sep 17 00:00:00 2001 From: Samuel Attard Date: Tue, 22 Nov 2016 16:13:08 +1100 Subject: [PATCH 5/9] Fix linting issues --- spec/api-browser-window-spec.js | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/spec/api-browser-window-spec.js b/spec/api-browser-window-spec.js index b44e2a5093bb..196ab1c94d5c 100644 --- a/spec/api-browser-window-spec.js +++ b/spec/api-browser-window-spec.js @@ -1765,12 +1765,12 @@ describe('browser-window module', function () { }) it('should not affect the bounds when restoring the window', function (done) { - w.minimize(); + w.minimize() setTimeout(() => { - w.restore(); - assert.equal(w.getSize()[0], 800); - done(); - }, 200); + w.restore() + assert.equal(w.getSize()[0], 800) + done() + }, 200) }) }) @@ -1781,13 +1781,13 @@ describe('browser-window module', function () { }) it('should not affect the bounds when restoring the window', function (done) { - const originalPos = w.getPosition(); - w.maximize(); + const originalPos = w.getPosition() + w.maximize() setTimeout(() => { - w.unmaximize(); - assertBoundsEqual(originalPos, w.getPosition()); - done(); - }, 200); + w.unmaximize() + assertBoundsEqual(originalPos, w.getPosition()) + done() + }, 200) }) }) }) From 238beb72ee9f15820a4639214cf18a27d735333f Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Tue, 22 Nov 2016 11:41:02 -0800 Subject: [PATCH 6/9] Remove .only call --- spec/api-browser-window-spec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/api-browser-window-spec.js b/spec/api-browser-window-spec.js index 196ab1c94d5c..d5d5b00f694c 100644 --- a/spec/api-browser-window-spec.js +++ b/spec/api-browser-window-spec.js @@ -1774,7 +1774,7 @@ describe('browser-window module', function () { }) }) - describe.only('window position', function () { + describe('window position', function () { beforeEach(function () { if (w != null) w.destroy() w = new BrowserWindow() From c0d9175bc3018204b55e8ab7edd0e832260168e6 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Tue, 22 Nov 2016 13:13:30 -0800 Subject: [PATCH 7/9] Try specs without setTimeout calls --- spec/api-browser-window-spec.js | 24 +++++++++--------------- 1 file changed, 9 insertions(+), 15 deletions(-) diff --git a/spec/api-browser-window-spec.js b/spec/api-browser-window-spec.js index d5d5b00f694c..75e4dab0b262 100644 --- a/spec/api-browser-window-spec.js +++ b/spec/api-browser-window-spec.js @@ -1755,7 +1755,7 @@ describe('browser-window module', function () { }) }) - describe('minWidth', function () { + describe('Restoring the window', function () { beforeEach(function () { if (w != null) w.destroy() w = new BrowserWindow({ @@ -1764,30 +1764,24 @@ describe('browser-window module', function () { }) }) - it('should not affect the bounds when restoring the window', function (done) { + it('should set the correct width when minWidth is used', function () { w.minimize() - setTimeout(() => { - w.restore() - assert.equal(w.getSize()[0], 800) - done() - }, 200) + w.restore() + assert.equal(w.getSize()[0], 800) }) }) - describe('window position', function () { + describe('Unmaximizing the window', function () { beforeEach(function () { if (w != null) w.destroy() w = new BrowserWindow() }) - it('should not affect the bounds when restoring the window', function (done) { - const originalPos = w.getPosition() + it('should set the correct position', function () { + const initialPosition = w.getPosition() w.maximize() - setTimeout(() => { - w.unmaximize() - assertBoundsEqual(originalPos, w.getPosition()) - done() - }, 200) + w.unmaximize() + assertBoundsEqual(initialPosition, w.getPosition()) }) }) }) From 63eec70350f60d8b435b2c3abcd66a92b9611fc2 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Tue, 22 Nov 2016 13:20:01 -0800 Subject: [PATCH 8/9] Assert bounds when restoring --- spec/api-browser-window-spec.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/spec/api-browser-window-spec.js b/spec/api-browser-window-spec.js index 75e4dab0b262..f18322bbf40a 100644 --- a/spec/api-browser-window-spec.js +++ b/spec/api-browser-window-spec.js @@ -1764,10 +1764,11 @@ describe('browser-window module', function () { }) }) - it('should set the correct width when minWidth is used', function () { + it('should restore the correct bounds when minWidth is used', function () { + const initialSize = w.getSize() w.minimize() w.restore() - assert.equal(w.getSize()[0], 800) + assertBoundsEqual(w.getSize(), initialSize) }) }) @@ -1781,7 +1782,7 @@ describe('browser-window module', function () { const initialPosition = w.getPosition() w.maximize() w.unmaximize() - assertBoundsEqual(initialPosition, w.getPosition()) + assertBoundsEqual(w.getPosition(), initialPosition) }) }) }) From 6f29d7211e0d6fec049301002c787de159202afc Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Tue, 22 Nov 2016 13:26:39 -0800 Subject: [PATCH 9/9] Move restore/unmaximize specs near state ones --- spec/api-browser-window-spec.js | 59 ++++++++++++++++----------------- 1 file changed, 28 insertions(+), 31 deletions(-) diff --git a/spec/api-browser-window-spec.js b/spec/api-browser-window-spec.js index f18322bbf40a..f7aa6436a47c 100644 --- a/spec/api-browser-window-spec.js +++ b/spec/api-browser-window-spec.js @@ -1320,6 +1320,34 @@ describe('browser-window module', function () { }) }) + describe('BrowserWindow.restore()', function () { + it('should restore the previous window size', function () { + if (w != null) w.destroy() + + w = new BrowserWindow({ + minWidth: 800, + width: 800 + }) + + const initialSize = w.getSize() + w.minimize() + w.restore() + assertBoundsEqual(w.getSize(), initialSize) + }) + }) + + describe('BrowserWindow.unmaximize()', function () { + it('should restore the previous window position', function () { + if (w != null) w.destroy() + w = new BrowserWindow() + + const initialPosition = w.getPosition() + w.maximize() + w.unmaximize() + assertBoundsEqual(w.getPosition(), initialPosition) + }) + }) + describe('parent window', function () { let c = null @@ -1754,37 +1782,6 @@ describe('browser-window module', function () { }) }) }) - - describe('Restoring the window', function () { - beforeEach(function () { - if (w != null) w.destroy() - w = new BrowserWindow({ - minWidth: 800, - width: 800 - }) - }) - - it('should restore the correct bounds when minWidth is used', function () { - const initialSize = w.getSize() - w.minimize() - w.restore() - assertBoundsEqual(w.getSize(), initialSize) - }) - }) - - describe('Unmaximizing the window', function () { - beforeEach(function () { - if (w != null) w.destroy() - w = new BrowserWindow() - }) - - it('should set the correct position', function () { - const initialPosition = w.getPosition() - w.maximize() - w.unmaximize() - assertBoundsEqual(w.getPosition(), initialPosition) - }) - }) }) const assertBoundsEqual = (actual, expect) => {