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

[WIP] Combine spans #68

Open
wants to merge 25 commits into
base: feature/demo-syntax
Choose a base branch
from
Open

[WIP] Combine spans #68

wants to merge 25 commits into from

Conversation

jmatsushita
Copy link
Collaborator

@jmatsushita jmatsushita commented Dec 15, 2018

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:

  • The Default instance for Cursors used an exclusive interval
  • The range lens ended up being off by one with the new combineSpans implementation
  • The above, and the fact that thinking of Ranges as mathematical intervals [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 :)
  • As we already discussed a bit, I think that we should be able to make things more easy to reason about by separating the abstractions of ranges over text and screen coordinates more neatly. I think it's not an Iso by the way because of the clamping.

While tracing the render code I noticed that spans in one line are computed as deeply nested HorizJoins because of the recursion in attrLine, 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:

  • split all values in the map with the upper and lower bounds of inserted value
  • split inserted value with all the intervals in the intersecting map
  • unionWith with <>

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 replacing Yi.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 a transparent 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

@jmatsushita
Copy link
Collaborator Author

Nested Markdown syntax finally looks like I was hoping. I realized that with the proper implementation of mempty as a neutral element then I was layering neutral styles and I needed a "base" style with the def Style covering the whole text so that further layers have the default to return to. So the fact that def is also absorbing actually didn't have anything to do with the problem, it was just that there needed to be a def layer as a base style.

image

I broke the insertion when trying to fix the deletion, so that needs more thought.

@jmatsushita
Copy link
Collaborator Author

Ok, I fixed the insertion by fixing delete instead of breaking the range lens.

The semantics of layering of styles over intervals, seem to have both this segmenting/chopping structure I tried to implement in combineSpans as well as this monoidal structure with regards to how segments on the same intervals add up. Maybe the absorbing aspect is that if you "reset the style of a selection" then that would be equivalent to adding a top layer that has overriding def styles which absorbs all that's below. This might give rise to a possible optimization in combineSpans where we don't need to computer intermediary mappends if we know the last one overrides.

@ChrisPenner
Copy link
Owner

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants