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

Upgrade to LSP 6.0.0 and Monaco 0.19.3 #7149

Merged
merged 11 commits into from
Mar 27, 2020
Merged

Upgrade to LSP 6.0.0 and Monaco 0.19.3 #7149

merged 11 commits into from
Mar 27, 2020

Conversation

RomanNikitenko
Copy link
Contributor

@RomanNikitenko RomanNikitenko commented Feb 13, 2020

What it does

Closes #6900

Upgrade to:

TODO

How to test

  • test language features with VS Code extension:
    • test that features are available in the editor context menu and in quick command palette (automated)
    • a declaration is revealed when following the reference (automated)
    • focus belongs to a proper editor after following a reference (for internal and outside references, from editor to editor preview and view versa) (automated)
    • reference peek is still opened after following a reference (for internal and outside references, from editor to editor preview and view versa) (automated)
    • esc closes reference peek widget (automated)
    • completion items are properly resolved and inserted (automated)
    • focus belongs to a proper editor on peek definition (for internal and outside references, from editor to editor preview and view versa) (automated)
    • check that rename work inside the file and across files (automated)
    • check that arguments are proposed by the signature help (automated)
    • check that hover is present with properly rendered docs (automated)
    • check that symbol occurrences is highlighted when a declaration or reference is selected (automated)
    • check that go to (type|implementation) definition works properly, i.e. definition is revealed (automated)
    • check that code lens work properly (automated)
    • check that code actions work properly (quick fixes) (automated)
    • check that document and selection formatting work properly (automated)
    • check color provider with css colors
    • check link provider with links in html
    • check that folding ranges are properly provided for js, css and html
    • check go to symbol command
    • check open workspace symbol command
  • test quick palette:
    • proper keys are displayed for the quick command palette, i.e. they are displayed and a corresponding Monaco command can be triggered by such keybinding
    • use sample vscode extension
    • use fuzzy search
  • test vscode tree view (that context keys work properly)
  • test plugin editor decorations, for example with power mode extension
  • test debug console input editor
  • test debug breakpoint editor
  • test that preferences are propagated as config key values: change editor preferences and check whether they are reflected
  • test language auto detection
  • test edit menu with editor
  • test selection menu with editor
  • test editor context menu
  • smoke test some VS Code extensions like go, python, just use editor for a bit, check backend and frontend logs for errors
  • test manually task related functionality Upgrade to LSP 6.0.0 and Monaco 0.19.3 #7149 (comment)

Review checklist

Reminder for reviewers

Signed-off-by: Roman Nikitenko rnikiten@redhat.com

@akosyakov akosyakov added lsp language server protocol monaco issues related to monaco vscode issues related to VSCode compatibility labels Feb 14, 2020
@akosyakov
Copy link
Member

@RomanNikitenko Could you reference relevant issue and fill int How to test with boxes from previous migration please?

@RomanNikitenko RomanNikitenko force-pushed the upgrade_monaco branch 2 times, most recently from 1a0e2d5 to 53b236d Compare March 1, 2020 20:24
@RomanNikitenko
Copy link
Contributor Author

RomanNikitenko commented Mar 1, 2020

@akosyakov
I updated the PR, but looks like ci build failed because of tests. I'll try to fix it today.

@RomanNikitenko RomanNikitenko force-pushed the upgrade_monaco branch 3 times, most recently from 22ed538 to e536c70 Compare March 2, 2020 13:52
@RomanNikitenko
Copy link
Contributor Author

I found that Peek actions are coming as items of Peek submenu in vs code, so I'm going to align it for theia.

@akosyakov
Copy link
Member

@RomanNikitenko ok, I will start testing it a bit this week.

@akosyakov
Copy link
Member

btw thank you for such diligent work on updating monaco typings 👍

@akosyakov
Copy link
Member

@RomanNikitenko I've started adding tests in #7265 against this PR. I will stabilise and add more tests next days. They should cover How to test section of this PR eventually.

@RomanNikitenko
Copy link
Contributor Author

@RomanNikitenko I've started adding tests in #7265 against this PR. I will stabilise and add more tests next days. They should cover How to test section of this PR eventually.

I think it will really help to test the current changes and will speed up the process of testing of next Monaco upgrade!

Thank you, Anton for doing this. l really appreciate it! 👍

@akosyakov
Copy link
Member

akosyakov commented Mar 4, 2020

I've noticed that go to definition does not jump to proper column anymore:

  • put a cursor in theia/examples/browser/src-gen/backend/server.js at Container word
  • use go to definition
  • on master a new editor is revealed and cursor is put at Container word, with this PR it ends up at the beginning of line

@akosyakov
Copy link
Member

akosyakov commented Mar 4, 2020

Regarding to #7149 (comment)

@RomanNikitenko there is a bug in the plugin system which breaks navigation coupled with this PR, here

export interface DefinitionLink {
uri: UriComponents;
range: Range;
origin?: Range;
selectionRange?: Range;
}
there are selectionRange and origin, but there are no such properties monaco.d.ts, instead it should be targetSelectionRange and originSelectionRange

Added an integration test for it: https://github.com/eclipse-theia/theia/pull/7265/files#diff-654c40ef160f7094cc2d64d1183831e6R95-R105

btw it used to work because Monaco was guessing before, but now they removed it and rely on properties above

@RomanNikitenko
Copy link
Contributor Author

RomanNikitenko commented Mar 4, 2020

@akosyakov

there are selectionRange and origin, but there are no such properties monaco.d.ts, instead it should be targetSelectionRange and originSelectionRange

the corresponding interface was renamed in vs code: DefinitionLink => LocationLink

Should we rename/deprecate it as well in plugin-api-rpc-model and keep it close to vs code?
or
Do you prefer to keep an old name for such cases?

@akosyakov
Copy link
Member

@RomanNikitenko I would be fine with it, it is not public API, so should be fine to align. It is certain helps when things named the same way during investigations.

@akosyakov
Copy link
Member

@RomanNikitenko thank you for the update, i will continue adding test tomorrow, had to focus on the vsx registry integration today. Could you check other my comments in the meantime?

@RomanNikitenko
Copy link
Contributor Author

RomanNikitenko commented Mar 5, 2020

@akosyakov

  • added Peek submenu
  • added fix for go to definition.
    Checked your use case manually - looks like it's fixed now. Applied your test - it failed. Will take a look at this today more closely...

Could you check other my comments in the meantime?

sure! thank you for your comments - yesterday I added TODO section to the PR description to not lose your comments and fix them.

I'm not sure I understand the use case for testing here: #7149 (comment). I played with it yesterday - tried to add and remove the corresponding code, but I didn' see the difference. Will do it today again...

@akosyakov
Copy link
Member

akosyakov commented Mar 5, 2020

I'm not sure I understand the use case for testing here: #7149 (comment). I played with it yesterday - tried to add and remove the corresponding code, but I didn' see the difference. Will do it today again...

@RomanNikitenko You can try to remove it on master and see what is the difference. I think it was related to peek reference dialog if you follow a reference from it then a new editor should be opened and it should have the peek reference opened as well, but old widget should not. Plus focus should be transferred properly, i.e. try to move up/down or type after navigation. On master it should break when you remove with new Monaco i hope it should be fine, at least it looks like it was fixed in the upstream.

Applied your test - it failed. Will take a look at this today more closely...

Maybe something with the test itself, will have look at it tomorrow as well.

@RomanNikitenko
Copy link
Contributor Author

@akosyakov

You can try to remove it on master and see what is the difference.

Got it! Yes, now I can reproduce it, thank you!
I added comment #7149 (comment)

@RomanNikitenko
Copy link
Contributor Author

RomanNikitenko commented Mar 26, 2020

@akosyakov

release is done, you can rebase, clean up the history and merge

I have two questions :-)

@akosyakov
Copy link
Member

akosyakov commented Mar 27, 2020

should I replace the function toCssClassName by method?

it would be nice, but i don't want to delay you from merging

I tried to reorganize git history according to resolved problems.

it looks great

@azatsarynnyy
Copy link
Member

@azatsarynnyy @RomanNikitenko Can we chat somehow about what you think about finding ways to do #7100 and #7261? Instead of upgrading as we do now?

@akosyakov sure, let's plan it on Monday

Signed-off-by: Roman Nikitenko <rnikiten@redhat.com>
Signed-off-by: Roman Nikitenko <rnikiten@redhat.com>
Signed-off-by: Roman Nikitenko <rnikiten@redhat.com>
Signed-off-by: Roman Nikitenko <rnikiten@redhat.com>
Signed-off-by: Roman Nikitenko <rnikiten@redhat.com>
Signed-off-by: Roman Nikitenko <rnikiten@redhat.com>
Signed-off-by: Roman Nikitenko <rnikiten@redhat.com>
Signed-off-by: Roman Nikitenko <rnikiten@redhat.com>
Signed-off-by: Roman Nikitenko <rnikiten@redhat.com>
Signed-off-by: Roman Nikitenko <rnikiten@redhat.com>
Signed-off-by: Roman Nikitenko <rnikiten@redhat.com>
@akosyakov
Copy link
Member

@azatsarynnyy Should we meet in our dev meeting room? What time will work for you? at 15 CET?

@akosyakov
Copy link
Member

@RomanNikitenko should anything else be done to merge it?

@azatsarynnyy
Copy link
Member

@azatsarynnyy Should we meet in our dev meeting room? What time will work for you? at 15 CET?

@akosyakov yes, we can use the same BJN link as for dev call. 15 CET is good for us. Thanks!

@RomanNikitenko RomanNikitenko merged commit 57fcabf into master Mar 27, 2020
@RomanNikitenko
Copy link
Contributor Author

@akosyakov
sorry for the delay - some things on our side...

Thank you very much for your support!
I think this kind of work would be impossible without your assistance.
I really appreciate your help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lsp language server protocol monaco issues related to monaco vscode issues related to VSCode compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade to the latest Monaco
7 participants