-
Notifications
You must be signed in to change notification settings - Fork 42
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
[WIP] Combine spans #68
base: feature/demo-syntax
Are you sure you want to change the base?
Conversation
Nested Markdown syntax finally looks like I was hoping. I realized that with the proper implementation of I broke the insertion when trying to fix the deletion, so that needs more thought. |
Ok, I fixed the insertion by fixing The semantics of layering of styles over intervals, seem to have both this segmenting/chopping structure I tried to implement in |
Hey @jmatsushita! Sorry I've been so terribly unresponsive, I've been in Thailand taking a holiday, but I'll be in Berlin in a month and can talk about all this over a beer haha. I still don't have my laptop handy so it'll be a while still before I can give this a proper try, but if you reach a point on this where you feel confident then go ahead and merge, I gave you commit access on purpose 😄 Hope you're doing well! The markdown highlighting looks great! |
Hi @ChrisPenner
So here's the work in progress with combineSpans, dealing with part of the consequences of the fix. Here's a few things worth noting:
range
lens ended up being off by one with the new combineSpans implementation[0,1[
really makes more sense to me, means that I'd prefer continuing by changing the interval to have an exclusive upper bound. I haven't done it yet, but I'd really like to convince you :)While tracing the render code I noticed that spans in one line are computed as deeply nested
HorizJoin
s because of the recursion inattrLine
, that was probably not intentional?with regards to combineSpans, I've used the IntervalMap package and the top level algorithm chops things in the following way:
<>
It really feels that the implementation could be improved massively. It's so far from being done in one pass but it feels like it should be possible. Also as we discussed, pushing the data inside the text data structure might be useful? I'm not so sure anymore, and intuitively still think, though don't understand nearly as much as I'd like to, that we're thinking of something that Kmett thought all the way through, including in how he packs his data in trees that somehow take advantage of the structure of the Dyck language which has a similar structure as our
Range
(and what he adds with relative deltas) and that this will somehow help when we'll want to do incremental parsing... But replacingYi.Rope
is way above my ability, but maybe not yours :)Also I think there's still something unclear for me about dealing with defaults and current styles within the Style Monoid or AttrMonoid... I guess the abstractions should be
resetting
the styles to get the defaults, and atransparent
style (or front, back colors and flairs) which keeps the current style (or front, back, flair), so it seems that we need a neutral element and and absorbing element?UPDATE: Looking at this again, and given this comment, I think the Monoid instance of Style is intended to be the neutral element and the Default instance to be the absorbing element. But with AttrMonoid, the Monoid instance is absorbing this should probably be consistent...
UDPDATE2: In which case we might want to use Zero instead of
Default
?Finally, there are still some things that seem broken, like hitting
G
in insert mode seems to make the cursor range blow up.Phew. That was a deep dive and much good learning :)
Jun