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

Use more specific default scopes for textDocument/documentHighlight #1584

Closed
jwortmann opened this issue Feb 15, 2021 · 2 comments · Fixed by #1585
Closed

Use more specific default scopes for textDocument/documentHighlight #1584

jwortmann opened this issue Feb 15, 2021 · 2 comments · Fixed by #1585

Comments

@jwortmann
Copy link
Member

The scopes for textDocument/documentHighlight regions can be configured via the "document_highlight_scopes" setting.

  1. It would be nice to have more specific default scopes here, such that color scheme authors can easily provide a better, customized style for them. My suggestion would be
  "document_highlight_scopes": {
    "unknown": "text markup.highlight.text.lsp",
    "text": "text markup.highlight.text.lsp",
    "read": "markup.inserted markup.highlight.read.lsp",
    "write": "markup.changed markup.highlight.write.lsp"
  },

These scopes are fully backwards compatible for color schemes which don't target markup.highlight (probably all existing color schemes). I saw that @rwols suggested markup.related.read and markup.related.write in sublimehq/Packages#1036 (comment), which could be an alternative, but I think I would prefer markup.highlight, because documentHighlight is the corresponding request name.
(Also VS Code themes have direct color settings for them (instead of Textmate scopes), which are named "editor.wordHighlight[...]" for DocumentHighlightKind.Read and "editor.wordHighlightStrong[...]" for DocumentHighlightKind.Write.)

  1. If the scopes get documented well and ideally mentioned in an update message for the next release, this setting could even be removed completely, because users could then use a color scheme customization for these scopes, instead of changing the scope names in the settings.

  2. Besides that, I would suggest to drop the "unknown" setting, and fall back to "text" if the DocumentHighlightKind is omitted, because there is no "unknown" kind in the specs, and Text is the default kind:

export interface DocumentHighlight {
	/**
	 * The range this highlight applies to.
	 */
	range: Range;

	/**
	 * The highlight kind, default is DocumentHighlightKind.Text.
	 */
	kind?: DocumentHighlightKind;
}

/**
 * A document highlight kind.
 */
export namespace DocumentHighlightKind {
	/**
	 * A textual occurrence.
	 */
	export const Text = 1;

	/**
	 * Read-access of a symbol, like reading a variable.
	 */
	export const Read = 2;

	/**
	 * Write-access of a symbol, like writing to a variable.
	 */
	export const Write = 3;
}

I.e. this kind value doesn't exist in the specs and the fallback value here should be DocumentHighlightKind.Text.

What do you think about 1./2./3.?

@jwortmann
Copy link
Member Author

Here is an example how this allows subtle background styling with a single rule for markup.highlight with foreground & background color in my color scheme and "document_highlight_style" set to "fill". Unfortunately Sublime's add_regions obtains the underline/outline color also from "background", so color scheme authors need to decide if they want a nice style either for "fill", or for the other options.

highlight

Should I prepare a PR?

@rwols
Copy link
Member

rwols commented Feb 16, 2021

Should I prepare a PR?

Sure, looking forward to it. Having document_highlight_scopes as a setting was a mistake in hindsight.

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

Successfully merging a pull request may close this issue.

2 participants