Skip to content

Address Case of the Missing Monitor#7541

Merged
niik merged 3 commits intodesktop:developmentfrom
say25:missing-monitor-fix
May 21, 2019
Merged

Address Case of the Missing Monitor#7541
niik merged 3 commits intodesktop:developmentfrom
say25:missing-monitor-fix

Conversation

@say25
Copy link
Member

@say25 say25 commented May 16, 2019

Overview

Closes #7418
Closes #2107

Description

  • Updates electron-window-state to 5.0.3
  • In my personal testing this package update alone did not resolve this issue on Windows yet it did not make the problem any worse. Maybe I didn't test the reproduction steps properly? Maybe the fact that both my monitors are 4k contribute to my ability to reproduce?
  • This eliminates the need for jsonfile@^2.2.3

Release notes

Notes: [Fixed] Application window positioned off-screen when using multiple monitors on Windows.

@niik niik self-assigned this May 16, 2019
Copy link
Member

@niik niik left a comment

Choose a reason for hiding this comment

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

I've been able to reproduce the scenario outlined in #7418 on Windows (macOS seems to work just fine with the previous version as far as I can tell). Additionally, and more importantly, I've been able to confirm that the scenario is no longer reproducible with this bump. Here are my reproduction steps @say25 so that we can compare notes and perhaps figure out why you can still reproduce the issue even with the bumped package.

@say25 just to be certain, did you do a yarn build:dev before testing this? The functionality from electron-window-state is only being used in the main process and the main process needs to be rebuilt to pick up on package bumps like this.

My setup: Windows 8 laptop connected to a second display.

  1. Rebuild and launch from development
  2. Drag the window to the second display
  3. Close the application gracefully
    At this point my window-state.json file looked like this
{
"width": 1201,
"height": 826,
"x": -1890,
"y": -415,
"isMaximized": false,
"isFullScreen": false,
"displayBounds": { "x": -2560, "y": -576, "width": 2560, "height": 1440 }
}
  1. Disconnect the second monitor by disconnecting the cable
  2. Launch the app again, verify that it appears to be off-screen.
  3. Close the app, switch to this branch
  4. Re-connect the second monitor
  5. yarn && yarn rebuild:dev && yarn start
  6. Verify that the window appears on the second display where I left it
  7. Gracefully exit the application
  8. Disconnect the second monitor by disconnecting the cable
  9. yarn start
  10. Verify that the application now appears on the primary (only) display.

Taking a look at what's changed since our previous version (mawie81/electron-window-state@v4.0.2...v5.0.3) I see nothing obvious to be concerned about, the bump seems to be a safe one.

I do, however, see a very happy addition in that it appears that they've added typescript declarations now. Could you be so kind @say25 and go through and remove our any casts such that we could leverage the type system for this module?

@shiftkey shiftkey added the ready-for-review Pull Requests that are ready to be reviewed by the maintainers label May 16, 2019
@shiftkey
Copy link
Member

I've been able to reproduce the scenario outlined in #7418 on Windows (macOS seems to work just fine with the previous version as far as I can tell).

This is great news, and I'm tentatively going to add #2107 to the list of Fixes issues on this PR so we can close that out too!

@say25
Copy link
Member Author

say25 commented May 16, 2019

Shall remove the anys and retest tonight

@say25
Copy link
Member Author

say25 commented May 17, 2019

@niik & @shiftkey changes made, I will try and test again on my Windows 10 machine soon

@niik
Copy link
Member

niik commented May 21, 2019

@say25 I'm gonna go ahead and bring this in since I've been able to reproduce the original issue and confirm that the update resolves it for me. I'd still love it if you could test it on your machine so that we can feed that data into either a stronger confirmation that the issue is resolved or a new issue with specific troubleshooting data.

@niik niik merged commit 4a1d1ee into desktop:development May 21, 2019
@say25 say25 deleted the missing-monitor-fix branch May 22, 2019 21:32
@say25
Copy link
Member Author

say25 commented May 22, 2019

Seems like I can reproduce now (I recently had an issue with my machine and had to get a new motherboard maybe that was it?)

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

Labels

ready-for-review Pull Requests that are ready to be reviewed by the maintainers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

update electron-window-state to get improved checks for missing monitor Window hiding after using a second screen

3 participants