Fixing #1869. Ensuring that the restored window position is valid #2459

Xapphire13 · 2017-11-15T01:54:28Z

I have also added unit tests for the change. And removed unit tests for files that no longer exist (does this imply UTs aren't run for this project?).

chabou · 2017-11-15T09:01:08Z

Thank you so much for this PR.
Indeed UTs are not systematically run for this project. And this is bad.
I have broken these UTs without being warned about this.

We need to include them in yarn test. Not only linting.
Do you mind to fix this test command in your PR?

Otherwise, I'll make another PR.

Xapphire13 · 2017-11-15T17:54:22Z

I shall modify the test command =]. Once this is in, do I need to cherry pick it to master?

…y and often

Xapphire13 · 2017-11-20T01:09:46Z

Ping

chabou · 2017-11-20T08:30:13Z

We definitely need to run UTs on push.
It will not prevent commiting early and often. (commit !== push). And if you need to push with broken UTs, you still can do : yarn push --no-verify (but this is bad 😜 ), but CI tests will fail.

Can you make UTs run on prepush?
Thanks !

Xapphire13 · 2017-11-20T08:37:50Z

Certainly! I'll get around to it either tomorrow or later this week =]

Xapphire13 · 2017-11-20T08:43:59Z

Actually, turns out I can edit on my phone =]. Done!

chabou · 2017-11-20T08:56:51Z

I will test & merge ASAP

chabou · 2017-11-20T12:55:02Z

app/config/windows.js

+function validateAndFixWindowPosition(position, size) {
+  const displays = electron.screen.getAllDisplays();
+  const [x, y] = position;
+  const positionIsValid = displays.some(({workArea}) => {


We can't test with any of available displays, only with targeted screen.
Screen targeting and some verification are done here: https://github.com/zeit/hyper/blob/5d0c6355e12ec8d9a89182556fa73227374b488f/app/index.js#L113-L148

It is a little bit messy and we probably should refactor it to add your position/size validation.

We can't test with any of available displays, only with targeted screen.

I'm having trouble understanding what you mean by this?

It is a little bit messy and we probably should refactor it to add your position/size validation.

Sounds reasonable. Would you have the logic persist a fixed location? Or just assume that a correct location will be written on next close, and if not, the verification logic will take care of it again.

My first point: I misunderstood that a windows referential is always the same. I was thinking that we specified a screen when opening a new window. I was wrong.

My second point: I would assume that a correct location will be written or fixed if needed. But never mind, let's merge this as it is.

chabou · 2017-11-20T12:57:48Z

app/config/windows.js

+  const displays = electron.screen.getAllDisplays();
+  const [x, y] = position;
+  const positionIsValid = displays.some(({workArea}) => {
+    return x >= workArea.x && x <= workArea.x + workArea.width && y >= workArea.y && y <= workArea.y + workArea.height;


We should test if size is valid too. We don't want to have a valid position and an almost invisible window. For example if x === workArea.width - 1 or y === workArea.height - 1.

I had contemplated this, but decided to keep it simple and follow YAGNI. If we were to get into this case, the use would have had to positioned the window there themselves and if it's on the screen the standard ctrl + arrow shortcuts would work.

If we go with this method we would need to define how big a work area is permissible. We could perhaps change the logic to check the center of the window, if the center of a terminal window is in the workarea, then we're good, if not, relocate? (This would cover both the edge case you mention, and the original "my monitor is no longer plugged in" case)

I think that considering window center is really better than top-left corner.
I know there is some shortcuts (different on each OS), but I am pretty sure that some users know nothing about them.

I will fix this by myself, because this PR had a too long wait time. Sorry again for this

No need to consider new window center because startX/startY is already corrected by existing code.

chabou · 2017-11-20T12:58:27Z

app/config/windows.js

+    const {workArea} = electron.screen.getPrimaryDisplay();
+    position[0] = workArea.x + (workArea.width - size[0]) / 2;
+    position[1] = workArea.y + (workArea.height - size[1]) / 2;
+    cfg.set('windowPosition', position);


We don't need to calculate a valid position. I think that fallback to default position/size is ok.

The defaults are private to the windows config module. If I refactor my change to be in index.js how would I go about invalidating the set values?

Thank you to have fixed this

…late valid location, use default

Xapphire13 · 2018-02-09T02:05:35Z

@chabou

I finally got around to refactoring this and addressing your comments. Could you please take a look?

…ition

Xapphire13 · 2018-02-13T01:19:57Z

Ping @chabou

Sorry if I'm being pushy, just this PR is pretty old now =]

cnt000 · 2018-02-19T14:23:15Z

Only for me this bug is a block? I can't use hyper.
There are issues or we will have possibilities to accept this? Gently @chabou :-D

…ition # Conflicts: # yarn.lock

Xapphire13 · 2018-02-20T22:02:45Z

@albinekb, could we get a second reviewer on this please?

chabou · 2018-02-28T16:03:48Z

I am really sorry for the delay.
I will review/merge it before next week.

This reverts commit 35f5985.

chabou · 2018-03-02T21:11:46Z

Thanks a lot for your help 🙏

Xapphire13 · 2018-03-02T21:12:36Z

Thanks so much @chabou!

Xapphire13 added 2 commits November 14, 2017 17:36

Removing tests for removed files

7a5ba4d

Fixing #1869. Ensuring that the restored window position is valid

a6244a1

chabou mentioned this pull request Nov 15, 2017

window screen location issue with multiple screens #1608

Closed

2 tasks

Xapphire13 added 3 commits November 15, 2017 10:21

Adding UTs to test script

b307425

Convert tests to not need private helper function to be exposed

d0a9b50

not sure we should run UTs on push, this would prevent commiting earl…

43515cd

…y and often

We should run UTs on push after all

9c44400

chabou self-assigned this Nov 20, 2017

chabou self-requested a review November 20, 2017 08:57

chabou suggested changes Nov 20, 2017

View reviewed changes

osvaldolove mentioned this pull request Nov 28, 2017

Windows - Hyper starts off screen and can't move #2500

Closed

2 tasks

albinekb mentioned this pull request Feb 6, 2018

Hyper opens off screen, cannot move it back to the screen (Windows 10) #1682

Closed

2 tasks

Xapphire13 added 2 commits February 8, 2018 17:56

Move validateAndFixWindowPosition() out of window config. Don't calcu…

8bfcb70

…late valid location, use default

Merge remote-tracking branch 'upstream/canary' into windowPosition

e8a12d9

Xapphire13 added 2 commits February 8, 2018 23:32

Refactor to simplify window-utils

f5da3cc

Merge branch 'canary' of https://github.com/zeit/hyper into windowPos…

a43c773

…ition

Merge branch 'canary' of https://github.com/zeit/hyper into windowPos…

e1f27c0

…ition # Conflicts: # yarn.lock

Xapphire13 mentioned this pull request Feb 26, 2018

Fix AVA tests and made them mandatory. #2705

Merged

Validate new window center rather than its top-left corner

35f5985

chabou approved these changes Mar 2, 2018

View reviewed changes

Revert "Validate new window center rather than its top-left corner"

d8a7075

This reverts commit 35f5985.

chabou merged commit 24b8361 into vercel:canary Mar 2, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing #1869. Ensuring that the restored window position is valid #2459

Fixing #1869. Ensuring that the restored window position is valid #2459

Xapphire13 commented Nov 15, 2017 •

edited

Loading

chabou commented Nov 15, 2017

Xapphire13 commented Nov 15, 2017

Xapphire13 commented Nov 20, 2017

chabou commented Nov 20, 2017

Xapphire13 commented Nov 20, 2017

Xapphire13 commented Nov 20, 2017

chabou commented Nov 20, 2017

chabou Nov 20, 2017

Xapphire13 Nov 20, 2017

chabou Mar 2, 2018

chabou Nov 20, 2017

Xapphire13 Nov 20, 2017

chabou Mar 2, 2018

chabou Mar 2, 2018

chabou Nov 20, 2017

Xapphire13 Nov 20, 2017

chabou Mar 2, 2018

Xapphire13 commented Feb 9, 2018

Xapphire13 commented Feb 13, 2018 •

edited

Loading

cnt000 commented Feb 19, 2018 •

edited

Loading

Xapphire13 commented Feb 20, 2018

chabou commented Feb 28, 2018

chabou commented Mar 2, 2018

Xapphire13 commented Mar 2, 2018

Fixing #1869. Ensuring that the restored window position is valid #2459

Fixing #1869. Ensuring that the restored window position is valid #2459

Conversation

Xapphire13 commented Nov 15, 2017 • edited Loading

chabou commented Nov 15, 2017

Xapphire13 commented Nov 15, 2017

Xapphire13 commented Nov 20, 2017

chabou commented Nov 20, 2017

Xapphire13 commented Nov 20, 2017

Xapphire13 commented Nov 20, 2017

chabou commented Nov 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xapphire13 commented Feb 9, 2018

Xapphire13 commented Feb 13, 2018 • edited Loading

cnt000 commented Feb 19, 2018 • edited Loading

Xapphire13 commented Feb 20, 2018

chabou commented Feb 28, 2018

chabou commented Mar 2, 2018

Xapphire13 commented Mar 2, 2018

Xapphire13 commented Nov 15, 2017 •

edited

Loading

Xapphire13 commented Feb 13, 2018 •

edited

Loading

cnt000 commented Feb 19, 2018 •

edited

Loading