Replace document_highlight_scopes setting with better default scopes

[jwortmann]

Resolves #1584

I saw that the markup.error/markup.warning/markup.info scopes for diagnostic regions in ST3 got "lost" in #1170 (but they are still used for the diagnostics panel). I would suggest to add them back on top of the region.* scopes which are currently used, because the markup scopes seemed like a nice semantic description and allow configuration of the styles for diagnostics colors independently of all the other places where region.* colors are used in ST.

I could also replace the text / markup.inserted / markup.changed scopes for documentHighlight that are kept for backwards compatibility by one (or separate) region.* scopes if desired.

Please checkout my branch and confirm that it actually works before merging this PR, because I haven't.

[rwols]

Nice work. Please also update sublime-package.json (used by LSP-json)

[rwols]

Can you give an example on how one would achieve the effect as in your gif in the linked issue? I've checked out your branch, and these are my overridden rules for Mariana:

{
    "rules": [
        {
            "scope": "markup.error",
            "foreground": "hsl(357, 79%, 65%)"
        },
        {
            "scope": "markup.warning",
            "foreground": "hsl(32, 93%, 66%)"
        },
        {
            "scope": "markup.info",
            "foreground": "hsl(0, 0%, 100%)"
        },
        {
            "scope": "markup.info.hint",
            "foreground": "hsl(0, 0%, 97%)"
        },
        {
            "scope": "markup.highlight",
            "background": "color(rgb(255, 255, 255) alpha(0.1))"
        }
    ],
}

It looks like this:

It looks like the color of markup.changed and markup.inserted is still applied, but also together with my markup.highlight scope? I don't really understand what is going on here.

[jwortmann]

This is correct, because markup.changed/markup.inserted will still match and they use (only) a foreground color. Therefore both your new rule for markup.highlight and the predefined rule from the color scheme for markup.changed/markup.inserted get applied. I think this is how color schemes work in general, it's just that the rule with the highest selector score gets applied last and then overrides potential colors from other rules for "background", "foreground" and styles for "font_style".

What I did was setting both a foreground and background color in the rule for markup.highlight in my screenshot (with foreground using the default text color). You could see from my screenshot that the foreground color will even override the syntax highlighting for the function argument when I click on "alpha".

For Mariana it would be

        {
            "scope": "markup.highlight",
            "foreground": "var(white3)",
            "background": "color(rgb(255, 255, 255) alpha(0.1))"
        }

In this PR I also added meta.fill to the scopes only when "fill" is used, so that color scheme authors could use a rule that applies only to filled regions, or tweak the style for both "fill" and other options.

Also I observed that setting an alpha value for the background color unfortunately has no effect for when the scope is used with view.add_regions. It would be nice to have blending colors for when "highlight_line" is used or within the red/green line backgrounds from ST's incremental diff feature.

[jwortmann]

Also ST seems to alter its behavior how to draw view.add_regions withouth sublime.DRAW_NO_FILL dependent whether the used scope has a background color assigned or not. The default markup.inserted/changed scope does not have a background color, and in that case ST uses the foreground color as background and automatically adjusts the foreground to provide a sufficient contrast.

[jwortmann]

It's even more confusing because ST seems to have another special behavior only when the scope name contains one of the region scopes. I replaced the markup.inserted and markup.changed with a region scope for testing (i.e. "region.greenish markup.highlight.read.lsp"), and in that case it looks like the original foreground color gets preserved, even with a color scheme rule with "foreground" for markup.highlight. And instead of the text foreground, this color scheme rule now controls the border color. Here is the result for Mariana with

        {
            "scope": "markup.highlight",
            "foreground": "#f0f",
            "background": "color(rgb(255, 255, 255) alpha(0.1))"
        }

And when I change the code to apply sublime.DRAW_NO_OUTLINE for "fill", the foreground color still has no effect:

Perhaps replacing the markup.changed/markup.inserted scopes with region scopes and using the sublime.DRAW_NO_OUTLINE flag would be the best option to allow a nice highlighting style for "fill", while still providing a good default style for "underline" and the other options.

To be continued....

[rwols]

See inline comment

[jwortmann]

...continuing from the inline comment above, because I did some more experiments.

My idea with the meta.fill scope to allow to target only the style in case of the "fill" setting has got some side effects if used together with the region scopes. The highlighting with preserved text color only works as shown in the screenshot above, if the region scope is the very first part of the full scope name, for example region.yellowish meta.fill markup.highlight. If the meta.fill gets prepended instead, the "foreground" color from a potential color scheme rule for meta.fill markup.highlight does get applied to the text color. Furthermore, the default style is pretty bad in that case, because the default text color has very little contrast to the background color.

So I think we have two options now:

1. Don't use the sublime.DRAW_NO_OUTLINE flag if the user setting is "fill" (current behavior): This way a border is drawn by default and the color scheme would require two separate rules to target the different styles, one of them for markup.highlight controlling the underline color via "foreground", and the other with e.g. meta.fill markup.highlight scope and "foreground" and "background" set to the same color value (assuming no visible border is desired). It also requires a bit more complicated logic to get the meta.fill into the middle of the full scope name. Alternatively I could use a scope like markup.highlight.read.fill.lsp to circumvent that, but it's still a bit awkward to implement and for users to customize.
2. If sublime.DRAW_NO_OUTLINE is used for "fill", something like a meta.fill scope is not necessary, because then a single color scheme rule for markup.highlight would be sufficient for all possible styles ("foreground" only controls the underline & outline ("box") colors, while "background" only controls the "fill" background and they don't affect each other). If one of "foreground" or "background" is missing, or if there is no color scheme rule for markup.highlight, the region colors of course are still applied. The only downside here would be that no border is possible at the same time with "filled" highlighting. But if a border is desired, the "box" setting can still be used as an alternative.

This is how the "fill" style looks by default in Mariana without any special color scheme rules:

I've implemented the option 2 now, because I think the border makes no big difference here and it's more simple for user configurations too. I would assume that the "fill" setting isn't used very much anyway, because the default highlighting style for it is very strong and distractive. But this PR easily allows to set a faint background now, which looks pretty nice then.

Notice that the removed border for the "fill" style will apply to diagnostics as well. Should I add back the previously used markup scopes on top of the region scopes for diagnostics here in this PR, so that color customizing becomes possible again for those too?
I added back the markup scopes for diagnostics too.

The chosen scopes for documentHighlight are now

• "region.bluish markup.highlight.text.lsp" (this is a new default color for the "Text" kind)
• "region.greenish markup.highlight.read.lsp"
• "region.yellowish markup.highlight.write.lsp"

to match the current colors for Read and Write kinds.

Another benefit from using the region scopes is, that apparently the font style (italic) is also preserved. The only problem I noticed with the default background color from region.greenish is that it has a very bad contrast in Mariana with the blue foreground color that is used e.g. for function calls.

[rwols]

I'll run with this for a few days and see if anything odd turns up. But it's looking good so far. One odd thing is that sublime doesn't remember the border color of other regions drawn with a box, e.g. diagnostic:

But, that's really more of an ST issue I guess.

[jwortmann]

I'll run with this for a few days and see if anything odd turns up.

Yes, sounds useful.

One odd thing is that sublime doesn't remember the border color of other regions drawn with a box, e.g. diagnostic:

I assume ST just draws the style on top from whatever view.add_regions call comes last, and in this case it's the one from documentHighlight. If you look closely at your screenshot you can see that the background from the documentHighlight spans the whole line height, and therefore just covers the diagnostics border too. How does it look if you change the diagnostics style to "fill" and the document highlight style to "box"?

[jwortmann]

Have you pulled my changes with sublime.DRAW_NO_OUTLINE from 3560d6d? If yes, then this looks like a regression in ST4, because it works fine in ST3:

Even if I set both "foreground" and "background" in my color scheme rule for markup.highlight, and also with the default style when there's no rule for it at all.

[jwortmann]

Nope, minimum working example shows the same behavior as in your screenshot in ST3 as well.

>>> view.add_regions("first", [sublime.Region(10, 30)], "region.redish markup.error", "", sublime.DRAW_NO_FILL)
>>> view.add_regions("second", [sublime.Region(15, 20)], "region.greenish markup.highlight", "", sublime.DRAW_NO_OUTLINE)

And still no change when I remove the region scope from "first". Not sure what the difference in the code is 🦧

I'll come back to this later.

[jwortmann]

I did some more testing and I see a quite random behavior for this. Look at the following gif, now I have two files open from the same folder, and for one of the files it behaves like in your screenshot, while for the other file the border is drawn on top like I described above.

Perhaps it's some kind of race condition dependent on which of the region keys gets created first for a particular view, or something like that. But apparently it has got nothing to do with the styles or scope names from this PR.

[jwortmann]

This indeed seems to be the case. You can test this by creating a new view, and then paste the two lines from my comment above into the console, but in reversed order ("second" first).

That means if you open a file and use the DocumentHighlight functionality before diagnostics are reported, you will get the desired behavior with the diagnostics border on top. So theoretically we could run add_regions with keys "lsp_highlight_text", "lsp_highlight_read", "lsp_highlight_write" and empty region arrays each, whenever a new file gets opened, to ensure the diagnostics border is always drawn on top. But this fails as soon as someone uses "fill" for diagnostics and another style for the document highlights. Anyway, I think that is out of scope for this PR.

[rwols]

LGTM

[rchl]

There is a lot of information here. I wonder if it would be useful to summarize it somewhere (in documentation?) in short points. I'm interested in customizing some stuff but it still feels like trial and error. Note that I haven't yet have time to really dig into what you wrote @jwortmann

[jwortmann]

I think the description and scope names that I already added to the documentation should be sufficient to customize the highlighting for diagnostics and document highlight. The only thing you need to know is how to customize your color schemes (i.e. create a file with the same name in your Packages/User folder), and that the "background" color from a color scheme rule controles the "fill" background color, while "foreground" controls all other (underline & box style) colors. If theres still something missing or it doesn't work as expected, let me know.

Notice that colors with alpha value are calculated correctly (against the global background color), but Sublime doesn't acually applies them with transparency (e.g. when drawn over an active line with "highlight_line": true in ST settings). I assume this happens for perfomance reasons, to avoid to dynamically recalculate the color everytime.

Ideally, color scheme authors need to follow now to provide a nice style for their color scheme. In jwortmann/brackets-color-scheme@8479e65 I added support for it in my color scheme.

Previously I disabled the DocumentHighlight functionality in LSP for myself, because the default colors looked ugly and too strong in my opinion. But with this adjustment and the style set to "fill" it looks much better to me and it might be a nice feature now. If the default colors weren't so distractive, we should even consider to use "fill" as the default style, because that's the style suggested from the specs:

/**
 * A document highlight is a range inside a text document which deserves
 * special attention. Usually a document highlight is visualized by changing
 * the background color of its range.
 *
 */
export interface DocumentHighlight {

I'm gonna make a comment about this new scope that we use here in the markup scopes RFC discussion in Sublime's Packages repo later.

---

Everything from my most recent comments here explains the deduction used to find a bug about the inconsistent behavior whenever regions from diagnostics and document highlights are drawn on top of each other, caused by a race condition.

I can create a separate issue for it, but I think it's low priority.

[predragnikolic]

Maybe not use abbreviation like "sighelp"?

[jwortmann]

This was added by @rwols in #1576. I do find it useful, so that it can be easily targeted by color schemes. Or do you mean write it out as e.g. "entity.name.function.signature-help.lsp" instead of "sighelp"?

But besides that, looking at the docs now, I recommend to change "markup.codelens.accent" to "markup.accent.codelens.lsp". The "markup.accent" looks general enough so it could be reused by other features than code lens, or other plugins too, and the "lsp" suffix to keep all scopes from LSP consistent.

Or maybe even better not to use a TextMate scope here, and instead use the "accent" color from the color scheme via var(--accent)? The related code for this is at

LSP/plugin/documents.py

Line 506 in 4610a1b

accent = self.view.style_for_scope("region.greenish markup.codelens.accent")["foreground"]

[predragnikolic]

Or do you mean write it out as e.g. "entity.name.function.signature-help.lsp" instead of "sighelp"?

Yeah I meant that, rename "sighelp" to "signature-help". (abbreviations are sometimes hard to understand)
But because this was already added I don't want to introduce breaking changes, so ignore this comment.

because this was recently added 11 feb. maybe worth reconsidering renaming... but again this is not a big deal.