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

CM6: Restore collapser #679

Closed
Tracked by #659
fcollonval opened this issue Sep 18, 2023 · 4 comments · Fixed by #701
Closed
Tracked by #659

CM6: Restore collapser #679

fcollonval opened this issue Sep 18, 2023 · 4 comments · Fixed by #701
Assignees

Comments

@fcollonval
Copy link
Collaborator

This is a follow-up of #673 in which the collapser of unchanged code was removed (as not ported from CM5 to CM6).

Some solution could probably be extrapolated from https://github.com/codemirror/merge

@krassowski
Copy link
Member

https://github.com/codemirror/merge appears to be using Decoration.replace which was also recommended recently in a related topic. Is there a reason for not depending on https://github.com/codemirror/merge directly? The decoration widgets are modular so possibly we could just import them (possibly with tiny upstream adjustments).

@fcollonval
Copy link
Collaborator Author

fcollonval commented Sep 18, 2023

Is there a reason for not depending on https://github.com/codemirror/merge directly?

Not from me 😉 - I have not tried using its collapser directly.

The only trouble with the default codemirror code is usually the data structure that is not compatible with the one defines by nbdime. So to be tested for the collapser I would say.

@JohanMabille
Copy link
Member

JohanMabille commented Sep 20, 2023

Is there a reason for not depending on https://github.com/codemirror/merge directly?

As @fcollonval said, nbdime defines its own data structure for diff chunks, not compatible with the one in codemirror/merge. Besides, codemirror/merge provides a 2 ways view only, while nbdime also provides 3 and 4 ways views, which require extra padding for alignment, so I don't see how we could reuse the classes defined in codemirror/merge.

Regarding the decorations defined in codemirror/merge, they seem to be private to me (but I might be wrong here, I'm definitely not a TypeScript expert).

Therefore I don't really see how we could benefit from depending on codemirror/merge (but again I might have missed things)

@krassowski
Copy link
Member

Regarding the decorations defined in codemirror/merge, they seem to be private to me

You are right, this is (among others) what I meant by "tiny upstream adjustments" ;)

Thank you for explaining the rationale in more detail. I would still consider it worthwhile trying to contribute some of the things upstream and then reusing theme here, but of course this has an overhead and we may prefer to have a custom implementation for time being.

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 a pull request may close this issue.

3 participants