Port the bracketmatching plugin #20

Closed
opened 2018-08-31 12:02:32 +02:00 by marijnh · 22 comments
marijnh commented 2018-08-31 12:02:32 +02:00 (Migrated from github.com)

Logic can stay largely the same. Will need access to token info, so this should probably wait for the highlighter integration to stabilize a bit.

Logic can stay largely the same. Will need access to token info, so this should probably wait for the highlighter integration to stabilize a bit.
marijnh commented 2018-09-24 15:33:44 +02:00 (Migrated from github.com)

Mostly done in cac0a01b56

Mostly done in cac0a01b562f
adrianheine commented 2018-09-27 10:41:20 +02:00 (Migrated from github.com)

I implemented the token type matching in 0450d9d945 in a hacky way. Do you want a direct way to access token info from the highlighter? Do you want to somehow make the highlighter accessible from the state?

I implemented the token type matching in 0450d9d945c9ca47fc05b13d8a46ff05b3f7d2b2 in a hacky way. Do you want a direct way to access token info from the highlighter? Do you want to somehow make the highlighter accessible from the state?
marijnh commented 2018-09-27 10:46:45 +02:00 (Migrated from github.com)

I think what we talked about last time was to make it possible to get token types from the state, and have the view component of the highlighter use that mechanism when highlighting. We want to somehow cache those results, so that things like bracket matching don't recompute stuff that the highlighter just computed, but that cache can be relatively small and simple.

I think what we talked about last time was to make it possible to get token types from the state, and have the view component of the highlighter use that mechanism when highlighting. We want to somehow cache those results, so that things like bracket matching don't recompute stuff that the highlighter just computed, but that cache can be relatively small and simple.
adrianheine commented 2018-09-27 11:00:12 +02:00 (Migrated from github.com)

I remember that discussion. Do you think we need another cache in addition to legacy-modes' StateCache? Getting token types from the state could use StateCache internally and extract the token type from the RangeDecoration.

I remember that discussion. Do you think we need another cache in addition to `legacy-modes`' `StateCache`? Getting token types from the state could use `StateCache` internally and extract the token type from the `RangeDecoration`.
marijnh commented 2018-09-27 11:24:52 +02:00 (Migrated from github.com)

Yes, I think it makes sense to separate caching the states from caching the tokens. I wouldn't store tokens in the format of decorations—those are just the materialization of the tokens given to the editor, and an awkward format for something like the bracket matcher to access. Maybe keep lines of tokens as arrays or something? We might have to revisit this if we ever get a new mode system off the ground and want to provide compatible interfaces, but for now it's okay to keep it simple.

Yes, I think it makes sense to separate caching the states from caching the tokens. I wouldn't store tokens in the format of decorations—those are just the materialization of the tokens given to the editor, and an awkward format for something like the bracket matcher to access. Maybe keep lines of tokens as arrays or something? We might have to revisit this if we ever get a new mode system off the ground and want to provide compatible interfaces, but for now it's okay to keep it simple.
adrianheine commented 2018-09-27 11:56:02 +02:00 (Migrated from github.com)

By »keep lines of tokens as array« do you mean for actual lines of the document?

I'll try to outline my understanding of your suggestion. There is a StateCache that stores mode states (S), keeps track of the frontier and provides:

interface StateCache {
 readonly frontier: number
 storeStates: (from, to, CachedState<S>[])
 getState: (EditorState, Mode<S>, pos) => S
 apply: (Transaction) => Self
}

That's used by TokenTypeCache that maybe provides

interface TokenTypeCache {
  typeAt: (pos) => string | null
  iterTokens: (from, to) => Iterator<{from, to, string}>
  apply: (Transaction) => Self
}

