Skip to content

Conversation

joshuaflores
Copy link

Hey there thanks for making this plugin. I needed syntax highlighting for elixir and noticed that shiki was out of date. I updated that dependency and moved from the now deprecated oniguruma to vscode-oniguruma.

Copy link
Owner

@mattbierner mattbierner left a comment

Choose a reason for hiding this comment

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

The shiki changes look good overall but the PR is doing several things which makes it difficult to review. I suggest keeping this one focused just on the shiki update and handling the language changes in a separate PR

"name": "docs-view",
"displayName": "Docs View",
"description": "Display documentation in the sidebar or panel.",
"version": "0.0.11",
Copy link
Owner

Choose a reason for hiding this comment

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

Please revert this. I will bump the version before publishing it

theme = await shiki.loadTheme(currentThemeName)
}

if (theme) {
Copy link
Owner

Choose a reason for hiding this comment

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

With this PR, the background is showing again on code blocks (we want to hide it). Does restoring this code fix that or is some other fix needed?

Copy link
Author

Choose a reason for hiding this comment

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

I applied the wrong function here. I reverted but the background color was still showing. For some reason shiki seems to be ignoring theme.bgand instead using colors.editorBackground. I've applied a patch but perhaps doing an override via css might be better. For future reference the css fix

pre.shiki {
   background-color: transparent !important;
}

Copy link
Author

Choose a reason for hiding this comment

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

I was wrong on this one: so what I gather is happening is that vscode themes have an optional settings section in their declaration file where they can declare styles to be applied to that scope. The problem is that some themes add a setting that with a global scope and that settings applies to everything. It seems that shiki is prioritizing the result of settings over theme.bg. In any case I just pushed a fix that should solve this. Although this works this the css fix is the css patch seems more reliable in the long run.

{ name: 'makefile', language: 'makefile', identifiers: ['Makefile', 'makefile', 'GNUmakefile', 'OCamlMakefile'], source: 'source.makefile' },
{ name: 'perl', language: 'perl', identifiers: ['perl', 'pl', 'pm', 'pod', 't', 'PL', 'psgi', 'vcl'], source: 'source.perl' },
{ name: 'r', language: 'r', identifiers: ['R', 'r', 's', 'S', 'Rprofile'], source: 'source.r' },
{ name: 'r', language: 'r', identifiers: ['R', 'r', 's', 'S', 'Rprofile', '\\{\\.r.+?\\}'], source: 'source.r' },
Copy link
Owner

@mattbierner mattbierner Apr 26, 2023

Choose a reason for hiding this comment

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

These changes seem kind of unfocused. Can you split them out into their own PRs with better explanations of why each one is needed

For all the ones adding support for {.LANG...}, could that just be a generic thing that happens automatically based on existing identifiers?

Copy link
Author

Choose a reason for hiding this comment

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

I agree that would be a better fix, I needed a quick fix so I just updated the const from 'https://github.com/Microsoft/vscode-markdown-tm-grammar/blob/master/build.js'

What if we just modify getHighlighter to something like this? document.languageId already returns the language identifier for the file so I'm not sure why the identifier lookup is happening here. By dropping the getLanguageId call, I'm getting syntax highlighting on languages not mentioned in const languages block

public async getHighlighter(document: vscode.TextDocument): Promise<(code: string, language: string) => string> {
		const highlighter = await this._highlighter;


		return (code: string, inputLanguage: string): string => {
			const language = inputLanguage || document.languageId;

			if (language && highlighter) {
				try {
					// const languageId = getLanguageId(language);
					if (language) {
						return highlighter.codeToHtml(code, { lang: language });
					}
				} catch (err) {
					// noop
				}
			}

			return code;
		};
	}

Copy link
Author

Choose a reason for hiding this comment

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

just created a new pr #45 based on this change. looks good on my end.

@mattbierner
Copy link
Owner

Closing this for now since it's gotten out of date. I believe the Shiki part has now been tackled but please submit another PR with the language changes if those are still needed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants