-
Notifications
You must be signed in to change notification settings - Fork 141
feat: support text_mode collapse recursive #558
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
Conversation
90da54f to
f2cbd25
Compare
f2cbd25 to
30b6718
Compare
|
That is impressive, this looks promising! 😎 The code looks really neat. Whilst testing I came across two issues though:
I think issue (1) is likely a small bug but (2) may be tricky. |
I will try to fix it |
|
Hi, I have completely rewritten the collapse logic. Previously, the foldAll function had issues. As stated in the official documentation:
This explains why folding doesn’t work reliably for large JSON files. To address this, I used ensureSyntaxTree to obtain a fully parsed syntax tree, then iterated through the nodes to collect foldable.from and foldable.to. Finally, I dispatched the folding ranges using: After testing, I found that folding works correctly for a 10k JSON! However, due to the large data size, dispatch the fold effects is quite time-consuming—it takes almost 10 seconds on my Mac M3 Pro. The time-consuming method in our code is getFoldRanges. 1K -> 100ms, 10k -> 720ms Additionally, I created a custom foldService (
My custom foldService replicates the logic of syntaxFolding, but uses ensureSyntaxTree instead of syntaxTree to guarantee a complete tree. This way, we can reliably retrieve from and to using foldable, and apply folding. The changes are quite substantial — let me know if anything looks off. |
|
I also submitted a PR to CodeMirror. If it gets merged, we’ll be able to remove our |
|
Impressive! The folding/unfolding works reliable now 🎉. The performance is indeed an issue. I think we can do two things (until maybe the performance improves some day).
I think option (2) would be nicest if possible, but it is also the most complex to implement. What do you think? |
I also agree with option 2. I will try to optimize the performance first, but if it doesn't work, I will use option 2 for optimization, though it may take some time. |
|
Thanks! |
8041707 to
f3023f7
Compare
|
That is awesome news! I'll review your latest update asap but that will probably be within one or two weeks from now. |
josdejong
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First: sorry for the delay in review, first I was on holiday and then I was sick 😅.
I've just tested this PR and it works like a charm, I'm really happy you managed to pull this off. Thanks for this awesome piece of work! The code is well taken care and easy to follow. I made only one small inline remark. Besides that I think this PR is ready to be merged.
I have a couple of ideas for further improvement, but it may be better to address that in separate PR's:
- Add a (recursive) "Collapse all" and "Expand all" button to the main menu, like in
treemode - Implement
ctrl+clickto recursively expand/collapse a node incodemode, just like it works intreemode - Change the expand/collapse methods to return a Promise so you know when the operation is actually finished. Also, either allow passing your own AbortController, or return the abort controller (alongside the promise?) so it is possible to programmatically cancel a recursive collapse too.
What do you think?
|
Thanks, awesome job!
Yes 😅. It was just the flu, nothing special though quite annoying.
👍 |
|
🎉 This PR is included in version 3.9.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |




To solve #557 issues
Add text_mode collapse recursive