Allow custom merge semantics for TextPresentation styles

[gregdyke]

• [x] I verified latest Integration Build of Eclipse SDK doesn't provide the feature.

Suggestion

When implementing an ITextPresentationListener (in my case, a SemanticHighlightingReconcilerStrategy for LSP4E), it would be helpful to have more options than just merging or replacing a new set of StyleRange[].

Proposed implementation would be to slightly decouple applyStyle(StyleRange, StyleRange) (logic for combining the 2 styles) from the applyStyleRanges (logic for determining overlapping style ranges between existing style ranges and to-be-merged-or-replaced style ranges)

More specifically, I would like to make it so that the Semantic Highlighting is able to replace some aspects of the existing TextPresentation, while merging others. For example:

• compare view highlight sets the background colour of a range that has been changed
• syntactic highlight from TM4E in LSP4E sets all apparent keywords for a language to bold
• semantic highlight (obtained at some point in the future from the language server) would like to unbold keywords that are actually not keywords, but preserve the background colour

(correct generic behaviour here is a bit tricky as LSP4E currently uses TM4E themes, which use SWT TextStyle to describe the desired change, without the capacity to distinguish between having and not having an opinion about bold. For this reason, it would be helpful to have a configurable option -> we can iteratively improve LSP4E to have a sensible coordination of semantic and syntactic highlighting without having to rewrite/hack TextPresentation functionality)

[mickaelistria]

I would rather see the decision and orchestration remaining on client code.

Isn't it somehow already the case for mergeStyleRange that you can call mulitple times with different ranges? It seems to me that the missing brick is more a resetStyle(IRegion range) that you could call before calling mergeStyleRange() when you want to clear a particuler range. Would that work?

[mickaelistria]

Also, in this story, I wonder whether the discussion shouldn't be more about LSP4E vs TM4E: if you have a semantic tokens support for LSP4E, then TM4E could be ignored, couldn't it?

[gregdyke]

I would rather see the decision and orchestration remaining on client code.

I don't quite understand this comment?

My suggestion is that TextPresentation keep the orchestration of figuring out what the mapping is between the existing StyleRange[] and the StyleRange[] to be applied, leaving the decision about how to merge the calculated pairs of <StyleRange template, StyleRange target> to the client. And then the implementation in the client would be:

  target.fontStyle = template.fontStyle // replace
  if (template.background != null)
    target.background = template.background; // merge
  // etc

Leaving whole thing entirely up to the client forces me to copy the code from TextPresentation that figures out the mapping, merge in client code and then call the replace API. Which is fine-ish, but I end up with 400 200 [edited after my next comment and actually counting the lines] lines of code copied from TextPresentation.

Isn't it somehow already the case for mergeStyleRange that you can call mulitple times with different ranges? It seems to me that the missing brick is more a resetStyle(IRegion range) that you could call before calling mergeStyleRange() when you want to clear a particuler range. Would that work?

Not really. There is already a replaceStyleRange(s), but that removes the style from highlighters that I want to merge with (as opposed to replace/override).

Also, in this story, I wonder whether the discussion shouldn't be more about LSP4E vs TM4E: if you have a semantic tokens support for LSP4E, then TM4E could be ignored, couldn't it?

That's something I've discussed with LSP4E: as there is typically a slight delay in getting semantic highlighting back from a language server, this would mean a brief flicker of text style for every file opening/editing. It's a nicer user experience for TM4E to apply its best guess immediately and then let LSP4E provide semantic highlighting overrides later (only on the subset of things TM4E doesn't know about/got wrong).

There are multiple things that LSP4E/TM4E doing that are not quite right

• converting CSS style to a TextStyle and losing desired styling vs implicit styling information in the process
• not having a tighter collaboration between Syntactic and Semantic Highlighting

But having a JFace API with more configurability (see my PR - it doesn't build for some reason but it's small enough to read) would help us on the path of getting it right

[mickaelistria]

So do I get it right that your goal is to "unbold" some StyleRanges? If so, can't you just try iterating on the presentation.getNonDefaultStyleRangeIterator() to collect the ranges you'd like to tweak, set styleRange.fontStyle = SWT.NONE on them, and then call presentation.replaceStyleRange() ?

[gregdyke]

Yes, that is the shortened goal. I'd considered that approach, but here's why I think it's not ideal.

Let's imagine a situation where multiple highlighters are in play in java-esque code:

• diff: showing as underline (this is an example of a highlighter that does not tokenise according to java)
• task-collector: showing task identifiers in comments as bold (this is an example of a highlighter that bolds and does not expect interference from tm4e or semantic highlighting)
• tm4e: showing keywords in java as bold
• semantic highlighting: showing keywords in java as bold and variables as italic

var foo = var; //TODO

At this point (before semantic highlighting arrives), the style ranges are {("va", 0, 1, bold), ("r", 2, 2, bold-underline), (" foo = ", 3, 9, underline), ("va", 10, 12, bold-underline), ("r", 13, 13, bold), ("TODO", 18,21, bold)}

Semantic highlighting arrives

var foo = var; //TODO

the ranges are {(("var", 0, 2, bold)), ("foo", 4, 6, italic), ("var", 10, 13, italic)}. In order to unbold according to your suggestion, it's not quite as simple as iterating over the ranges; I must do some logic to find overlaps (the set of begin/end is neither a subset nor a superset of the other). That logic is almost the same as the 200 lines (applyStyleRange and applyStyleRanges) I would otherwise have to copy from TextPresentation, and is duplicating some of the work that will be done anyway by those methods during replace/merge.

---

However, inspired by your suggestion, I thought of an alternative. Would you support the following in LSP4E? (my original reason for doing the issue/PR here is suspecting that you might not be super happy with a PR to LSP4E that copy-pastes 200 lines of code from TextPresentation)

I could first merge, guaranteeing that there are no style ranges in the presentation that overlap with style ranges from semantic highlighting (i.e. the set of begin/end in the presentation will be a superset of the those from semantic highlighting), simplifying the double-iteration loop. But that's still a 50-line algorithm that might be wrong while TextPresentation has been there for 15+ years, and there is some duplication of work in terms of performance.