Semantic Tokens: Do not use default theme #1029
Conversation
Fix for eclipse#1028 - we requested the default theme if no TM mapping for the editor, but this calls syncExec. Reconciler threads mustn't block on the UI thread as it is possible for the UI thread to be blocked waiting for reconciliation; this deadlock was indeed observed.
ahmedneilhussain
requested review from
sebthom and
rubenporras
and removed request for
sebthom
|
Hi @rubenporras without the fallback a test failed. I added a custom content type with a real TM grammar mapping and tweaked the expected tokens to match. Is that OK? Felt a bit heavyweight, but I couldn't see an easy way of mocking/injecting a TM mapping in the test setup. I thought it best to create a new content type mapping so that this change was segregated from the other tests. |
|
hi @ahmedneilhussain , some tests are still failing. Are these some flaky tests or does the removal of the fallback break those tests as well? Regards |
HI according to this https://ci.eclipse.org/lsp4e/job/lsp4e-github/job/PR-1029/2/testReport/ all the tests are passing. Am I missing something? Some of the CI runs are complaining because of the eclipse version bump check - I'm not sure if I'm supposed to do something about that or whether it will get bumped as part of prep for next release. There are definitely some flaky tests but I think the fallback should not interfere with the majority of tests: the mock language server won't have anything in place for semantic tokens other than where we are explicitly testing that functionality, and we don't configure tm4e other than where I just introduced it (for a new content type used only for this one case). I could be wrong, but the test run above and the fact they ran clean (but flaky!) locally suggests not... Cheers, |
|
Sorry, I have not noticed that. You need to manually bump the version of org.eclipse.lsp4e.test, if I recall right in the manifest.mf and in the pom.xml. |
Fix for #1028 - we requested the default theme if no TM mapping for the editor, but this calls syncExec. Reconciler threads mustn't block on the UI thread as it is possible for the UI thread to be blocked waiting for reconciliation; this deadlock was indeed observed.