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

Rebind Go to Definition to ctrl+f11 #6411

Merged
merged 1 commit into from
Oct 18, 2019
Merged

Rebind Go to Definition to ctrl+f11 #6411

merged 1 commit into from
Oct 18, 2019

Conversation

svor
Copy link
Contributor

@svor svor commented Oct 17, 2019

Signed-off-by: svor [email protected]

What it does

Sets ctrl+f11 as a default shortcut for Go to Definition command to avoid collision with Go to Implementation command.

Related issues:

How to test

  • Start Theia in the browser
  • Open some java class and make sure that Java LS started
  • Try to execute Go to Definition via ctrl+f11 and Go to Implementation via ctrl+f12

Review checklist

Reminder for reviewers

@svor svor requested a review from akosyakov October 17, 2019 11:12
@svor svor self-assigned this Oct 17, 2019
@akosyakov
Copy link
Member

@eclipse-theia/eclipse-theia Does anyone has objections, concerns about such keybinding?

@akosyakov akosyakov added the monaco issues related to monaco label Oct 17, 2019
@thegecko
Copy link
Member

I don't think this raises any concerns.

@lmcbout
Copy link
Contributor

lmcbout commented Oct 17, 2019

Tested on Ubuntu
Works fine but I noticed that the keybinding "F12" in electron is for "Go to Definition" and in the browser, the keybinding is "ctrlcmd + f11".
Can we use the same keybindings with electron and the browser ?
In electron, "F12" is for "monaco.editor.gotoNextSymbolFromResult" but "ctrl+ f11" is not used

@svor
Copy link
Contributor Author

svor commented Oct 17, 2019

@lmcbout Monaco uses F12 as a keybinding for Go to Definition if it is running not in the browser - it is default value, for the browser it is Ctrl+F12.

Can we use the same keybindings with electron and the browser ?

Yes, we can set Ctrl + F11 for the electron as well, but I'm not sure if it would be correct decision

@lmcbout
Copy link
Contributor

lmcbout commented Oct 17, 2019

@svor Thanks for the info.
I don't know if some end-users use the browser and the electron version, i don't , but if they do, it would be nice for them eventually to use the same keybindings.
So for this PR, works fine for me , no objections

Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

thank you @svor!

@akosyakov
Copy link
Member

akosyakov commented Oct 17, 2019

Yes, we can set Ctrl + F11 for the electron as well, but I'm not sure if it would be correct decision

I think it is good to keep it align with VS Code. When VS Code team sorted out the issue in Monaco we can revert this fix.

Copy link
Contributor

@benoitf benoitf left a comment

Choose a reason for hiding this comment

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

tested on macos

@svor svor merged commit 162a48a into eclipse-theia:master Oct 18, 2019
@svor svor deleted the sv/shortcut branch October 18, 2019 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
monaco issues related to monaco
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants