-
Notifications
You must be signed in to change notification settings - Fork 192
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
Allow custom merge semantics for TextPresentation styles #2543
Comments
…pse-platform#2543 TextPresentation performs at least two tasks when applying new StyleRanges: - identify which style ranges need creating/matching with new styles - merge or replace an individual style range Decouple the two tasks by exposing methods for applying style range(s) that accept a BiConsumer to customise how style ranges should be merged
…pse-platform#2543 TextPresentation performs at least two tasks when applying new StyleRanges: - identify which style ranges need creating/matching with new styles - merge or replace an individual style range Decouple the two tasks by exposing methods for applying style range(s) that accept a BiConsumer to customise how style ranges should be merged
…pse-platform#2543 TextPresentation performs at least two tasks when applying new StyleRanges: - identify which style ranges need creating/matching with new styles - merge or replace an individual style range Decouple the two tasks by exposing methods for applying style range(s) that accept a BiConsumer to customise how style ranges should be merged
…pse-platform#2543 TextPresentation performs at least two tasks when applying new StyleRanges: - identify which style ranges need creating/matching with new styles - merge or replace an individual style range Decouple the two tasks by exposing methods for applying style range(s) that accept a BiConsumer to customise how style ranges should be merged
…pse-platform#2543 TextPresentation performs at least two tasks when applying new StyleRanges: - identify which style ranges need creating/matching with new styles - merge or replace an individual style range Decouple the two tasks by exposing methods for applying style range(s) that accept a BiConsumer to customise how style ranges should be merged
…pse-platform#2543 TextPresentation performs at least two tasks when applying new StyleRanges: - identify which style ranges need creating/matching with new styles - merge or replace an individual style range Decouple the two tasks by exposing methods for applying style range(s) that accept a BiConsumer to customise how style ranges should be merged
I would rather see the decision and orchestration remaining on client code. Isn't it somehow already the case for |
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? |
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
Not really. There is already a
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
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 |
So do I get it right that your goal is to "unbold" some StyleRanges? |
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:
At this point (before semantic highlighting arrives), the style ranges are Semantic highlighting arrives
the ranges are 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 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. |
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 theapplyStyleRanges
(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:
(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)
The text was updated successfully, but these errors were encountered: