Create extra regions for diagnostics with tags

[rchl]

Creates extra regions with tag-specific scopes so that it's possible to target unused imports or variables with a scheme customizations.

I probably don't consider it ready yet because it has some drawbacks.

• I'm creating new regions instead of appending tag scopes to existing diagnostic regions. That is because then it would only work if the user has specifically chosen "box" style for diagnostics.
• That has a downside that if the user doesn't customize the style for the tagged scope, those will be shown with an ugly filled box:
  
• On the other hand, if the style for the tagged scope is customized then it can look decent (I haven't been able to get it to look just right - I'd prefer just fading the text color without having any extra fill):
  
  The style above is with this scheme customization:

        {
            "scope": "markup.tag.unnecessary.lsp",
            "foreground": "color(rgb(255, 255, 255) alpha(0.2)"
        }

[rchl]

In a quest to try to make the text color faded without changing the background color or using fill, I've tried the workaround from sublimehq/sublime_text#817 (comment) but it didn't seem to work.

                        flags = (sublime.DRAW_NO_FILL | sublime.DRAW_NO_OUTLINE)
                        self.view.add_regions(key, data.regions, ' '.join(tag_scopes), flags=flags)

        {
            "scope": "markup.tag.unnecessary.lsp",
            "foreground": "color(rgb(120, 120, 120) alpha(0.2)",
            "background": "var(blue4)"
        }

[predragnikolic]

Rafal would you like something like this?

        {
            "name": "Unused",
            "scope": "unused",
            "foreground_adjust": "l(- 40%)",
            "background": "#00000001"
        },
        

you can try it in the ST console:

view.add_regions('unused', [sublime.Region(0, 20)], scope="unused")

[jwortmann]

You need to use fill for this to work. But the only way to provide a nice default style for this would be hijacking the color scheme and inject a rule to it, but I would really avoid to do that.

This PR with markup.tag scopes can only be a workaround, but the intended ways for dimmed or strike-through style as suggested in the specs should be independent of the color scheme and would require an implementation for that in ST core / API update. Because e.g. the "dimmed" style should be possible for different colors of various tokens, so a scope rule for that isn't sufficient. It would require new flags like sublime.DRAW_STRIKETHROUGH and sublime.DRAW_DIMMED for the add_regions method.

I want to write this in the Packages discussion later, but have some cooking/baking stuff to do first now.

[rchl]

Rafal would you like something like this?

That works when tested in isolation but not when applying the same scopes to those regions that I'm creating here. Possibly because those are drawn on top of existing diagnostic regions.

This PR with markup.tag scopes can only be a workaround, but the intended ways for dimmed or strike-through style as suggested in the specs should be independent of the color scheme and would require an implementation for that in ST core

I agree but I kinda want it now so if we can find a solution that works partially without affecting normal uses then I'd still want to go with it. As it is now it's affecting normal uses as it creates an extra filled region though...

Because e.g. the "dimmed" style should be possible for different colors of various tokens,

Yes, but a partial solution that always uses a single color (gray or whatever fits the color scheme) would be sufficient for now IMO.

[rwols]

Can you extend the test in https://github.com/sublimelsp/LSP/blob/st4000-exploration/tests/test_server_notifications.py ?

[rchl]

Extending the test revealed something I wasn't aware of before - adding two regions with the same key, at the same position, makes ST consolidate them somehow, resulting in only one region being returned from view.get_regions. So I'm creating tags using a different region key now.

That also makes the trick that @predragnikolic pointed out work correctly so I guess that's a good thing.

Of course, there is still a problem that when there is no style assigned to markup.tag.unnecessary.lsp in the color scheme then the region will be an ugly, filled box. But it can be made like this now, with proper style overrides:

[rwols]

With these extra rules in my color scheme as suggested by @predragnikolic:

        {
            "scope": "markup.tag.unnecessary",
            "foreground_adjust": "l(- 40%)",
            "background": "#00000001"
        },

I get this:

Looks pretty good. But if there's no appropriate rule then the default looks jarring I agree. By the way, I don't see the squiggles underneath as in your screenshot.

[rchl]

In Mariana I've used this override which seems better:

        {
            "scope": "markup.tag.unnecessary.lsp",
            "foreground": "color(rgb(255, 255, 255) alpha(0.4))",
            "background": "color(var(blue3) alpha(0.9))"
        },

[rchl]

By the way, I don't see the squiggles underneath as in your screenshot.

The squiggles appear only if the diagnostic region is added after the tag region. If you create an unused variable then the squiggles will show because there will be those two diagnostics sent, first with the tag and the second without:

    {
      "message": "\"unuseds\" is not accessed",
      "range": {
        "end": {
          "character": 23,
          "line": 120
        },
        "start": {
          "character": 16,
          "line": 120
        }
      },
      "severity": 4,
      "source": "Pyright",
      "tags": [
        1
      ]
    },
    {
      "code": "reportUnusedVariable",
      "codeDescription": {
        "href": "https://github.com/microsoft/pyright/blob/master/docs/configuration.md"
      },
      "message": "Variable \"unuseds\" is not accessed",
      "range": {
        "end": {
          "character": 23,
          "line": 120
        },
        "start": {
          "character": 16,
          "line": 120
        }
      },
      "severity": 3,
      "source": "Pyright"
    }

So I guess I can try to create tag diagnostics first.

[jwortmann]

So I guess I can try to create tag diagnostics first.

Your implementation for this is wrong. The reason why the squiggly underline can only sometimes be seen is the same as explained in #1585. So the only way to solve this would be to create empty regions for all of the "tag" keys whenever a view gets opened, before any other diagnostics are reported.

I don't think the markup.tag scope is really good, because the "unnecessary" and "deprecated" properties seem sufficiently different to me and I would maybe think of HTML tags or @param in JSDoc for such a scope. So if you really want to introduce scopes for these, I would rather go with markup.unnecessary and markup.deprecated.

And what's the plan with this PR in general? I guess 99% of color schemes won't have rules for these scopes in the foreseeable future, and then the background will use the global foreground color which looks just ugly.

[rchl]

Your implementation for this is wrong. The reason why the squiggly underline can only sometimes be seen is the same as explained in #1585. So the only way to solve this would be to create empty regions for all of the "tag" keys whenever a view gets opened, before any other diagnostics are reported.

There is a defined order to diagnostic regions creation. First, we are adding regions for errors, then warnings, infos, and hints. Any of those can also include an "unnecessary" or "deprecated" tag. As long as we create that region before the actual diagnostic, it should be fine. Unless the later diagnostic is added at the exact same region in which case it won't work entirely correct.

I don't think the markup.tag scope is really good

I don't have a strong preference and don't mind tweaking that.

And what's the plan with this PR in general?

I hoped we could find a way to introduce those scopes without affecting setups that don't have scheme customizations but if that's not possible then I guess I'll have to admit defeat and wait for a proper API for that.

[jwortmann]

As long as we create that region before the actual diagnostic, it should be fine. Unless the later diagnostic is added at the exact same region in which case it won't work entirely correct.

Not sure if I understand exactly what you mean with this, however my experiments showed that the order when a specific regions key is created the first time for a particular view, is the crucial point. You can try that yourself:

1. Create or open a view with some diagnostics in it, but none of the diagnostics can have a tag.
2. Edit the view's content such that a diagnostic with a tag gets reported, with the region overlapping with another diagnostic (with a kind which was already there before).

Then you should see that the tag-diagnostic will still be drawn on top.

I hoped we could find a way to introduce those scopes without affecting setups that don't have scheme customizations but if that's not possible then I guess I'll have to admit defeat and wait for a proper API for that.

I can think of only one "trick" how you could do some styling with regions without having color schemes affected that don't have the corresponding scope. You can try the scope region.deprecated.lsp with flag sublime.DRAW_NO_OUTLINE. This should have no effect if there's no rule for it, but you can style the background (and nothing else) how you like with a color scheme rule for it.

[jwortmann]

Have you tried my latest suggestion and would it be a sufficient alternative?

I had another idea how to allow special highlighting for diagnostic tags without bad side effects if there's no color scheme rule, hacking into user's color schemes, or waiting for an API update:
Just check beforehand whether there's a rule for the scopes in the color scheme, and only draw the regions if that is the case.

if "background" in view.style_for_scope("markup.unnecessary.lsp"):
    view.add_regions(..., scope="markup.unnecessary.lsp", flags=sublime.DRAW_NO_OUTLINE)  # regions with "unnecessary" tag
if "background" in view.style_for_scope("markup.deprecated.lsp"):
    view.add_regions(..., scope="markup.deprecated.lsp", flags=sublime.DRAW_NO_OUTLINE)  # regions with "deprecated" tag

The above scopes seem uncommon enough to only have a background color if someone really intends to tweak the look for this LSP workaround (they would also match in case someone had a rule for only "markup" - but that's quite unlikely I think).

[rchl]

I can think of only one "trick" how you could do some styling with regions without having color schemes affected that don't have the corresponding scope. You can try the scope region.deprecated.lsp with flag sublime.DRAW_NO_OUTLINE. This should have no effect if there's no rule for it, but you can style the background (and nothing else) how you like with a color scheme rule for it.

Have you tried my latest suggestion and would it be a sufficient alternative?

I haven't tried but since you said that only the background can be changed then it wouldn't really be enough because I want to fade out the foreground color.

Your latest trick looks quite nice though. I'll play with it at some point.

[rchl]

I suppose there might still an issue with the order the diagnostic and tag regions are drawn in some cases. Will have to check it out later.

[rchl]

1. Create or open a view with some diagnostics in it, but none of the diagnostics can have a tag.
2. Edit the view's content such that a diagnostic with a tag gets reported, with the region overlapping with another diagnostic (with a kind which was already there before).

Then you should see that the tag-diagnostic will still be drawn on top.

Can you think of any way I could reproduce that? The error diagnostics disappear when I make the code unused because pyright does no longer report them so I'm not sure how this can be reproduced.

[jwortmann]

Can you think of any way I could reproduce that? The error diagnostics disappear when I make the code unused because pyright does no longer report them so I'm not sure how this can be reproduced.

But the reported tag is always a part of a diagnostic, so how can pyright no longer report the diagnostic for it?

To ensure that I understand correctly, you want to achieve the following, right?

I.e. have a region to adjust the foreground color via a color scheme rule, but the squiggle or box for the corresponding diagnostic severity should still always be drawn on top of that region so that it's still visible.

First you need to find out which severity the reported diagnostics with the "unnecessary" tag from pyright have (probably "information" or "hint"). Then you could either produce any other diagnostic with that same severity, but no tag, in a new file, or in case such a diagnostic doesn't exist for pyright, you could also override the severity for any diagnostic type via the "python.analysis.diagnosticSeverityOverrides" setting. So for example assign the corresponding severity to a syntax error diagnostic, and then in a new file produce that syntax error first, and afterwards the diagnostic with a tag.

Edit 1: Maybe my description from the previous comment was a bit confusing because the "overlapping" regions referred more to the same problem for the documentHighlight regions pointed out before. But in this case here with the diagnostics with tags, the regions (i.e. their positions) are actually the exact same, because we want to draw two seperate regions on top of each other from a single diagnostic.

Edit 2: Okay, now I understand what you meant with that pyright no longer reports the diagnostic. I guess this missunderstanding was caused because my "overlapping regions" from above was unclear and misleading. So to sum up, we want to ensure that the first time when the tag-diagnostic regions keys are created, happens always before when the tag-less region keys get created the first time. This is only an issue if a language server uses a certain severity of tag-diagnostics (probably "hint" or "information") not exclusively for those, but also for other tag-less diagnostics. Because in this case the tag-less diagnostics might occur before any of the tag-diagnostics, and therefore will always kept to be drawn "on the bottom". This may not be a problem for pyright if all tag-less diagnostics have another severity. I have not tested whether view.erase_regions will reset the region drawing order of ST, but this wouldn't save us anyway because an unrelated tag-less diagnostic with the same severity could still be in the view when a new tag-diagnostic gets reported. I hope this explanation wasn't even more confusing than this discussion already has been.

[jwortmann]

I created an issue for this at #1593. A proper solution should also include the regions from dcoument highlights, because the dimmed foreground for "unnecessary" tags and the style for document highlights can probably not be displayed at the same time if used for a single token.

[rchl]

Created some snippets for pasting into the console as I can't be bothered to try to find a way to force the server into doing things in the right order...

# diagnostic - box
view.add_regions("diag", [sublime.Region(0, 100)], scope="region.bluish markup.info.hint.lsp", flags=33)
# diagnostic - squiglly
view.add_regions("diag", [sublime.Region(0, 100)], scope="region.bluish markup.info.hint.lsp", flags=2337)

# tag
view.add_regions("diag_tags", [sublime.Region(0, 100)], scope="markup.unnecessary.lsp", flags=256)

# document highlight
view.add_regions("highlight_read", [sublime.Region(0, 100)], scope="region.greenish markup.highlight.read.lsp", flags=800)

because the dimmed foreground for "unnecessary" tags and the style for document highlights can probably not be displayed at the same time if used for a single token.

The tag region will override document highlight or diagnostic if created later than those, unfortunately.
But it works if the tag region is created first.

[rchl]

I suppose some central region manager could solve the ordering issue but it would incur some overhead since we would have to in some cases remove some regions and re-apply them in different order.

EDIT: I've just read your #1593 which shows that there is more to it than I thought.

[rchl]

I'm thinking that we don't need two separate regions if the diagnostic includes the "Unnecessary" tag. We can just create one with relevant scopes. That would somewhat solve the main issue here.

[jwortmann]

I'm thinking that we don't need two separate regions if the diagnostic includes the "Unnecessary" tag. We can just create one with relevant scopes.

Yes, that sounds like the easiest and best option for now. This would have the same effect as drawing the "filled" region always on top and hiding the squiggly/underline/box, but this is exactly what is recommended in the specs for it.

I haven't digged deep into the code how the general diagnostics logic is implemented, but may there still be a logical error in the implementation right now? From this PR I see simplified:

        for severity in reversed(range(1, len(DIAGNOSTIC_SEVERITY) + 1)):

            data = data_per_severity.get(severity)
            # ...
                if data.tags:
                    # ...
                if tag_scopes:
                    self.view.add_regions(key_tag, data.regions, ' '.join(tag_scopes), flags=sublime.DRAW_NO_OUTLINE)
                else:
                    # allow showing diagnostics with same begin and end range in the view
                    flags |= sublime.DRAW_EMPTY
                    self.view.add_regions(key, data.regions, data.scope, data.icon, flags)

So that looks like to me as if now all diagnostics of a particular severity will be drawn with the "filled" (unnecessary/deprecated) style (if supported by the color scheme), even when only a single diagnostic of that severity has the tag. What if a server reports some diagnostics without tags, and some other diagnostics with tags, but all with the same severity? Not sure where/how the data.tags is stored currently, but I believe it needs to be stored per diagnostic, and not per severity.

[rchl]

Not sure where/how the data.tags is stored currently, but I believe it needs to be stored per diagnostic, and not per severity.

That's true. Good catch.

[rchl]

The updated logic should be correct although I feel the code is getting messy and harder to follow...

[rchl]

I think I won't come up with anything better so either we go with this or scrap the idea and wait for some native API instead.

[jwortmann]

I haven't tested it but I added some suggestions. When these are addressed, maybe we should just try it, and if anything odd shows up we can always adjust or revert the PR later. Not sure if any API update is planned or considered for this by SublimeHQ in the future. So if the diagnostic tag styling should be implemented, then this workaround is probably the only way for now.

[rwols]

Let's try it out