-
Notifications
You must be signed in to change notification settings - Fork 285
WIP 2746 hack tables into existing doc structure #2830
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
base: main
Are you sure you want to change the base?
WIP 2746 hack tables into existing doc structure #2830
Conversation
… ImeNodeSerialization for CompositeNode. Now selection is not crashing.
…Working on deletion..
…tructure # Conflicts: # super_editor/lib/src/core/editor.dart # super_editor/test/super_editor/supereditor_components_test.dart
… by double/triple click
|
Update:
While working on the above, I found that some built-in logic isn’t obviously applicable to Examples:
I see three possible approaches to handle this:
I think the third approach is probably a way to go, then during the implementation I can try move logic into @matthew-carroll what do you think about this? |
…chy. Updated all NodeDocumentChange to use `NodePath` instead of `nodeId`. Start working on upstream/downstream deletion inside CompositeNode
|
Update Keep working on this PR. Marked it Draft, as it's not yet ready. Changes:
As for my previous question:
I decided to go with second approach, and just defined 2 methods in bool canInsertChildAfter(String nodeId) {
return true;
}
bool canDeleteChild(String nodeId) {
final nodeToDelete = getChildByNodeId(nodeId);
if (nodeToDelete == null || !nodeToDelete.isDeletable) {
return false;
}
// If CompositeNode is not deletable, at least one child must be inside
if (!isDeletable && children.length == 1) {
return false;
}
return true;
}that way CompositeNode can decide on default behavior. |
6de4ace to
4bbd126
Compare
…d, so it works within CompositeNode
4bbd126 to
a84cfbd
Compare
…`getLeafNodes` method.
…previous selectors)
|
Thanks for working on this. I'm not sure when I'll have a chance to dig into your changes, and the review might be quite involved when we get there, but just letting you know that I'm aware of this PR. Also, FYI, there may have been local changes that I had not yet pushed to my PR for this. I haven't checked. So be prepared for significant conflicts if that ends up being the case. |
|
@matthew-carroll thank for feedback. Just an update and my plan going forward: I was keep working on this until now, and I think I'm about to finish most of it today/tomorrow. Most APIs are done (like @override
Iterable<(NodePath, DocumentNode)> getLeafNodes({bool? reversed, NodePath? since}) sync* {
final iterator = _LeafNodeIterator(this, sincePath: since, reversed: reversed);
while (iterator.moveNext()) {
yield iterator.current;
}
}Now I'm just keep reworking existing code to be compatible with Regarding unpublished changes, what kind of changes are you referring to? Maybe they are not related to my changes. I'd like to coordinate on plans to avoid wasted effort from any side. At least I'd like to agree on core api. With the changes I'm about to release, I think we would be able to implement proper table and similar components. The only future improvements I have in mind are:
But I'm not sure if I should work on these points too, as it is time consuming and more changes to review/merge. I think we can do that with subsequent PRs, as public API won't be changed there, just internal improvements. Would you like to discuss any of these topics? |
For my PR, I was writing bits of implementation and then pushing them. It's possible that I've got a bunch of commits locally that I never pushed. I haven't touched it in a while, so I don't know if I do, and if I do, I don't know which changes those are.
Given the volume of changes you've already made, you might as well continue. The existing work will already take a major effort to review. Please make sure to write thorough tests along the way. You can use our many thousands of existing tests to demonstrate the variety of what should be tested, and how to test it. A meaningful review can't take place until we have corresponding tests, so better to do that sooner than later. Also, tests tend to reveal missing features. Lastly, with regard to follow up PRs, we pretty much need to tackle everything in one PR. Anything we put off until later might show a fundamental flaw in the approach. Therefore, we need to update every |
|
After some thinking, it looks like I can revert lots of changes I did and go back to (mostly) original API, where we always address nodes by id. I'll just create a mapping Also, to make all |
|
Update: New approach works really well. I reverted a lot of changes I did before. What's the main idea, and key points:
In result, after doing these 3 points, everything seems to work fine without any changes to any Next:
|
Hello @matthew-carroll and thanks for the wonderful library (the code is really good)!
I've tried to implement an editable table on my own, but I found very quickly that it's impossible to do without modifying a core components of the library.
So, after few trials I found this draft PR #2765 and it looks like a proper way to go.
Indeed, we need a
CompositeNodethat would allow us overcome "single column layout" limitation.So I decided to continue your PR. This is still work-in-progress, but what I did there is:
ColumnDocumentComponentso it works with real children node ids.CompositeNodeViewModel, so it propagates selection/selectionColor to it's children view models.Demo:
Screen.Recording.2025-11-12.at.04.11.13.mov
Notes about implementation:
CompositeNodeeasily and avoid rewriting existing logic, we can keep all theseis TextNodechecks, we just need to get leaf nodes byDocumentPositioninstead of root nodes bynodeIdwhere it matters.TextInsertionEventneeds to be migrated to useDocumentPositioninstead ofint offset(seeNodeTextInsertionEventsuggested implementation). The plan is to continue and migrate other events to be position-based.The goal of that PR is to avoid conflicts going forward and get some feedback (Like is there any chance to contribute?). If it makes sense, I can continue with that task in that branch. Thank you!