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

Remove global shortcuts from Electron, use WindowService for reload #10704

Merged
merged 2 commits into from
Feb 7, 2022

Conversation

colin-grant-work
Copy link
Contributor

What it does

Fixes #10701 by removing the registration of globalShortcuts from the ElectronMainApplication. There is already a command available internally to reload the window, which can be rebound or unbound and is context-sensitive, so the global shortcut is not necessary.

How to test

  1. Run the Electron application
  2. Open a bash terminal
  3. Use the ctrl+r shortcut to access 'reverse-i-search'
  4. Put focus on some element other than the terminal.
  5. Use the ctrl+r shortcut to refresh the window - it should refresh
  6. Dirty an editor and use the ctrl+r shortcut - you should be prompted to confirm the refresh
  7. If you'd like, rebind the Refresh Window command to some other keybinding and confirm that it behaves correctly.

Review checklist

Reminder for reviewers

@colin-grant-work colin-grant-work added terminal issues related to the terminal electron issues related to the electron target labels Feb 2, 2022
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

I confirmed that the changes fix #10701, and when a file is dirty the reload will prevent the app from restarting unless prompted.

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

@colin-grant-work colin-grant-work merged commit 4dafce5 into master Feb 7, 2022
@github-actions github-actions bot added this to the 1.23.0 milestone Feb 7, 2022
@orange-pig
Copy link

Sorry, I find the remove funciton 'attachGlobalShortcuts', like being refactored to theia-electron-window.ts in #10600 before time.

They are all removal operations, and there is no conflict when merging.

The problem still exists.

@kittaakos
Copy link
Contributor

Did you deprecate disallowReloadKeybinding? It's not used anymore, just in tests. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
electron issues related to the electron target terminal issues related to the terminal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ctrl+R in bash terminal does not activate 'reverse-i-search'
4 participants