Skip to content
This repository has been archived by the owner on Mar 22, 2024. It is now read-only.

semantic highlighting works with classic editor #45

Merged
merged 4 commits into from
Sep 19, 2023
Merged

semantic highlighting works with classic editor #45

merged 4 commits into from
Sep 19, 2023

Conversation

kaisalmen
Copy link
Collaborator

@kaisalmen kaisalmen commented Sep 8, 2023

The wrapper always uses the configuration service from monaco-vscode-api what basically makes updateUserConfiguration mandatory for configuration updates after start.

In classic mode editorOptions that are editor specific work, but things like semanticHighlighting requires the user configuration update due to the activation of the vscode configuration service. This PR takes the semanticHighlighting from the editorOptions and applies them the the userConfiguration. This prevents to run into the same problem as #32 .

This PR moves userConfiguration to EditorAppBaseConfig which makes it available in both classic mode and vscodeApi mode. It is generally advised to use this mean to configure the app, but we leave the monaco-editor options in for BW-compatibility reasons.

Overall aim to allow monarch + semanticHighlighting is achieved.

@kaisalmen kaisalmen requested review from montymxb and removed request for montymxb September 8, 2023 09:20
@kaisalmen kaisalmen mentioned this pull request Sep 14, 2023
Copy link
Member

@montymxb montymxb left a comment

Choose a reason for hiding this comment

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

Some general points for improvement, but the functionality looks good overall. Nothing seems up regarding tests, and running the classical demo locally shows semantic highlighting as expected!

@kaisalmen
Copy link
Collaborator Author

Thank you. This is good to go from my side now.

Copy link
Member

@montymxb montymxb left a comment

Choose a reason for hiding this comment

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

Alright, new changes look good to go :octocat:

@kaisalmen kaisalmen merged commit 9115ccb into main Sep 19, 2023
3 checks passed
@kaisalmen kaisalmen deleted the issue-32 branch September 19, 2023 09:09
@cdietrich
Copy link
Contributor

@kaisalmen is this already part of 3.1.0-next.0?

@kaisalmen
Copy link
Collaborator Author

@cdietrich yes it is. This PR is in and 47 (status from yesterday) and 48 are in.

@cdietrich
Copy link
Contributor

cdietrich commented Sep 20, 2023

hmm somehow i still get the same error in my example. need to digg.

problem found: is working now

@kaisalmen
Copy link
Collaborator Author

@cdietrich Superb! Btw, the langium example now allows you to launch the editor in both modes!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants