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

plugin-ext: late textmate grammar activation #7544

Merged
merged 1 commit into from
Apr 14, 2020

Conversation

paul-marechal
Copy link
Member

There is a race condition when the layout is restored: if an editor was
opened, it will be restored and wait for some language activation event.
The issue with this was first seen with the builtin cpp extension: two
cpp grammars are contributed, but the first one is not the right one to
use on a cpp file. When an editor opened, the first bogus grammar is
loaded and activated, triggering bogus coloration.

This commit ensures that all grammars are more correctly contributed
before activating anything in the next event-loop tick.

Signed-off-by: Paul Maréchal [email protected]

Fixes eclipse-theia/theia-cpp-extensions#100

How to test

See eclipse-theia/theia-cpp-extensions#100, the coloration should not be bogus anymore.

Review checklist

Reminder for reviewers

@paul-marechal paul-marechal added the textmate issues related to the textmate grammars label Apr 9, 2020
@paul-marechal paul-marechal force-pushed the mp/cpp-textmate-issue branch from 212b4de to a0b6799 Compare April 9, 2020 21:27
@paul-marechal paul-marechal changed the title plugin-ext: late activation plugin-ext: late textmate grammar activation Apr 9, 2020
@paul-marechal paul-marechal force-pushed the mp/cpp-textmate-issue branch from a0b6799 to 78f839a Compare April 9, 2020 21:30
@marcdumais-work
Copy link
Contributor

marcdumais-work commented Apr 13, 2020

Setup:

I did a quick sanity check on this PR, using Gitpod, and the following content saved as a.h in the workspace:

/* > Description *********************************************************************/
/**
 * sdads das asd asd asdasdasdasedasdsdas  dsa asd asd sd asdasd 
 * as dasd sd ad sad sad asd sad sad asd asdasdasdasdasdasdsadasddas
 *
 *****************************************************************************/
 /**
  * @file asdasasdasdasdasdasd.h
  * @brief
  * @see
  *
 *****************************************************************************/

/*>a************/
#ifndef asdasdasdasdasdasd
#define asdasdasdasdasdasd
#endif

There is another bug, related to the cpp grammar that's used in the example app: builtin [email protected]. So I removed the entry for this builtin from root package.json and removed the extension from ./plugins:

# stop test instance
rm -rf plugins/vscode-builtin-cpp/
# restart test instance
yarn --cwd examples/browser start ../.. --hostname=0.0.0.0

Then I installed the latest cpp built-in using the extensions view.

Test:

Without this fix, I only have to reload the application with editor containing a.h opened, and upon reload the syntax coloring is broken in the comments. With this fix, the coloring is still good after a few reloads! So it looks promising!

There is a race condition when the layout is restored: if an editor was
opened, it will be restored and wait for some language activation event.
The issue with this was first seen with the builtin cpp extension: two
cpp grammars are contributed, but the first one is not the right one to
use on a cpp file. When an editor opened, the first bogus grammar is
loaded and activated, triggering bogus coloration.

This commit ensures that all grammars are more correctly contributed
before activating anything in the next event-loop tick.

Signed-off-by: Paul Maréchal <[email protected]>
@paul-marechal paul-marechal force-pushed the mp/cpp-textmate-issue branch from 78f839a to 55a7629 Compare April 13, 2020 16:22
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.

code-wise it makes sense, i have not tested, please make sure that it does not introduce regressions for previous changes

  • use blame to identify commits
  • from commits look at PRs
  • run tests from How to test sections on such PRs

@paul-marechal
Copy link
Member Author

@akosyakov thanks, I tried the instructions from #6966 and did not notice any regression.

@paul-marechal paul-marechal merged commit c5d3708 into master Apr 14, 2020
@paul-marechal paul-marechal deleted the mp/cpp-textmate-issue branch April 14, 2020 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug bugs found in the application textmate issues related to the textmate grammars
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sometimes broken TextMate coloring in header files
3 participants