fix: fix next setNodeMarkup setp rebase fail when pre step is a setNodeMarkup step

Akimotorakiyu commented

2022-05-04 19:52:15 +02:00

(Migrated from github.com)

When setNodeMarkup Step just change the attributes or marks of the node, it will not change the structure, so that time the return value of getMap should be an empty StepMap.

marijnh commented

2022-05-06 10:15:11 +02:00

(Migrated from github.com)

Changing a node's attributes should count as having that node's token changed. Other code relies on that.

Also I don't think adding data to steps without including it in their JSON structure is going to work, and changing the JSON structure at this point is not an option.

Changing a node's attributes _should_ count as having that node's token changed. Other code relies on that. Also I don't think adding data to steps without including it in their JSON structure is going to work, and changing the JSON structure at this point is not an option.

Akimotorakiyu commented

2022-05-06 13:27:03 +02:00

(Migrated from github.com)

Also I don't think adding data to steps without including it in their JSON structure is going to work, and changing the JSON structure at this point is not an option.

@marijnh I forgot to add that data to JSON structure, I will fix that.
fixed

> Also I don't think adding data to steps without including it in their JSON structure is going to work, and changing the JSON structure at this point is not an option. - [x] @marijnh I forgot to add that data to JSON structure, I will fix that. - [x] fixed

Akimotorakiyu commented

2022-05-06 13:48:44 +02:00

(Migrated from github.com)

Changing a node's attributes should count as having that node's token changed. Other code relies on that.

@marijnh Do you have any other solutions or ideas for this rebase failed issue? (Even for online co-editing, this failure is unexpected.)

> Changing a node's attributes should count as having that node's token changed. Other code relies on that. @marijnh Do you have any other solutions or ideas for this rebase failed issue? (Even for online co-editing, this failure is unexpected.)

marijnh commented

2022-05-06 16:53:24 +02:00

(Migrated from github.com)

I don't know what "the rebase failed" issue is.

Also, no, we can't change the JSON structure of Steps anymore -- people have that in their databases.

I don't know what "the rebase failed" issue is. Also, no, we can't change the JSON structure of Steps anymore -- people have that in their databases.

Akimotorakiyu commented

2022-05-06 19:19:29 +02:00

(Migrated from github.com)

I don't know what "the rebase failed" issue is.

🌰 example:

A Heading Node has two attrs - level and align.

// SchemaSpec
const heading = {
  attrs: {
    level: {
      default:"1"
    },
    align: {
      default:"left"
    }
  }
  // ...
}

There are two user are co-editing: userA and userB

first, they are sync with server, they get the same doc:

---------------------------------------------------------------------
I'm heading{ level: 1,  align:'left'}
---------------------------------------------------------------------

second, at the same time

userA change align from 'left' to 'right' - AStep
userB change the level from '1' to '2' - BStep

third, the step which first sync to server will be save, and another will rebase on the saved setp and rebase fail( the map result mapped is null )

// prosemirror-collab
export function rebaseSteps(steps, over, transform) {
    // ....
    for (let i = 0, mapFrom = steps.length; i < steps.length; i++) {
        //  here is null
        let mapped = steps[i].step.map(transform.mapping.slice(mapFrom));
        mapFrom--;
        if (mapped && !transform.maybeStep(mapped).failed) {
            // ...
         }
    }
    return result;
}

@marijnh Do you have any other solutions or ideas for this rebase failed issue?
Expected: Both of those steps are saved!

> I don't know what "the rebase failed" issue is. 🌰 example: A `Heading Node` has two attrs - level and align. ```typescript // SchemaSpec const heading = { attrs: { level: { default:"1" }, align: { default:"left" } } // ... } ``` There are two user are co-editing: userA and userB first, they are sync with server, they get the same doc: ```text --------------------------------------------------------------------- I'm heading{ level: 1, align:'left'} --------------------------------------------------------------------- ``` second, at the same time - userA change align from 'left' to 'right' - `AStep` - userB change the level from '1' to '2' - `BStep` third, the step which first sync to server will be save, and another will rebase on the saved setp and rebase fail( the map result `mapped` is null ) ```typescript // prosemirror-collab export function rebaseSteps(steps, over, transform) { // .... for (let i = 0, mapFrom = steps.length; i < steps.length; i++) { // here is null let mapped = steps[i].step.map(transform.mapping.slice(mapFrom)); mapFrom--; if (mapped && !transform.maybeStep(mapped).failed) { // ... } } return result; } ``` @marijnh Do you have any other solutions or ideas for this rebase failed issue? **Expected: Both of those steps are saved!**

marijnh commented

2022-05-09 07:53:14 +02:00

(Migrated from github.com)

Do you have any other solutions or ideas for this rebase failed issue?

This is how the merging works, intentionally. With your patch, both steps will apply, but only the last one will take effect, so the result is arguably worse (the first one to commit gets overwritten), so that doesn't seem like an improvement.

> Do you have any other solutions or ideas for this rebase failed issue? This is how the merging works, intentionally. With your patch, both steps will apply, but only the last one will take effect, so the result is arguably worse (the first one to commit gets overwritten), so that doesn't seem like an improvement.

Akimotorakiyu commented

2022-05-09 17:37:25 +02:00

(Migrated from github.com)

With your patch, both steps will apply, but only the last one will take effect, so the result is arguably worse (the first one to commit gets overwritten), so that doesn't seem like an improvement.

If with my patch, there is a chance to get more info and do more when rebase.

Else, may need to implement one or two SetAttrsStep for our aim.

> With your patch, both steps will apply, but only the last one will take effect, so the result is arguably worse (the first one to commit gets overwritten), so that doesn't seem like an improvement. If with my patch, there is a chance to get more info and do more when rebase. Else, may need to implement one or two `SetAttrsStep` for our aim.

marijnh commented

2022-05-09 18:39:49 +02:00

(Migrated from github.com)

You're going to have to find another way to solve this. This patch is not going to be acceptable.

Pull request closed

This pull request cannot be reopened because the branch was deleted.

Rows
Columns

fix: fix next setNodeMarkup setp rebase fail when pre step is a setNodeMarkup step #21

Pull request closed