And decorating is just calling iterTokens for a range and mapping {from, to, string} into RangeDecorations with a one-entry cache (lastDecorations.

TokenTypeCache is easily accessible through the state.

By »keep lines of tokens as array« do you mean for actual lines of the document? I'll try to outline my understanding of your suggestion. There is a `StateCache` that stores mode states (`S`), keeps track of the frontier and provides: ```typescript interface StateCache { readonly frontier: number storeStates: (from, to, CachedState<S>[]) getState: (EditorState, Mode<S>, pos) => S apply: (Transaction) => Self } ``` That's used by `TokenTypeCache` that maybe provides ```typescript interface TokenTypeCache { typeAt: (pos) => string | null iterTokens: (from, to) => Iterator<{from, to, string}> apply: (Transaction) => Self } ``` And decorating is just calling `iterTokens` for a range and mapping `{from, to, string}` into `RangeDecoration`s with a one-entry cache (`lastDecorations`. `TokenTypeCache` is easily accessible through the state.
marijnh commented 2018-09-27 12:01:17 +02:00 (Migrated from github.com)

Yes, that sounds reasonable (though, since we're not depending on ES6, I don't think we should make Iterator part of our interface—that'll require polyfilling Symbol and generating big ugly code for for/of). Since the highlighter will be running the mode a line at a time, maybe just getting a full line as an array is workable.

You'll have to see what works as you implement it—I can't really tell yet, at this point.

Yes, that sounds reasonable (though, since we're not depending on ES6, I don't think we should make `Iterator` part of our interface—that'll require polyfilling `Symbol` and generating big ugly code for `for/of`). Since the highlighter will be running the mode a line at a time, maybe just getting a full line as an array is workable. You'll have to see what works as you implement it—I can't really tell yet, at this point.
adrianheine commented 2018-09-27 12:02:57 +02:00 (Migrated from github.com)

I didn't mean ES iterators, yes. I think we can work with lines as units for now, since our old modes work that way. What should the interface in EditorState look like? I suppose the token type provider has to register itself somehow?

I didn't mean ES iterators, yes. I think we can work with lines as units for now, since our old modes work that way. What should the interface in `EditorState` look like? I suppose the token type provider has to register itself somehow?
adrianheine commented 2018-09-28 10:21:23 +02:00 (Migrated from github.com)

I started work on separating the caches in 8e98a4e3e0b9d0bc6fe3a891f11fc73515e2c31b. TokenTypeCache doesn't actually cache yet, and I don't know what a good cache strategy would be here. I also didn't expose TokenTypeCache yet on EditorState.

I started work on separating the caches in 8e98a4e3e0b9d0bc6fe3a891f11fc73515e2c31b. `TokenTypeCache` doesn't actually cache yet, and I don't know what a good cache strategy would be here. I also didn't expose `TokenTypeCache` yet on `EditorState`.
adrianheine commented 2018-09-30 16:44:16 +02:00 (Migrated from github.com)

Adapted to the new line interface in c605247991c442745ce6086bf18ace7c72fc981a.

Adapted to the new line interface in c605247991c442745ce6086bf18ace7c72fc981a.
marijnh commented 2018-09-30 16:49:25 +02:00 (Migrated from github.com)

Nice! (Though I think you meant a7b35f6539)

Nice! (Though I think you meant a7b35f6539afdc)
adrianheine commented 2018-09-30 17:38:22 +02:00 (Migrated from github.com)

Nice, but I actually meant the tokenTypes branch :)

Nice, but I actually meant the tokenTypes branch :)
curran commented 2018-10-21 15:05:46 +02:00 (Migrated from github.com)

The implementation in
https://github.com/codemirror/codemirror.next/blob/master/matchbrackets/src/matchbrackets.ts
appears to be pretty complete. Any remaining work required for this issue?

Really excited to see this project happening, thank you for all your work!

The implementation in https://github.com/codemirror/codemirror.next/blob/master/matchbrackets/src/matchbrackets.ts appears to be pretty complete. Any remaining work required for this issue? Really excited to see this project happening, thank you for all your work!
adrianheine commented 2018-10-21 21:00:37 +02:00 (Migrated from github.com)

It's quite complete, but the way decorations are referenced is awkward. I started work to make decorations accessible on the editor state in https://github.com/codemirror/codemirror.next/tree/tokenTypes.

It's quite complete, but the way decorations are referenced is awkward. I started work to make decorations accessible on the editor state in https://github.com/codemirror/codemirror.next/tree/tokenTypes.
curran commented 2019-03-18 11:14:29 +01:00 (Migrated from github.com)

This issue feels closeable, as the bracket matching plugin seems pretty stable.

This issue feels closeable, as the bracket matching plugin seems pretty stable.
marijnh commented 2019-03-18 14:22:32 +01:00 (Migrated from github.com)

There's one open item — using tokenizer information when matching brackets, to avoid, for example, matching a bracket in a string or comment with one in the actual source language.

There's one open item — using tokenizer information when matching brackets, to avoid, for example, matching a bracket in a string or comment with one in the actual source language.
curran commented 2019-03-22 23:47:13 +01:00 (Migrated from github.com)

I'm noticing that the .codemirror-nonmatching-bracket class is not appearing on any bracket elements, even after this commit where the class was added github.com/codemirror/codemirror.next@b21b89713b

Here's a reproduction example, where you can see that the .codemirror-matching-bracket is activated, but not .codemirror-nonmatching-bracket. Expected behavior: all non-matching brackets get the class .codemirror-nonmatching-bracket.

I'm noticing that the `.codemirror-nonmatching-bracket` class is not appearing on any bracket elements, even after this commit where the class was added https://github.com/codemirror/codemirror.next/commit/b21b89713be4c8772c4f57d9ca3f8998e0a4f2dd Here's a [reproduction example](https://bl.ocks.org/curran/d8de41605fa68b627defa9906183b92f/b34bad0852405616db99dfecdc14f4b09ecb9be3), where you can see that the `.codemirror-matching-bracket` is activated, but not `.codemirror-nonmatching-bracket`. Expected behavior: all non-matching brackets get the class `.codemirror-nonmatching-bracket`.
marijnh commented 2019-03-23 08:59:26 +01:00 (Migrated from github.com)

Expected behavior: all non-matching brackets get the class .codemirror-nonmatching-bracket.

That's not how this plugin works—it only highlights the bracket at the cursor position.

> Expected behavior: all non-matching brackets get the class .codemirror-nonmatching-bracket. That's not how this plugin works—it only highlights the bracket at the cursor position.
curran commented 2019-03-24 14:38:16 +01:00 (Migrated from github.com)

Ah I see. It was my misunderstanding as to what should be expected.

In this case, it's not clear when .codemirror-nonmatching-bracket would be activated, if at all.

Ah I see. It was my misunderstanding as to what should be expected. In this case, it's not clear when `.codemirror-nonmatching-bracket` would be activated, if at all.
adrianheine commented 2019-03-24 18:30:09 +01:00 (Migrated from github.com)

In ([), if you're over one of the parentheses.

In `([)`, if you're over one of the parentheses.
curran commented 2019-03-25 01:32:56 +01:00 (Migrated from github.com)

Aha! I get it now. Thank you @adrianheine

Aha! I get it now. Thank you @adrianheine
marijnh commented 2019-08-23 14:09:45 +02:00 (Migrated from github.com)

Closing this now that the lezer branch has landed. It's not unlikely that there are some problems in the new code still, but we'll open specific separate issues as we run into them.

Closing this now that the lezer branch has landed. It's not unlikely that there are some problems in the new code still, but we'll open specific separate issues as we run into them.
Sign in to join this conversation.
No labels
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
codemirror/dev#20
No description provided.