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

[RFC] Style Identifiers #284

Open
rkusa opened this issue May 10, 2017 · 35 comments
Open

[RFC] Style Identifiers #284

rkusa opened this issue May 10, 2017 · 35 comments

Comments

@rkusa
Copy link
Contributor

rkusa commented May 10, 2017

Current Situation

We have a set_style RPC call (which is actually called def_style in the source):

set_style
  id: number
  fg_color?: number // 32-bit RGBA value
  bg_color?: number // 32-bit RGBA value, default 0
  weight?: number // 100..900, default 400
  italic?: boolean  // default false

This is intended to be used by syntax highlighting, linting and similar plugins, which set color spans for parts of the text. Instead of having to define and therefore repeat the style for each span separately, set_style allows to reuse them.

When a Line is send from the core to the frontend, it could contain a number[] array which defines the style spans of this line.

To quote the current update.md:

It is conceptually an array of triples (though flattened, so triple at is styles[i3], styles[i3 + 1], styles[i*3 + 2]). [...] The third element is a style id.

Shortcomings

When having different plugins that set style spans on their own (1 syntax highlighting, 1 linting, ...), we probably end up with colors that do not match well (i.e. how does the lint plugin know to use a red which matches my dark or light theme).

Possible Solution

As in other editors, styles could be represented by a string (often called a "scope" in other editors). For example Sublime Text defines the following for a minimal scope coverage:

entity.name
entity.other.inherited-class
entity.name.section
entity.name.tag
entity.other.attribute-name
variable
variable.language
variable.parameter
variable.function
constant
constant.numeric
constant.language
constant.character.escape
storage.type
storage.modifier
support
keyword
keyword.control
keyword.operator
string
comment
invalid

However, to not restrict plugins in their power, we might not predefine/restrict these scopes and let plugins define them theirselves.
So the Syntax Highlighting plugin might define a style for invalid, while a Lint plugin re-uses this invalid style definition or defines its own, if a style for invalid does not already exist.

That is, the set_style RPC call would have two different semantics, one that overrides a style, and one which only sets the style if it does not already exist:

set_style
  id: string
  redefine: bool // whether the style should be redefined if it already exists
  fg_color? ... italic?

Discussion

  • Should be use string identifiers at all?
  • Should the scope of set_style be to a tab, or to the global session?
  • Should these style/"scope" identifiers be predefined?
  • How to send style spans, since number[] would not be possible anymore?
@AljoschaMeyer
Copy link

Other editors have a concept of markers for general subsections of text that should move consistently with buffer manipulation. Style spans fit into that category as well. So this might be a good point to think about:

  • whether Xi will support markers
  • how spans differ from markers
  • whether it makes sense to provide both

@eyelash
Copy link
Member

eyelash commented May 10, 2017

Are you talking about the plugin <-> core protocol or about the front-end <-> core protocol? Might be good to clarify that. For the front-end <-> core protocol I think sending the colors as RGBA value is not a problem.

@raphlinus
Copy link
Member

These are good questions to think about, but I'll quickly echo what @eyelash has said. My current thinking is that plugins communicate scopes to the core, the core applies a theme and resolves those into font/color styles, and the core communicates those styles to the front-end.

I do see a numeric id system for plugin->core communication, strictly for efficiency. This lets us implement theme resolution as a simple id->id map in the hit case.

@AljoschaMeyer I haven't thought too much about markers yet but I would guess they would work as a type of span. My idea is that spans are a fairly general concept, and would also incorporate annotations such as warnings, not just syntax highlighting.

@trishume
Copy link
Collaborator

A variation, based on how Sublime works:

  • Use full hierarchical scope selectors like Sublime so that themes can do nice things like define more styles to do an extra good job of highlighting certain languages. But basic themes still only have to define a few colors.
  • Let plugins append styles/rules to the same set used by themes, so that the theme is preferred if it defines its own match with equivalent strength, but a plugin can override that by using a more specific match, but a theme can counter that in turn by including its own color for that specific plugin's use by including a specific selector.

@cmyr
Copy link
Member

cmyr commented May 15, 2017

I think this makes sense generally and also makes conflict resolution potentially easier, as scopes have priorities implied in their hierarchy, and certain scopes like invalid can also be given additional priority.

One side effect of this approach is that the concept of scopes & themes has to live in xi-core. This is a fairly big change, and is maybe an argument for moving syntax highlighting itself into core? It would definitely be convenient if we could reuse the scope/theme stuff already implemented in syntect, and if syntect is going to be a dependency keeping syntax highlighting in-process might make sense.

@trishume
Copy link
Collaborator

@cmyr an approach @raphlinus and I discussed a while ago is splitting syntect into two crates, one with the parsing and one with the theme application. Instead of submitting colour spans like they do now, parser plugins can just submit scope stack operations like the current parser outputs.

This is good because it maintains the property of having syntax parsing be a plugin, allowing fancier or faster parsing systems than syntect.

It's also good because now other plugins can access the analysis that parsing plugins provide in order to do things like "expand to scope" and "go to function" regardless of the parsing plugin by taking advantage of a common intermediate representation.

@cmyr
Copy link
Member

cmyr commented May 15, 2017

@trishume at first blush that sounds like a good idea to me.

@AljoschaMeyer
Copy link

One side effect of this approach is that the concept of scopes & themes has to live in xi-core.

Couldn't a plugin handle theming, scopes and syntax detection? I don't see why the core should be opiniated about this stuff. Is the performance gain significant here?

Aside: Obviously, this would require better inter-plugin communication. Which would be a lot of effort to implement. I'm feeling somewhat bad for just throwing in these suggestions without contributing any code. But I'm very excited about what Xi could become, so I'm giving my idealistic comments anyways. And when pragmatic requirements outweight idealism, that's still fine. I'm thankful for all of you investing your time into the project - the progress you made in the last few weeks is amazing.

@cmyr
Copy link
Member

cmyr commented May 15, 2017

Your feedback is totally welcome, glad somebody is noticing we're getting some work done. (also glad, perhaps, that it isn't too many people... 😉)

If I'm understanding: the main thing that core needs to be opinionated about is ensuring UI consistency. In the simplest case, this means that plugins should not be sending specific color information.

There are other approaches, but to me they seem to add a bunch of complexity and not offer much upside, outside of some currently-very-abstract stuff like "improved flexibility".

Having color schemes live in core, and having plugins send references to styles that are resolved by lookups into a color scheme, feels like a good solution for now. Plugins are still responsible for scopes and syntax detection, just not for assigning colors to them.

@rkusa
Copy link
Contributor Author

rkusa commented May 15, 2017

Thanks for all the comments!

Other editors have a concept of markers for [...]

@AljoschaMeyer: Thanks for the link! Markers look similar to spans (but I only skimmed through the linked page). The concept of Validity of markers is interesting and maybe useful for spans, too. E.g. a search highlight could have a validity of inside, a search highlight touch, ...

Are you talking about the plugin <-> core protocol or about the front-end <-> core protocol? Might be good to clarify that. For the front-end <-> core protocol I think sending the colors as RGBA value is not a problem.

@eyelash: I was actually thinking about providing the same approach for both plugin <-> core and front-end <-> core. I agree that we could keep the current approach for front-end <-> core. Would this require functionality to cleanup the frontend style cache? E.g. when switching between themes, the front-end might end up with a lot of unused styles.

One side effect of this approach is that the concept of scopes & themes has to live in xi-core.

@cmyr Maybe not a requirement? I am just thinking out loud now. I think it is not necessary that scopes live in the core. The core could just maintains a [string: Style] map, without restricting the list of possible scopes. It is then up to the plugin landscape to establish a common set of scopes. This might of course have some disadvantage. E.g. that plugins that do not work well together because the plugin landscape cannot agree on certain set of scopes. (1)

Instead of submitting colour spans like they do now, parser plugins can just submit scope stack operations like the current parser outputs.

@trishume Sounds good.

It's also good because now other plugins can access the analysis that parsing plugins provide in order to do things like "expand to scope" and "go to function" regardless of the parsing plugin by taking advantage of a common intermediate representation.

This requires to attach some semantic to a scope. In this case my thinking above (1) would not work, because there scope names are just "stupid" strings.

Couldn't a plugin handle theming, scopes and syntax detection? I don't see why the core should be opiniated about this stuff.

@AljoschaMeyer Your opinion and feedback is welcome! I'd also still like to see syntax detection as a plugin. Unless proven to have a poor performance. Because I still like the idea to have an AST based syntax highlighting for certain languages (besides, as already discussed on some other issues, the challenges that are attached with this approached).

Plugins are still responsible for scopes and syntax detection, just not for assigning colors to them.

@cmyr Could summary :)

@trishume
Copy link
Collaborator

@rkusa

This requires to attach some semantic to a scope. In this case my thinking above (1) would not work, because there scope names are just "stupid" strings.

The core could just maintains a [string: Style] map, without restricting the list of possible scopes. It is then up to the plugin landscape to establish a common set of scopes. This might of course have some disadvantage. E.g. that plugins that do not work well together because the plugin landscape cannot agree on certain set of scopes.

Sublime has a solution to this problem. First, it has a document describing standardized scope names. This lets syntax authors and theme authors coordinate. It also makes the scopes useful for plugins to interpret.

And since scopes are actually a stack, things like "expand to scope" can work quite well, it just keeps expanding up through hierarchical constructs (brackets, block, function, module) as you continue pressing the key binding. Similarly, the scope names for marking function definitions are standardized so "Go to function" can work, and there's a config file to add things like module definitions if you want to add things to it.

Also you mention a [string: Style] map, but that would be very limiting, the whole point of scopes is hierarchical matching. You need a selector matching engine for that, but there's one in syntect that could be split into a separate crate, along with a special scope data structure that always takes at most 16 bytes to represent a scope and can check if one scope is a prefix of another in a few machine instructions. It pools strings in memory using Servo's atom library so that all the scope strings don't waste tons of memory.

As for if this should be done in plugins instead of the core. I think of scopes more as a coordination mechanism than a piece of functionality. They don't actually do much on their own, I think they're an excellent candidate for inclusion in the core.

I don't think they should be used for the front end as well though, it would be sad if every front end had to unnecessarily implement their own theme engine to apply colours. It makes much more sense to do it in the core and send colours/styles in some (perhaps semi-compressed form) to the front end.

@raphlinus
Copy link
Member

I'm in favor of scope->style resolution being a separate crate which is included in the xi core, for the arguments @trishume has outlined.

I'm finding that for any piece of functionality, there are tradeoffs from doing it in the front-end, the core, or plugins. For scope->style resolution in particular, I think the core is the right place. It could be done in front-ends, but then every front-end would need to duplicate the logic. It shouldn't be done in the same plugin as implementing the syntax, because that would make it a lot harder to use multiple syntax highlighting plugins. It could be done in a separate plugin, but it's hard to see the advantage - you need to have a consistent mechanism for selecting a theme, etc., so it's very hard to see what benefit there is to swapping it out. Also, for certain things (like selecting a background color) you really want it to be fast so there isn't any visible flickering.

So count me in favor of that approach.

@cmyr
Copy link
Member

cmyr commented May 16, 2017

Okay, I think we're in broad agreement. I'd like to roadmap this, since it is pretty tightly coupled with plugin support. Some Qs:

  1. @trishume should I open an issue in syntect about splitting the crates? Would you like help with this work?
  2. @raphlinus how does this fit in with any future plans for xi-lang? If xi-lang is imagined as a future replacement, it might be worth thinking about api compatibility.
  3. I asume for the time being we will still be relying on syntect for syntax-specific scoping. Do we want to expose that scoping to other consumers (plugins, e.g.) or do we just want to use it for coloring?
  4. Where would rope science 11 fit into this? Would that be implemented as a new-style syntect plugin?
  5. What else am I not thinking of?

@trishume
Copy link
Collaborator

@cmyr

  1. Sure. I'm unlikely to work on it myself because I'm already doing 8 hours of Xi per day, but it shouldn't be very difficult at all. Also because I'd have to go through Google's (admittedly not too hard) open source patching process if I wanted to work on syntect right now.
  2. Theoretically any parser should be able to output scopes
  3. Eventually it would be nice to expose it to plugins, but that's not urgent.
  4. It would probably be part of the syntect plugin, or a Rust crate that could be used in the syntect plugin and the xi-lang plugin.

@cmyr
Copy link
Member

cmyr commented May 25, 2017

Okay, so I guess we're moving on this: trishume/syntect#70 is merged, which means that the highlighting + scopes components of syntect are now available without having to use the parsing module.

This necessarily involves some architectural changes. My initial thoughts:

Editor gets a Spans<Scope> member. Plugins send Scope spans instead of style spans.
View takes scopes and a theme and generates a Spans<Style>, which tracks the actual colors as displayed.

There's a new Theme global, probably passed around via DocumentCtx.

This gets us most of the way there. Some things I'm not sure about:

  • how do we resolve scopes coming from multiple sources? An example would be making sure we show an error span related to a lint, and don't overwrite that style when we update syntax highlighting.

  • what is the mechanism for defining additional scopes? Is there a mechanism for defining a custom style, associated with a scope?

@trishume
Copy link
Collaborator

@cmyr:

One thing to note is that Sublime/Textmate/Atom/etc don't actually store one scope per character, but each span actually has a scope stack. syntect outputs these as a series of push and pop operations so as to minimize space usage.

There's a couple ways Xi could do this:

  • Store only one scope per character. Have worse support for cool themeing than Textmate-like editors, but better than other editors that don't even use hierarchical scopes.
  • Be like Atom and store multiple scopes, but not in order. This has most but not all of the themeing power of an ordered stack of scopes. Xi could do this with overlapping spans, if that's a thing the Spans model can support.
  • Be like Sublime and have a full ordered scope stacks. Xi could do this either with overlapping spans, if there is some way to ensure order, at least to some extent. Or alternatively we could add a new system that might be useful for other things, that is like spans but just one point, and use that to store the pushes and pops that syntect outputs.

Re first question:

Sublime solves this by having highlights from plugins be totally separate from syntax highlighting, but I'm not sure if we should take that approach. It avoids conflicts, brokenness and weirdness, but it limits possible plugins like highlighting identifiers in rainbow colours, or based on type.

One easy solution is just to join the scope stacks of various plugins in some order, and then theme selector matching precedence logic will determine which wins.

Re second queston:

Plugins should be able to submit lists of (selector, style) pairs, these are then concatenated in some order based on some kind of plugin precedence, then used with syntect to match against scope stacks. Selector matching precedence logic then solves most interference concerns.

@raphlinus
Copy link
Member

Quick thoughts.

I think Spans<Scope> is conceptually sound, but I like the id registration mechanism that's currently used for core to front-end communication. I'd like to see the same thing for plugins. This is primarily an optimization, but I think can result in nice separation of concerns in the code. Also, as @trishume points out, it's really a stack of scopes (ie I prefer the Sublime-like approach).

It sounds like your question about defining additional scopes boils down to whether themes can be extended through some other mechanism. There are generally lots of scopes that don't get styles.

There's more to say but I have limited time right now.

@trishume
Copy link
Collaborator

I talked to @raphlinus about this IRL, and his idea (which I agree with) is to represent this in syntect as Spans<ScopeStackId> where ScopeStackId is just a u32 or something and the plugin can send RPCs to register these to scope stacks which it sends in JSON as just list of strings like ["source.rust", "meta.block", "string.quoted"] and it could then use that number in later spans it sent.

One way to do this, and that ties in well with how we are talking about #318 is that scope stack IDs are unique per layer, and are allocated in order so that lookup is in a Vec and the plugin can start using it without waiting for an ID to be returned. The only way to get rid of them would be to throw away the entire layer, which should be fine, otherwise there would need to be a garbage collector.

Then the theme engine would apply syntect's selector matching logic on each of these scope stacks to find a style for them.

This can be made to slow down tremendously for pathological files (e.g 10000 open braces) but since syntax highlighting is a plugin this won't do anything other than make highlighting slow for that file. The representation of pushes pops and scopes that syntect uses handles that case fine, but it's really not necessary.

@cmyr
Copy link
Member

cmyr commented May 25, 2017

Okay, to clarify:

  1. Plugins will generate Spans of ScopeStacks, represented for convenience/expedience/performance by some value type.

  2. Each plugin which generates these will have them assigned to a 'layer'. (Exactly one plugin per layer).

  3. A layer will consist of an interval tree (Spans<T>) where T is some value type, plus a lookup table for mapping T -> ScopeStack.

  4. Styles will be resolved through some join operation on all of the extent intervals in all layers for a given document range. Algo TBD; first pass will probably just be some arbitrary 'priority' system.

An invariant I am assuming: within a layer, spans will not overlap, e.g. a span represents the complete ScopeStack for that source at that point. Any issues?

@trishume
Copy link
Collaborator

Yup, that's all what I'm thinking. Including spans not overlapping, except perhaps between layers where you'll have to concatenate scope stacks or something before doing theme resolving.

@cmyr
Copy link
Member

cmyr commented May 26, 2017

@trishume: Okay, one thing that's bugging me right now:

In the plugin, I'm going to have a ScopeStack that is composed of 128 bit values. I'm going to convert this to a u32, which I'm going to send to the client, where it will be... converted back into a string? and then... reconverted back into the 128 bit representation? I guess most of these conversions aren't conversions, but borrows from a lookup table, so this maybe isn't that bad? It just feels strange to be moving between three representations.

@trishume
Copy link
Collaborator

Yah it is. But I don't think it is that bad:

  • Syntect plugin maintains a hash table or tree structure to map stacks to u32. You can use trees or nested hashes to efficiently go from syntect's pushes and pops to a stack id.
  • If it doesn't find it, it submits an id allocation request to the core and then adds it to the data structure. This allocation request requires serializing the ScopeStack/Vec<Scope> to a JSON string list.
  • The core, upon receiving an allocation request parses the JSON string list into a syntect ScopeStack/Vec<Scope> and stores it in an internal lookup table.
  • When doing theme matching, for each stack in the table the core uses syntect to resolve it to a style with the theme, and stores these styles in a separate table.
  • When outputting spans to the frontend, the core sends (or synchronizes somehow) the style table to the front end, and then passes the spans of u32 through exactly as is, but serialized over JSON.

@trishume
Copy link
Collaborator

trishume commented May 29, 2017

So I modified the synstats example in syntect to count unique scope stacks and total operation and span amounts, and have some interesting new data.

Basically the amount of unique scope stacks in Rust and JS files (two syntaxes that use meta scopes for blocks and so generate more unique scope stacks than other syntaxes).

The data below was generated with this branch of syntect, look there to see what the numbers mean.

But basically sending all the scope stack ops in a binary encoding would take about 1mb for jQuery, but sending the span stack ids and the unique stacks as strings in a fairly compact JSON encoding would take 3.3mb for the stacks and another 1mb for the spans with stack ids. So about 4x as much data.

I haven't yet benchmarked the overhead for doing highlighting calculations on unique spans and the overhead of the data structures to encode stacks as unique spans.

Not sure if this information changes anything, haven't thought about it enough yet.

Edit: one thing to note is that the unique stacks only need to be sent really once per file. The stack repo can probably be kept around and won't need to be added to much on large files, so it's a cost to be paid on first highlight.

Another thing I noticed is that this cost could be dramatically reduced because most new stacks in the stack repository will be additions to a previous stack, so there could be a message representation like "add new stack starting with id 2524 and adding meta.block.for.js". Only as an easy bolt-on optimization if we find that things are slow.

$ cargo run --example synstats testdata/parser.rs
################## Files ###################
testdata/parser.rs

################## Stats ###################
File count:                                1
Total characters:                     243222

Function count:                          226
Type count (structs, enums, classes):     11

Code lines (traditional SLOC):          5020
Total lines (w/ comments & blanks):     6197
Comment lines (comment but no code):     630
Blank lines (lines-blank-comment):       547

Lines with a documentation comment:      272
Total words written in doc comments:    1537
Total words written in all comments:    3323
Characters of comment:                 26802

Unique stacks:                          2158
Total length of unique stacks          22622
Total stack length on wire            464218
Total stack in memory                 361952

Total ops sent                         84880
Total pushes sent                      42113
Total spans sent                       52565
Average push len                      3.6243440267850784
Total op bytes sent naive             886989
Total op bytes sent                   560558
Total span bytes sent                 473085
$ cargo run --example synstats testdata/jquery.js
################## Files ###################
testdata/jquery.js

################## Stats ###################
File count:                                1
Total characters:                     247597

Function count:                          368
Type count (structs, enums, classes):      0

Code lines (traditional SLOC):          6171
Total lines (w/ comments & blanks):     9210
Comment lines (comment but no code):    1491
Blank lines (lines-blank-comment):      1548

Lines with a documentation comment:       90
Total words written in doc comments:     289
Total words written in all comments:    8509
Characters of comment:                 68785

Unique stacks:                         12753
Total length of unique stacks         169938
Total stack length on wire            3379165
Total stack in memory                 2719008

Total ops sent                        128556
Total pushes sent                      61304
Total spans sent                       82765
Average push len                      3.814971290617252
Total op bytes sent naive             1311176
Total op bytes sent                   859362
Total span bytes sent                 744885

cmyr added a commit to cmyr/xi-editor that referenced this issue May 30, 2017
This is progress towards xi-editor#284 / xi-editor#319. Instead of explicitly sending
style information (e.g. actual colors) plugins now send scope
information, which is resolved into colors in core.

This is incomplete: importantly it has dropped the ball on the important
question of how to define custom styles, and how to merge scope
definitions from multiple plugins.
@cmyr
Copy link
Member

cmyr commented May 30, 2017

Okay, so there are two major outstanding issues, once #329 lands.

Span merging

Span merging involves coalescing multiple layers' spans into a single tree, resolving conflicts. The algo is fairly simple: working with two layers at a time, you merge each overlapping section of spans into a new span.

layer one:           _________
layer two:      ---------
merged:         ------..._____

or:

layer one:    0000110222234444
layer two:             55555  
merged:       0000110226678844

My inclination is to add this as a new operation on Spans<T>. I can think of two approaches: the first is some function merge<T: Mergeable>(&self, other: &Self) -> Self, and the other would be some function merge<T, O>(&self, other: &Self, f: Fn(T::L, T::L) -> O) -> Spans<O>. That is to say: either the merge operation should require that T is some type which implements merging logic, or else it takes a closure that produces a Span of some new type.

I'm leaning towards the first approach, combined with makingStyle merge-aware. This feels right because We would like to be able to set priorities on styles anyway.

@raphlinus any possible problems with this approach?

Style definitions

One of the major impetuses for this work was to allow the declaration of custom styles. This is the area I've thought about the least, in terms of how style defs should fit into the existing theme system; I'll give this some thought a bit later.

@cmyr
Copy link
Member

cmyr commented Jun 3, 2017

@trishume okay another quick thought/question about stack merging / conflict resolution.

Let's say we're the syntect plugin, in a rust file. For some span, we send ["source.rust", "meta.impl.rust", "meta.block.rust", "meta.function.rust", "variable.language.rust"]. Also running is some rustc backed plugin. For the given span, it is sending (say) ["invalid.error", "meta.rustc.error.e0507"] (or something; but some stack that should resolve to an error).

Basically I'm wondering if you think that in this case we merge the stacks, or if we resolve to a style for each stack and then merge those?

In the first case, is there some conflict-free way of determining the stack-merge order? In the second case, since we couldn't trust the theme to correctly privilege the styling associated with the error, we would have to associate some sort of priority to that style; this would have to be out of band, e.g. sent alongside that scope when that scope was defined by the plugin? (I think).

Let me know if this doesn't make any sense.

@trishume
Copy link
Collaborator

trishume commented Jun 3, 2017

There's two avenues. Both of them need each layer to have a "priority" to determine which to favour when there is a conflict.

  1. Merge styles. Re-do the syntect scope stack highlighting function to return a StyleModifier instead of a Style. Then apply those over the default base style in increasing order of priority. This is the most efficient, clean and simple to implement.
  2. Merge stacks. Concatenate stacks in increasing priority order. The only clean way I can think of doing this is for the merge process to go directly to Style after concatenating everything rather than to stack ids, since stack ids would require a new global namespace of all layers and that might grow quite a bit and be rather redundant. The most common case will be only one stack for a given span, and that will just look up the pre-resolved style like it does now, and if things are concatenated, it'll call syntect on the concatenated stack and return the Style. This design is slower and more complex but gives the power to do combine knowledge from different plugins in highlighting decisions, like highlighting an error (from linter's stack) in a string (the scope for which is in syntect's stack) differently than an error elsewhere. I can't really think of any cases where this would be useful or nice, but I haven't thought about it enough to rule out it maybe being nice.

@trishume
Copy link
Collaborator

trishume commented Jun 3, 2017

There's two ways I can think of for doing priority:

  1. Have plugins give themselves a scope_priority in their config. Publish a document with some conventional priority ranges like 200-300: syntax highlighting, 400-500: semantic code annotation, 600-700: linters.
  2. Have users order their plugins in a list and use that as a priority. More work for the user but more likely to be correct.

@cmyr
Copy link
Member

cmyr commented Jun 13, 2017

So I’ve been thinking about this again:

I think there’s some conflation of things going on here, or at least I’ve been conflating some things.

I don’t think that the types of highlighting that is presented by a linter is a style in quite the same way that syntax highlighting is a style. That is: syntax highlighting is a special case of style.

Something that comes in from a linter could (and probably would, should) have a presentation style that is distinct from the styles available to syntax highlighting: for instance a colored wavy underline in ST.

This is also distinct from (at least some) other forms of annotations; for instance if some identifier has documentation available on hover, we might represent that in a distinct group of spans.

Anyway: I’m trying to zoom out a bit and think about exactly what a style is, and exactly that we’re merging. Do we represent a lint as a distinct style, and the frontend is responsible for drawing it alongside syntax highlighting, or do we define a new style for each highlighting+lint combination?

Do we allow multiple syntax highlighting plugins to be active in a given buffer? Plugins that provide scope information could be a distinct type, and we could choose to run only one of them using some rule of precedence.

In any case, it is unclear to me that the problem of merging styles in the current theme system is a big problem, because most of the conflicting styles provided by various plugins seem (to me) to be styles of separate types. The question is whether and how we want to merge these.

@trishume
Copy link
Collaborator

@cmyr yes this is a real concern, and there are two approaches that I know of in existing editors. Personally I'm fine with either, it's a tradeoff where you can sacrifice some power for the benefit of plugins not interfering with each other as much (which is a real benefit Sublime has over things like Emacs).

The Sublime Approach

Syntax highlighting is a privileged type of styling that uses a different mechanism than linters and plugins. Syntax highlighting determines the color, background and font style of the text. Plugins can then use view.add_regions to add layers of "regions" on top. These regions can do things like put a wavy underline, highlight box, or outline around text, but can't change its color or font style. Regions are still colored based on a scope, but only a single scope and not a stack.

Pros:

  • Never have to worry about plugins conflicting with highlighting.
  • Avoids ever messing up and trying to syntax highlight the same text with two different plugins.
  • Makes it illegal for plugin authors to do syntax highlighting in Python, which absent a well thought out incremental highlighting protocol is a recipe for terrible slowness.

Emacs Method

In Emacs plugins can do any highlighting they want and there's no super-clear distinction between a syntax highlighter and something like a linter.

Pros

  • Somewhat conceptually simpler, one concept/API instead of two.
  • Enables a few use cases that Sublime doesn't support well, they are nice, but you can easily argue these use cases aren't very important and Sublime gets along fine without them:

@cmyr
Copy link
Member

cmyr commented Jun 17, 2017

Okay, I've done some more thinking on this, and I have an approach that I'm mostly okay with. This is an amalgam of the two approaches @trishume outlines above.

Proposal: two style APIs

I propose that there be two distinct APIs through which plugins can modify text styles. One would be the current scope-based system, and would be the general approach for syntax highlighting; the second would use named identifiers, and would be preferred for special cases, and would have access to richer style definitions. Merging styles from both of these sources would be possible.

syntect styles & xi styles

The style information provided by syntect (& supported by thTheme files) is limited to foreground & background colour, font-weight, and bools for italics & underline.

We imagine that ultimately xi will support a superset of these, including things like overline, strikethrough, css-style border attributes, or possibly even fancier things like drop shadows & corner radius.

xi-styles will be the working way of describing text style in xi-core. syntect-styles will be convertible to xi-styles. xi-styles will have a priority property, and will be mergeable with one another; all attributes on xi-style is optional, and the style sent to the client is generated by merging xi-styles and then applying them to some default style.

syntax highlighting styles

Plugins which are providing syntax highlighting will operate as they currently do. This API hasn't been thoroughly described yet, but basically the plugin defines scopes, which it sends to core, and then sends span+scope groups to describe the styling for various text regions.

(for more info on scopes see the sublime text docs.)

When xi-core receives these scopes, it resolves them into a syntect-style (technically a syntect StyleModifier, which is a style where all properties are optional) and then this is converted to a xi-style.

custom styles

A plugin which wishes to define custom styles skips this intermediate step. Instead it sends an add_styles rpc, with params { "styles": [xi-style] }. These styles are unique for each layer. (Which means, currently, unique for each plugin). It is an error for add_styles and add_spans to be called with the same layer argument.

These styles are then assigned to spans through the same RPC as is currently used with scopes.

drawbacks

  • API: It's possibly confusing to reuse the same RPC method for two related but distinct pieces of functionality; another option would be to have a distinct method for setting explicit styles, and have custom styles referenced by string identifiers.

  • Working with themes: this strategy does not cooperate with themes. There is no way for a theme to provide an override for a custom style; but then syntect themes do not support the various fancy attributes that xi-style provides, so there are limitations in what we can do without augmenting the theme format.

Also (cc @trishume) A limitation of our current implementation is that scopes are defined in a per-plugin/per-buffer basis, so two plugins sending scopes to the same buffer don't know about each other or each other's scopes. This is a structural reality of using an index based API. It's unclear to me whether or not this is a serious limitation or not.

but if we were augmenting the theme format...

One possibility for this would be to allow theme authors to define certain named colours, e.g. errorColor, warningColor, and maybe even theme-appropriate values for blue/lightBlue etcetera; these could be used in custom style declarations, which would then fill in the values as appropriate for a given theme.

resolution

In xi-core, all styles would be represented as xi-styles; when resolving styles from various sources they would be merged, using priority to handle conflicts. These final styles would then be sent to the client.

This mechanism gets us more or less where we want to be.

If anyone thinks this is a bad idea, let me know, because I'm going to start implementing it. 😏


bonus round: Styles vs Annotations

or: how should a linter actually work?

In a lot of this thinking about merging styles, our example has been merging styles from something like a linter with styles from syntax highlighting.

A linter is more than just a style; it's an annotation. Does a linting plugin send two different types of spans: styles and annotations? Or does it send a single annotation span that maps to a style and has associated metadata?

@trishume
Copy link
Collaborator

@cmyr that sounds good, the only thing I might add to that proposal is that like Sublime, you can make the custom styles fit with the theme by mandating that the color be specified as a scope. So for example linters typically use the invalid scope as the color for a wavy underline, which is normally some kind of red.

That way the plugin decides style attributes that don't really depend on the theme (is it a border? fill? underline? squiggly underline?) and the theme decides the color. If themes want to specify a certain color (dunno why they would?) we can have a convention for themes like color.blue.light.

I don't think the separate scope ids per layer is a limitation. It allows the syntax highlighting plugin to not have to wait for the core to tell it an Id for the stack/style it just sent before using it. If it didn't do that the round trip waiting would just kill performance. Sharing between layers of the same buffer wouldn't be much of a win since normally there will only be one syntax highlighting plugin, and if there are multiple they'll probably be using different scopes. Sharing between buffers of the same file type (or all buffers) would save memory, but I'm not sure how to do it without losing the no-round-trip property, and I'm not sure how big a win it would be. Per layer should be fine for the foreseeable future.

@cmyr
Copy link
Member

cmyr commented Jun 17, 2017

Okay, so then the only funny part (to me) is this: if we use scopes for custom styles, how do we decide when to use the theme's colors and when to use the custom style's?

@trishume
Copy link
Collaborator

@cmyr the custom style's color is based on the theme. Possibly just as the recommended way of setting the color, possibly as the only way to prevent people from making theme-incompatible plugins.

So the serialization of a xi-style when adding a custom one should be something like {"underline_style": "squiggle", "color": "invalid.illegal.syntax.js"} and then when Xi turns that into its internal XiStyle struct it uses syntect's Highlighter::get_style with only the one scope with the current theme to get a color, and then stores that in the XiStyle struct.

At some point it will probably be a good idea to extend syntect and Xi to be able to add/remove themes in an ordered list of precedence so for example if color.blue.light isn't found in the user's theme it looks it up in the default Xi theme. Or if a plugin actually wants exact colors (say for highlighting CSS hex color literals) it can add theme rules as necessary like colors.hex.34Af62 or possibly we can just provide a discouraged direct color-specifying api.

@cmyr
Copy link
Member

cmyr commented Jun 17, 2017

@trishume I guess my main hangup here is that in the general case a scope is resolved to a style, i.e. fg+bg+text_styles, and it feels a bit strange to override this, and resolve scopes in some contexts to just their fg_color value. In any case this does feel like even sublime's use of fg_color for applying colour to regions is hacky, and the default scopes aren't really designed with lints/errors/warnings in mind.

In any case I'm not really implementing region stuff right now, so I'm happy to punt on some of the details and take some more time to mull it over. I appreciate all of your input, you've obviously thought about this problem a bit. 😎

I will have a PR shortly that finishes off this preliminary style work. At some point this week I'd appreciate if you could get a syntect release out with my last patch, but maybe hold off a few days and make sure I don't have any other needs.

@trishume
Copy link
Collaborator

@cmyr My entire thinking process is to look at what Sublime does and think about why they made those choices, and they are often good reasons.

In Sublime's case using the fg_color to color regions works because there are very few cases that occur in real use:

  • I want to outline/underline something: give a very specific non-standard scope like outline.find.matches. It will get matched to the default text color, which is fine since it is an outline, but themes can override it if they want, either specific outlines or outlines in general.
  • This thing is wrong: give it either an outline or a squiggly underline with a scope like invalid.illegal.lint.warning.js and it will normally get highlighted in red, unless the theme wants to do something like color warnings differently.
  • I want to outline/underline these N types of things with distinctive colors: Just give them N different irrelevant scopes because you don't really care about semantic meaning but want colors that fit with the theme, so things like keyword.myplugin, string.myplugin. For example this is how rainbow-identifiers/parens could work, but you'd probably want to add specific selectors to your theme to make it look excellent, but I don't think there's a way to avoid that.
  • I'm a merge conflict resolving plugin and I want to highlight sections as red, blue and green: Use scopes like color.red... which you add rules for or there are rules for in a default fallback theme, but importantly users can override this in their theme file. This was a problem I had in Emacs where I had to switch themes when resolving merge conflicts because the colors were hard-coded and had no contrast with my default theme.
  • I'm that plugin that highlights hex color literals with the color they represent: Dynamically create a theme rules for the colors you need, or use an API for exact colors. The way the Sublime one works is one big static theme file with tons of rules that it merges into your them. It doesn't have every hex color but it rounds to the nearest one it has.

lord pushed a commit to lord/xi-editor that referenced this issue Oct 31, 2018
Delay hiding of statusbar to avoid flicker
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants