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

feat(edgeless): group #5069

Merged
merged 21 commits into from
Oct 25, 2023
Merged

feat(edgeless): group #5069

merged 21 commits into from
Oct 25, 2023

Conversation

regischen
Copy link
Contributor

@regischen regischen commented Oct 20, 2023

close #4999

notes

  1. group is designed as surface-element, and have children of Y.Map field.
  2. surfaceBlock add private groupMap property for tracking child to it's group. And provide getGroup and setGroup method.
  3. SelectionManager now has a _activeGroup property, which is to track current activated group for interaction
  4. groupRootId constant, virtual root group id.

Because Y.Map doesn't support Set value, so here using array
just make a update size test
100 shape elements

children type group patch size release one patch size total size
Y.Map 2338 11 49715
Array 1340 1248 48720

50 shape elements

children type group patch size release one patch size
Y.Map 1238 11
Array 740 618

10 shape Elements

children type group patch size release one patch size
Y.Map 358 11
Array 259 138

2 shape Elements

children type group patch size release one patch size
Y.Map 181 11
Array 164 42

we can see that when group size is becoming larger, the Group operation size of Y.Map is becoming larger. But it's updating patch size is constantly small.
However the updating patch size of Array is also becoming larger.
Besides if type is array, the searching would also be inefficient.
So maybe Y.Map is good choice?
@doodlewind

@vercel
Copy link

vercel bot commented Oct 20, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
blocksuite ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 25, 2023 4:58am
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
blocksuite-docs ⬜️ Ignored (Inspect) Visit Preview Oct 25, 2023 4:58am

Copy link
Member

@doodlewind doodlewind left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All EdgelessElement all have group property which is updated in runtime. for searching from child to parent.

This should not be persisted into CRDT.

@regischen
Copy link
Contributor Author

All EdgelessElement all have group property which is updated in runtime. for searching from child to parent.

This should not be persisted into CRDT.

of course

packages/blocks/src/note-block/note-model.ts Show resolved Hide resolved
@@ -202,12 +209,30 @@ export class EdgelessComponentToolbar extends WithDisposable(LitElement) {
);
}

private _getCreateGroupButton() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All these _getFoo template getters are not align with our best practice. The expected usage should be using pure functions like function CreateGroundButton(props) {} for wrapping these components. See the "Function Components
" part in lit docs.

packages/blocks/src/image-block/image-model.ts Outdated Show resolved Hide resolved
@regischen
Copy link
Contributor Author

regischen commented Oct 23, 2023

image

YMap value doesn't support Set type

@regischen regischen merged commit 81b6543 into master Oct 25, 2023
18 checks passed
@regischen regischen deleted the feat/edgeless-group branch October 25, 2023 05:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notable Major improvement worth emphasizing
Projects
Archived in project
Archived in project
Development

Successfully merging this pull request may close these issues.

Support group in edgeless
2 participants