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

menu: update go main-menu #10299

Merged
merged 3 commits into from
Oct 27, 2021
Merged

menu: update go main-menu #10299

merged 3 commits into from
Oct 27, 2021

Conversation

vince-fugnitto
Copy link
Member

What it does

The commit updates the go main-menu to add missing items used for navigation, location, language-features and problems.

image

How to test

  1. start the browser or electron example application
  2. confirm the go menu is correct
  3. confirm that the menu items based on the available commands work well

Review checklist

Reminder for reviewers

Signed-off-by: vince-fugnitto [email protected]

@vince-fugnitto vince-fugnitto added the menus issues related to the menu label Oct 19, 2021
@vince-fugnitto vince-fugnitto self-assigned this Oct 19, 2021
@msujew msujew self-requested a review October 20, 2021 14:55
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

LGTM, I can confirm that the added menu entries work as expected and are grouped/ordered as they are in vscode 👍

Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Due to the merge of #10106, I'd recommend to add the appropriate nls.localize calls with the respective vscode IDs.

packages/editor/src/browser/editor-command.ts Outdated Show resolved Hide resolved
packages/editor/src/browser/editor-menu.ts Outdated Show resolved Hide resolved
packages/editor/src/browser/editor-menu.ts Outdated Show resolved Hide resolved
packages/editor/src/browser/editor-menu.ts Outdated Show resolved Hide resolved
packages/monaco/src/browser/monaco-menu.ts Outdated Show resolved Hide resolved
packages/monaco/src/browser/monaco-menu.ts Outdated Show resolved Hide resolved
packages/monaco/src/browser/monaco-menu.ts Outdated Show resolved Hide resolved
@vince-fugnitto
Copy link
Member Author

Due to the merge of #10106, I'd recommend to add the appropriate nls.localize calls with the respective vscode IDs.

@msujew ehh...I did not expect the codebase to have so much noise as we now reference vscode everywhere:

Screen Shot 2021-10-21 at 5 30 03 PM

I have a few follow-up questions:

  • how are we expected to find these keys (no documentation for developers or in coding guidelines)
  • I don't see anything that helps developers remember to localize - syntax help, warnings
  • What should we localize (falls under the documenation)
  • What do we do in case the commands, menus, preferences have no vscode counterpart?

I think without all these in place it makes handling localization going forward difficult.

@msujew
Copy link
Member

msujew commented Oct 21, 2021

ehh...I did not expect the codebase to have so much noise as we now reference vscode everywhere:

@vince-fugnitto I feel like localization in general is a lot of noise, I noticed that as well when looking at the codebase of vscode.

Regarding your questions: I already have some stuff prepared for the coding guidelines (in regards to how and what to localize), which I will add tomorrow and improve over the coming days. I also have something planned for developers to have an easier time finding keys that already exist inside of the vscode language packs.

I don't see anything that helps developers remember to localize - syntax help, warnings

This is probably the one that's the hardest to deal with, as programmatically identifying which strings are shown to the user and which ones aren't is quite impossible. I'll keep that issue in mind and see what I can do.

What do we do in case the commands, menus, preferences have no vscode counterpart?

That will also be included in the coding guidelines. In short: Write a nls.localize('theia/<package>/<id>', '<value>') call. The new id and the corresponding default value will be picked up by the appropriate theia-cli command and translated into different languages later on (hopefully completely automatically).

@vince-fugnitto
Copy link
Member Author

@msujew

@vince-fugnitto I feel like localization in general is a lot of noise, I noticed that as well when looking at the codebase of vscode.

I don't mean so much the noise of all the .localize calls, but rather than the entire codebase now refers to vscode and now needs to be coupled with their keys which we now need to find which I'm not sure is trivial or not. Given that language-packs are in fact plugins I did not expect all the functionality to bleed to the rest of the framework.

We are a framework, and we are Eclipse Theia not VS Code so I did not expect to have references of internals of vscode everywhere.

That will also be included in the coding guidelines. In short: Write a nls.localize('theia/<package>/<id>', '<value>') call. The new id and the corresponding default value will be picked up by the appropriate theia-cli command and translated into different languages later on (hopefully completely automatically).

There are many commands I assume already in the framework which have no vscode counterpart, are these handled today or are simply not localized yet?

The commit updates the `go` main-menu to add missing items used for
navigation, location, language-features and problems.

Signed-off-by: vince-fugnitto <[email protected]>
@msujew
Copy link
Member

msujew commented Oct 22, 2021

We are a framework, and we are Eclipse Theia not VS Code so I did not expect to have references of internals of vscode everywhere.

I see, that's understandable. I'll think about how we could reduce our tight coupling with vscode.

There are many commands I assume already in the framework which have no vscode counterpart, are these handled today or are simply not localized yet?

They're currently not localized. Around 15% of user-facing strings in Theia (there only a bit more than a thousand of them in total) are not translated, mainly in the custom views that do not exist inside of vscode (like the properties view, scm amend, scm history, etc.). For the actual translation, if I recall correcly, @brianking proposed during a dev-meeting to have manual reviewers/contributors for each supported language which utilize the machine-translations (generated by the appropriate CLI commands). I'll start to work on this next.

Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

I am currently working on tooling to improve finding translation keys... :/

The commit fixes a typo in the key which translates the label.

Signed-off-by: vince-fugnitto <[email protected]>
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

LGTM, merge at your own discretion 👍

Copy link
Contributor

@colin-grant-work colin-grant-work left a comment

Choose a reason for hiding this comment

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

I ran every command. It seems 'Go to declaration' didn't work in either Theia or VScode for TypeScript, but otherwise all executed successfully. I also installed the Chinese (Traditional) language pack and observed that all of the commands in the menu were translated.

packages/monaco/src/browser/monaco-menu.ts Outdated Show resolved Hide resolved
@colin-grant-work
Copy link
Contributor

@msujew, while I was testing this, I installed a language pack and then switched to that language, and everything went well. Then I experienced a bug that shut the application down, and when I restarted it, everything was in English again. I tried running the 'Configure Display Language' command to switch back, but when I selected Chinese, nothing happened. Selecting English again caused a refresh, then everything was in English again, and then I could select Chinese and produce a refresh. However, shutting things down and restarting got me back to English again, with selecting Chinese doing nothing. Is this a known issue, or should I create one?

@msujew
Copy link
Member

msujew commented Oct 26, 2021

@colin-grant-work Interesting, please go ahead and create an issue for that. I should have time tomorrow to work on fixing this.

Signed-off-by: vince-fugnitto <[email protected]>
@vince-fugnitto vince-fugnitto merged commit 4aaa6b8 into master Oct 27, 2021
@vince-fugnitto vince-fugnitto deleted the vf/update-go-menu branch October 27, 2021 17:18
@github-actions github-actions bot added this to the 1.19.0 milestone Oct 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
menus issues related to the menu
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants