Fix: Validate state when saved display unavailable

[MathieuDebit]

What is this PR

This PR fixes the window positioning at startup when the saved display is unavailable.

See the corresponding issue: #44.
Related: #1, #4, #27, #31, #38, #43.

Commit aac92c0 shows that the current implementation is broken since the last test is failing (Ensure window is visible at startup if saved display is unavailable and was on the left).

How does it work

The solution currently adopted consists of reseting the state if the saved display is unavailable.

The advantage is that we ensure the window will always be visible if the saved display is not found. The downside is that the window will not be positioned to the saved coordinates if another display contains the corresponding bounds.

Better implementations that check for each display available and ensure the window is fully visible on one can be found in electron-boilerplate and VS Code.
See:

• electron-boilerplate:src/helpers/window.js
• vscode:src/vs/code/electron-main/window.ts

What about using their methods?

Test instructions

• Fetch this branch and run npm link
• Clone Bug: App Position with Second Screen MathieuDebit/electron-quick-start#3
• Run npm link electron-window-state
• Follow the reproductions steps on the pull request and verify that the bug is fixed

Todos

• Add Changelog
• Bump version

[mawie81]

Thanks a lot for the effort you are putting into this 👍
I'm fine with simply falling back to the default state in such cases.

I'll do some tests on Windows and Mac tomorrow.

If all is fine and there is nothing else you would like to add in here, I'm happy to merge.
I'd like to skip your giant .gitignore though 😉 ... lots of stuff that seems pretty unrelated in there.
I can do the Changelog and the version bumping when doing the release 😃

[mawie81]

So I did some tests on Mac and Windows. Mac is fine, but on Windows the app is still of screen.
I just positioned the app on the second screen and unplugged the primary one.
The state before starting again is looking like this:

{ 
  width: 1000,
  height: 326,
  x: 2577,
  y: 673,
  isMaximized: false,
  isFullScreen: false,
  displayBounds: { x: 0, y: 0, width: 1680, height: 1050 } 
}

When starting with the single monitor again the bound are fine and the window gets positioned at x=2577 and is off screen.
The strange thing is that the displayBounds in the state file is retrieved using screen.getDisplayMatching(winBounds).bounds and the winBounds in there had x=2577 🤔

So the only way a can currently think of fixing this, is the same approach like e.g. electron-boilerplate does. I quickly hacked their ensureVisibleOnSomeDisplay into the code and it worked.

[MathieuDebit]

Thanks for your tests 🙏

Actually the bug as described in my tests is not reproductible in MacOS because this OS seems to handle out-of-bounds windows differently. See #44 (comment). OSX has other issues (like unplug the second screen when the app was fullscreen).

Does your tests pass on Windows? Would you think of a new test that would fail on your case? Unfortunately I can't manage to reproduce your issue 😕 I tried different ways but it's always working for me.

And I added this big general gitignore as it covers pretty all useless file which is nice for open-source projects, but okay I'm removing it!

[mawie81]

Here is a test that fails and would render the app off screen:

test('test', t => {
  const jsonfile = require('jsonfile');
  sinon.stub(jsonfile, 'readFileSync').returns({
    width: 1000,
    height: 326,
    x: 2577,
    y: 673,
    isMaximized: false,
    isFullScreen: false,
    displayBounds: {x: 0, y: 0, width: 1680, height: 1050}
  });

  const {screen} = require('electron');
  const screenBounds = {x: 0, y: 0, width: 1680, height: 1050};
  sinon.stub(screen, 'getDisplayMatching').returns({bounds: screenBounds});

  const state = require('.')({
    defaultWidth: 500,
    defaultHeight: 300
  });

  t.is(state.x, 0);
  t.is(state.y, 0);
  t.is(state.width, 500);
  t.is(state.height, 300);
  t.is(state.displayBounds, screenBounds);

  jsonfile.readFileSync.restore();
  screen.getDisplayMatching.restore();
});

[MathieuDebit]

Ok I see, of course the state is not reset because the saved screen is found. It would be interesting to know how you got this state 🤔

Anyway I'm adding the ensureVisibleOnSomeDisplay method.

Another nice feature would also be to add a resetState method that could be triggered at anytime by the app (via a menu for example) and let the user bring back the window to the main display if lost. What do you think?

[tobiasmuecksch]

I've implemented a fix that works fine on windows, but it doesn't work on macOS. Maybe we can merge our solutions somehow. Well at least it stops working on macOS as soon as you build it. When running it directly from the CLI it's working perfectly fine on macOS too. So strange...

You can find my solution here: burningparrot@d906c7f

[MathieuDebit]

Hi @tobiasmuecksch 👋

Sure we could merge our solutions! Weird it crashes only on build, do you have a test repo with an electron-quick-start?

I'm working on implementing the ensureVisibleOnSomeDisplay method from electron-boilerplate but I have some issues on tests 😅

[MathieuDebit]

I just pushed a ensureWindowVisibleOnSomeDisplay() method that fixes @mawie81's previous issue. Unfortunately there is still a failing test in one particular case 😅

See Validate state if saved display is available and secondary on left:

electron-window-state/test.js

Lines 256 to 292 in 477f3db

	test('Validate state if saved display is available and secondary on left', t => {
	const primaryDisplayBounds = {x: 0, y: 0, width: 1920, height: 1080};
	const secondaryDisplayBounds = {x: -2560, y: -480, width: 2560, height: 1440};
	const jsonfile = require('jsonfile');
	sinon.stub(jsonfile, 'readFileSync').returns({
	x: -2000,
	y: -1000,
	width: 800,
	height: 600,
	displayBounds: secondaryDisplayBounds
	});
	const {screen} = require('electron');
	sinon.stub(screen, 'getDisplayMatching').returns({bounds: secondaryDisplayBounds});
	sinon.stub(screen, 'getPrimaryDisplay').returns({bounds: primaryDisplayBounds});
	sinon.stub(screen, 'getAllDisplays').returns([
	{bounds: primaryDisplayBounds},
	{bounds: secondaryDisplayBounds}
	]);
	const state = require('.')({
	defaultWidth: 500,
	defaultHeight: 300
	});
	t.is(state.x, -2000);
	t.is(state.y, -1000);
	t.is(state.width, 800);
	t.is(state.height, 600);
	t.is(state.displayBounds, secondaryDisplayBounds);
	jsonfile.readFileSync.restore();
	screen.getDisplayMatching.restore();
	screen.getPrimaryDisplay.restore();
	screen.getAllDisplays.restore();
	});

Still working on it 👨🏻‍🔧

[tobiasmuecksch]

Well, my solution doesn't crash. It's just not working as expected. It always places the window on the primary screen on macOS. I haven't used the built-in tests in this repo yet.

[tobiasmuecksch]

Why do you try to make sure, that the bounds are equal? Isn't that irrelevant? The only thing that should matter - is the window visible? What if I switch the display inplace. Let's say my second screen at work is slightly bigger than at home. But I still want the app to open at the same place.

[MathieuDebit]

Yes you right I deleted it in the last commit ee6c18c!

[tobiasmuecksch]

I'm sorry. I haven't found out yet how to have a look at the current codebase of your fork.

[MathieuDebit]

@mawie81 I just removed the last failing test and propose to open another PR to fix it. This way we can merge this one that fixes a lot of issues for now.

Ready for review ✨

[tobiasmuecksch]

I agree. What do you think @mawie81 ?

[tobiasmuecksch]

I'd love to test this pull request, but I don't find any button or something alike to check it out.

[MathieuDebit]

@tobiasmuecksch The fork is https://github.com/getstation/electron-window-state.
You can clone the branch this way:

git clone https://github.com/getstation/electron-window-state.git -b fix/validate-state-when-saved-display-unavailable
cd electron-window-state
npm install
npm link

Otherwise on can put

"electron-window-state": "github:mawie81/electron-window-state#pull/45/head"

in your package.json to install this PR directly ✨

[tobiasmuecksch]

@MathieuDebit Thank you, I was looking for the URL. Will test it over the weekend.

[mawie81]

I could only give a short test, but I think the window is now no longer rendered off screen 👍

However I encountered a new issue on Windows when re-opening a previously maximized window. There are cases where the window is re-created using the defaults.
But that might be unrelated to the fix or the broader issue just surfaced because of the fix.
I quick scan over the code made me think that this could have something to do with not updating the window dimension if the window is maximized. How do we know on which display to restore the maximized window 🤔

Will try to find some time on the weekend to dig deeper.
Will also think about merging this and tackle my current issue after that.

[MathieuDebit]

Yes there is this kind of issue on MacOS too! You right we could handle this in another issue, let's validate this PR first 🎉

[tobiasmuecksch]

@MathieuDebit Have you tested this on macOS Mojave? When I run the app using electron electron/main.js it works fine. But when I build the app using electron-builder --win --mac it stops working. The app always restores on the main monitor.

I have also tested this with the electron-boilerplate app, and the issue doesn't occur. It works perfectly.

[MathieuDebit]

Sure I tested it on OSX Mojave and Windows 10, I also published a version on my fork (https://www.npmjs.com/package/@getstation/electron-window-state). It builds correctly.

Do you have enough material to open a new issue?

[tobiasmuecksch]

I don't have problems with building. My problem is, that the app won't restore the position correctly, if I install and run the built version.

[MathieuDebit]

Hmm that's weird 🤔I don't know how this could happen, but we can handle this in another issue with reproduction steps and I'll look into it!

[tobiasmuecksch]

Thanks. I'll create a demo repo and open an issue