Skip to content
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

Automatically reset window position when disconnecting secondary screen #1280

Merged
merged 15 commits into from
Mar 10, 2020
Merged

Automatically reset window position when disconnecting secondary screen #1280

merged 15 commits into from
Mar 10, 2020

Conversation

tomioe
Copy link
Contributor

@tomioe tomioe commented Feb 18, 2020

Added a few features that I find immensely useful in my everyday use of Caprine (on Windows 10).

  1. When going from multi-screen to single-screen (i.e. when undocking laptop) the main window of Caprine will sometimes be stuck off-screen (cannot even using Win+Arrow Keys to move it). Having a 'Reset Position' in the tray menu helps this problem.

  2. I try to minimize my mouse movement as much as possible, but using Alt to toggle the menu and going into Exit simply isn't fast enough. I believe Command+Q is automatically bound in macOS to make an application quit, but not so on Windows, so I have added the Ctrl+Q keybinding to quit Caprine.

@tomioe
Copy link
Contributor Author

tomioe commented Feb 18, 2020

For some reason I am getting an require-atomic-updates rule is disabled but never reported when building on source\browser.ts:581:31 and source\browser\conversation-list.ts:90:32.

@sindresorhus
Copy link
Owner

When going from multi-screen to single-screen (i.e. when undocking laptop) the main window of Caprine will sometimes be stuck off-screen (cannot even using Win+Arrow Keys to move it). Having a 'Reset Position' in the tray menu helps this problem.

I guess this is the cause: electron/electron#10862

@sindresorhus
Copy link
Owner

For some reason I am getting an require-atomic-updates rule is disabled but never reported when building on source\browser.ts:581:31 and source\browser\conversation-list.ts:90:32

That means you can just remove those disable comments.

@sindresorhus
Copy link
Owner

Can you try to do this automatically? You could listen to this event https://www.electronjs.org/docs/api/screen#event-display-removed and then reset the position. I wonder if after resetting the position, we could set the saved position again and it would be correct. Worth a shot.

@tomioe
Copy link
Contributor Author

tomioe commented Feb 18, 2020

For some reason I am getting an require-atomic-updates rule is disabled but never reported when building on source\browser.ts:581:31 and source\browser\conversation-list.ts:90:32

That means you can just remove those disable comments.

Removing those disable comments makes my build pass locally, but fails when building in Travis. For some reason, I don't get the Possible race condition... error when building locally.

@tomioe
Copy link
Contributor Author

tomioe commented Feb 18, 2020

Can you try to do this automatically? You could listen to this event https://www.electronjs.org/docs/api/screen#event-display-removed and then reset the position. I wonder if after resetting the position, we could set the saved position again and it would be correct. Worth a shot.

Will give this a shot, would definitely be smoother to have it happen automatically.

@tomioe
Copy link
Contributor Author

tomioe commented Feb 26, 2020

Can you try to do this automatically? You could listen to this event https://www.electronjs.org/docs/api/screen#event-display-removed and then reset the position. I wonder if after resetting the position, we could set the saved position again and it would be correct. Worth a shot.

I've tried an implementing this in 9f6e1ad. It can be tested in Windows (if using two screens), by moving the Caprine main window to the secondary screen and setting the "Project" (Win+P) setting to "PC Screen Only". The Caprine window position should move to the same position on the primary screen as it had on the secondary screen.

@sindresorhus
Copy link
Owner

I believe Command+Q is automatically bound in macOS to make an application quit, but not so on Windows, so I have added the Ctrl+Q keybinding to quit Caprine.

Is that really a Windows convention? Do other Windows app support that. Because the menu item is named "Exit" on windows, so "Q" doesn't really make sense.

source/index.ts Outdated Show resolved Hide resolved
source/index.ts Outdated Show resolved Hide resolved
source/tray.ts Outdated Show resolved Hide resolved
source/browser.ts Outdated Show resolved Hide resolved
@tomioe tomioe requested a review from sindresorhus March 8, 2020 19:59
@tomioe
Copy link
Contributor Author

tomioe commented Mar 8, 2020

I believe Command+Q is automatically bound in macOS to make an application quit, but not so on Windows, so I have added the Ctrl+Q keybinding to quit Caprine.

Is that really a Windows convention? Do other Windows app support that. Because the menu item is named "Exit" on windows, so "Q" doesn't really make sense.

Huh, I must've picked up that bad habit over the years... I just assumed it was a conventional shortcut, due to how closely it is to Cmd+Q, but I was wrong: https://support.microsoft.com/en-us/help/12445/windows-keyboard-shortcuts

Although uncommon, it is still implemented in some applications (i.e. Firefox uses Ctrl+Shift+Q), while others seem to favor the more idiomatic Alt+F, X:https://www.ghacks.net/2018/10/22/google-retires-ctrl-shift-q-in-chrome-to-exit-web-browser/

Currently Alt+F, X does not work in Carpine, since underlined letters for keyboard navigation have not been implemented. Perhaps this is a better suited goal of this PR?

And also, cheers for the review! 👍

@sindresorhus
Copy link
Owner

Currently Alt+F, X does not work in Carpine, since underlined letters for keyboard navigation have not been implemented. Perhaps this is a better suited goal of this PR?

Yes, but that's really out of the scope of this PR and should be a separate PR.

source/menu.ts Outdated Show resolved Hide resolved
source/index.ts Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

The PR title needs to be updated.

@tomioe tomioe changed the title Add 'Reset Position' in tray menu and shortcut for quitting Automatically reset window position when disconnecting secondary screen Mar 9, 2020
@tomioe tomioe requested a review from sindresorhus March 9, 2020 17:44
source/menu.ts Outdated Show resolved Hide resolved
@sindresorhus sindresorhus merged commit dce6cfd into sindresorhus:master Mar 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants