8969-upgrade-monaco-editor-core-to-standalone-0.23.x

[danarad05]

In preparation for upgrading monaco-editor-core to standalone/0.23.x these changes were implemented:

1. signatures alignment to standalone/0.23.x
2. editor preferences synced with standalone/0.23.x

CQ: https://dev.eclipse.org/ipzilla/show_bug.cgi?id=23341

Signed-off-by: Dan Arad dan.arad@sap.com

What it does

This PR updates Theia repo with these changes:

1. signatures alignment to standalone/0.23.x
2. editor preferences synced with standalone/0.23.x
3. Changes to theme services based on changes in 0.23.x
4. Removal of QuickOpen modules and implementing QuickInput instead.
5. Running quickInput functionality in Extensions - this I am still currently being worked on by me

(It is also second part of 8969 where first part is generating monaco-editor-core@0.23)

How to test

1. How to test section for the previous PR Upgrade to Monaco 0.20.0 #8010.
2. Integration tests in: add integration tests for typescript language #7265.
3. All quickOpen replaced by quickInput functionality like:

• All Quick Open prefixes (Ctrl+P and then '?') - also from shortcut keys:
  
• All GIT commands - Go to source control view (Ctrl+Shift+G) and test out all git commands including setting up new repository cloning , fetch, pull, push, merge, checkout, chooseTagsAndBranches , commitMessageForAmend, stash actions, also GIT ASKPASS in electron (which I’m not sure yet how to test) and also git sync functionality (theia\packages\git\src\browser\git-sync-service.ts). Also Change repository (ScmQuickOpenService):.
  -- there is also electron-browser/prompt/git-quick-open-prompt.ts which needs testing - and I'm not sure how to test it.
• mini browser - open url
• all tasks functionality

4. All functionality of themes should be tested (Select File Icon Theme, Select Color Theme)
5. Theia Commands:

• Choosing keyboard layout: “core.keyboard.choose”

• “Editor: Change Language Mode”

• “Editor: Change File Encoding”

• Config indentation: 'textEditor.commands.configIndentation'

• 'Indent Using Spaces'

• 'Indent Using Tabs'

• Selecting formatter for document (when there are multiple ones) – i.e.:
  

• 'Deploy Plugin by Id'

• Using “vscode-extension-samples/configuration-sample” sample, run 'config.commands.configureEmptyLastLineFiles' command and select workspace folder:
  

• Register Variables – for VariableContribution.

• ‘Open Recent Workspace’

6. Quick Input extensions usage – using “vscode-extension-samples/quickinput-sample” sample.

Review checklist

• as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

• as a reviewer, I agree to behave in accordance with the review guidelines

[danarad05]

@marechal-p
Could you review? - updated relevant references to standalone/0.23.x
Thanks

[danarad05]

@vince-fugnitto @RomanNikitenko @marechal-p
Concerning existing monaco.quickOpen quick-open module - should I delete all irrelevant code or should I mark it with @deprecated?

[paul-marechal]

@danarad05 to me deprecated means "you should use something else but it should still work". In our case, if a type really doesn't map to any implementation then we should remove it.

[danarad05]

@danarad05 to me deprecated means "you should use something else but it should still work". In our case, if a type really doesn't map to any implementation then we should remove it.

Thank @marechal-p. Wanted to make sure.

[azatsarynnyy]

Hi @danarad05! I see there's good progress on the Monaco upgrade. Thank you for working on that! 👍
I wonder if there's something here that could be done in parallel. From the Red Hat side, we could have a couple of people to help you with this upgrade. At least, for the next month. Or maybe some help will be needed when testing/reviewing the PR.
Please, let me know if more people involved in this work would help. Thanks!

[azatsarynnyy]

I've just noticed the comment #8969 (comment). So, I've got the answer 🙂

[danarad05]

Hi @danarad05! I see there's good progress on the Monaco upgrade. Thank you for working on that! 👍
I wonder if there's something here that could be done in parallel. From the Red Hat side, we could have a couple of people to help you with this upgrade. At least, for the next month. Or maybe some help will be needed when testing/reviewing the PR.
Please, let me know if more people involved in this work would help. Thanks!

Thanks for the offer @azatsarynnyy - it's a shame I didn't know about it sooner as it would have progressed PR faster.
I'm finishing today - tomorrow and will be updating all modules/tests required and of course I would want a design wise review from as many reviewers as possible. I'm sure solution could be improved more.
Thanks.

[danarad05]

@vince-fugnitto @azatsarynnyy @marechal-p @RomanNikitenko @marcdumais-work @offer8
PR is ready for starting review. I've mentioned above also sections that need to be tested (other then of course design review):

I've updated above the tests required.

Thanks

[tsmaeder]

@danarad05 I don't really understand why you have added the "QuickInput" interfaces to the monaco typings: as far as I understand, the quick-open API has been removed from the monaco editor without replacement: the quick-input API is seems to be used only outside the editor subtree in VS Code. Since this decision drives a lot of the changes in this PR, it would be helpful for my code review if you could explain.

[tsmaeder]

@danarad05 as I understand it, we are not consuming all of VS Code, but only the monaco editor (located in the src/vs/editor subtree of the VS Code source). The interfaces to interact with monaco (either to consume functionality or to provide services to plug into monaco) are described in the typings/monaco/index.d.ts. But IMO monaco never invokes the quick-input feature nor does monaco provide any services that are described by the quick-input interfaces. So these interfaces don't have to be in the monaco typings, and therefore should not.

Also, if the quick-input is not part of the monaco interface, it does not have to be aligned with the VS Code version of those services. It might be desirable, because we sometimes reuse VS Code source, but it's not a necessity.
Having the interfaces described as part of the monaco interfaces forces you to move the quick-input-service from the common to the browser folder: you end up importing from the browser folder in core/src/node/backend-application-module.ts and we should not do that: browser and node can import from common, but not vice-versa and browser and node cannot import from each other.

I would suggest removing the quick-input stuff from the monaco typings to an appropriate place according to the theia module structure. Common services need to be in common and we could proceed from that starting point.

Please let me know if my understanding is wrong.

[tsmaeder]

@esterw1109 I will, but you'll need to stop pushing fixes ;-)

[EstherPerelman]

@esterw1109 I will, but you'll need to stop pushing fixes ;-)

There where conflicts... no more fixes (hopefully)
@tsmaeder Should I rebase to one commit?

[tsmaeder]

We have "Squash and merge" now, so I don't think there's a need.

[tsmaeder]

I see the failures on Ubuntu on other PR's as well. The test failure is because of a newly introduced test. I'm going to merge now.
@esterw1109 @vince-fugnitto could you please cooperate on a followup PR? I'll file an issue.

[tsmaeder]

@esterw1109 could you please take care of the followup: #9692

[RomanNikitenko]

btw: I get the following errors when I run tests from the root (not from /theia/examples/browser), it looks like it's another problem

[EstherPerelman]

@tsmaeder Thank you for merging! @RomanNikitenko @vince-fugnitto I'll take care of the failing tests (I'm only responding now because Friday & Saturday are our rest days...)

[EstherPerelman]

btw: I get the following errors when I run tests from the root (not from /theia/examples/browser), it looks like it's another problem

Are you sure this error is because of this PR? I ran yarn test from root in Theia v1.14.0 and it fails on another error before testing editor-preview which fail on this:

[RomanNikitenko]

@EstherPerelman
I tested on the master when the PR was not merged yet and didn't see such errors.
You could try git reset to the commit before these changes to check it.

[EstherPerelman]

@EstherPerelman
I tested on the master when the PR was not merged yet and didn't see such errors.
You could try git reset to the commit before these changes to check it.

@RomanNikitenko I have used the: git checkout 9648953cd6e8552b7e027558509ba77770bd8b6f which is the last commit before this merged (you can ensure by git log), run yarn and then yarn test -> the same error appears...

[vince-fugnitto]

@danarad05 @esterw1109 the editor-preview-manager and editor-preview-factory tests no longer exist as far as I understand (changes).

• locally the command yarn test:theia passes (run unit-tests)
• CI is green for unit-tests

Your environment is likely dirty:

• git clean -ffdx
• yarn
• yarn test:theia

The tests which are failing are: #9701.

[tsmaeder]

@RomanNikitenko unless you do a git clean, there will be stale stuff in some of the lib folders. yarn clean is not reliable, in my experience 🤷

[RomanNikitenko]

I didn't check it again, but I believe @vince-fugnitto is right, the problem could be on my side due to:

Your environment is likely dirty

@EstherPerelman, sorry that I mislead you.

So, if I understand correctly, there is still an issue related to tests:

The tests which are failing are: #9701.

[tsmaeder]

@esterw1109 one thing we completely forgot in this to add an entry to the Changelog.md file describing at least the breaking API changes related to the quick-input/quick-open changes.

[colin-grant-work]

@tsmaeder, it looks like this can probably be removed now?

theia/packages/core/src/browser/frontend-application.ts

Lines 165 to 182 in 6bdf64d

	/* vvv HOTFIX begin vvv
	*
	* This is a hotfix against issues eclipse/theia#6459 and gitpod-io/gitpod#875 .
	* It should be reverted after Theia was updated to the newer Monaco.
	*/
	protected inComposition = false;
	/**
	* Register composition related event listeners.
	*/
	protected registerCompositionEventListeners(): void {
	window.document.addEventListener('compositionstart', event => {
	this.inComposition = true;
	});
	window.document.addEventListener('compositionend', event => {
	this.inComposition = false;
	});
	}
	/* ^^^ HOTFIX end ^^^ */

[tsmaeder]

@colin-grant-work probably, good catch. Could you open an issue, please?