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

Add unique ids for each block #1333

Closed

Conversation

christoph-kluge
Copy link

@christoph-kluge christoph-kluge commented Sep 25, 2020

I'm adressing #873 with this PR.

Tested Features:

  • Existing data: If your previous block does not have an "id" property, it will create a new unique id
  • Existing data: If your previous block does have an "id" property, it will keep the existing unique id
  • Existing data: If a block gets changed the "id" property stays the same
  • Insert New Block: It will create a new unique id
  • Copy & Paste blocks: It will create a new unique ids per copied block
  • Move Blocks (Up/Down): It will keep the existing unique id

Important Note:

  • If you "transform/transition" an existing block to another block it will receive a NEW unique id

@christoph-kluge christoph-kluge changed the title Add unique ids on next Add unique ids for each block Sep 25, 2020
@dsaralaya
Copy link

I need this feature pls merge

@khaydarov khaydarov changed the base branch from next to next-rebased October 14, 2020 09:02
@khaydarov khaydarov changed the base branch from next-rebased to master October 14, 2020 11:59
@christoph-kluge christoph-kluge changed the base branch from master to next October 16, 2020 16:28
@christoph-kluge
Copy link
Author

Updated this PR and fixed conflicts.

Is there any update about unique block ids? How do you want to proceed with this issue/feature?

@sis-dk
Copy link
Contributor

sis-dk commented Oct 19, 2020

Does this handle creating a new id when copying a block and pasting it as a new block? Initialise empty editor, type a para block, CMD+A (selects all the blocks. In our case the para block), CMD+V. Would the newly pasted block have a new id?

@christoph-kluge
Copy link
Author

@sis-dk thanks for spotting the copy & paste issue. I have adressed this in my latest commit

@mqtik
Copy link

mqtik commented Oct 31, 2020

Why don't use shortid for id generation?
It's what I'm about to use in a module I'm working on (native side), but this one uses shortid to generate ID's.

Just want to keep consistent.

@christoph-kluge
Copy link
Author

I have used this feature on a custom build since 2.15.2 with UUIDs. Now I wanted to update for latest >2.18+ version and simply ported this feature with UUIDs. I think we could also add some custom configuration/callback to generate the IDs in a different way. Since One editor will not have quadrillions of entries I could imageine that shortids are pretty valid too 👍

Maybe the maintainers (@neSpecc, @khaydarov, @gohabereg) could give a statement about this. I'm fine to add some custom configuration/callback to support custom-ids instead of hardcoded uuids.

const savedData = await Promise.all(
this.selectedBlocks.map((block) => {
block.id = _.generateUuidv4();
block.save();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
block.save();
return block.save();

const savedData is void[] currently.

Copy link
Contributor

Choose a reason for hiding this comment

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

So copy paste functions via this.Editor.Paste.MIME_TYPE is not worked.

const savedData = await Promise.all(this.selectedBlocks.map((block) => block.save()));
const savedData = await Promise.all(
this.selectedBlocks.map((block) => {
block.id = _.generateUuidv4();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
block.id = _.generateUuidv4();
const savedData = await Promise.all(this.selectedBlocks.map(async (block) => ({
...await block.save(),
id: _.generateUuidv4()
})));

block.id = _.generateUuidv4(); may be unsafe because of object mutation.

block.id = _.generateUuidv4();
block.save();
})
);
Copy link
Contributor

Choose a reason for hiding this comment

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

This file changes may be not needed because IDs should be regenerated in paste function.

@@ -772,6 +772,7 @@ export default class Paste extends Module {
}

BlockManager.insert({
id,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
id,

Copy link
Contributor

Choose a reason for hiding this comment

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

The IDs of pasted data via this.Editor.Paste.MIME_TYPE should be regenerated in paste function.
When copy as this.Editor.Paste.MIME_TYPE one time and pasted multiple times, The IDs are duplicated.

/**
* Unique Id of the block
*/
id: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
id: string;
id?: string;

By allowing undefined value, This PR become UNBREAKING Change.

@@ -63,12 +65,14 @@ export default class Renderer extends Module {
*/
public async insertBlock(item: OutputBlockData): Promise<void> {
const { Tools, BlockManager } = this.Editor;
const id = item.id || _.generateUuidv4();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const id = item.id || _.generateUuidv4();
const id = item.id;

When const id is undefined, a new ID is generated in BlockManager.insert().

@ChasLui
Copy link

ChasLui commented Dec 16, 2020

I need this feature pls merge @christoph-kluge 🙏

@neSpecc
Copy link
Member

neSpecc commented Sep 13, 2021

Resolved by #1667

@neSpecc neSpecc closed this Sep 13, 2021
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.

7 participants