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

Maybe a Bug in snapshots #82

Open
joacub opened this issue Nov 17, 2021 · 10 comments
Open

Maybe a Bug in snapshots #82

joacub opened this issue Nov 17, 2021 · 10 comments
Assignees
Labels
bug Something isn't working

Comments

@joacub
Copy link

joacub commented Nov 17, 2021

I don't know if is a bug or is the expected behavior, but when we applied a snapshot some attrs are gone, well I think is all attrs for deleted items, this line of code:

const attrs = typeMapGetAll(
      el
    );

    if (snapshot !== undefined) {
      if (!isVisible(/** @type {Y.Item} */ el._item, snapshot)) {
        attrs.ychange = computeYChange
          ? computeYChange('removed', /** @type {Y.Item} */ el._item.id)
          : { type: 'removed' };
      } else if (!isVisible(/** @type {Y.Item} */ el._item, prevSnapshot)) {
        attrs.ychange = computeYChange
          ? computeYChange('added', /** @type {Y.Item} */ el._item.id)
          : { type: 'added' };
      }
    }

we tested and if we get all attributes that are also deleted in that function everything works properly and all attributes are printed in the prose mirror state after applied the snapshot, this should take all attributes even if they are deleted when the snapshot is present, I don't know if behind of that behavior is another purpose.

thanks for your excellent work.

@joacub joacub added the bug Something isn't working label Nov 17, 2021
@TeemuKoivisto
Copy link
Contributor

I'm quite sure it's a missing feature that I too noticed a while back. I have had other things that have kept my busy so kinda forgot about it. Here's my reproduction https://github.com/TeemuKoivisto/pm-node-attrs-yjs-snapshots I assume it's the same thing

@joacub
Copy link
Author

joacub commented Nov 17, 2021

Yes read your post days ago, and the solution is in that function when the snapshots are available so you are comparing, skip the condition of the deleted records.

@dmonad
Copy link
Member

dmonad commented Nov 17, 2021

This code can be found in variations all over the codebase. So which function are you referring to specifically?

I don't know how to reproduce this issue. Can you please provide an example?

@joacub
Copy link
Author

joacub commented Nov 17, 2021

This code can be found in variations all over the codebase. So which function are you referring to specifically?

I don't know how to reproduce this issue. Can you please provide an example?

The function typeMapGetAll get the attributes of the elements but only the ones that are not deleted, so when you compare a snapshots there is a lot of deleted attributes that are not showing in the view. If still not understanding I will send you in a couple hours and example more specific. Thanks

@joacub
Copy link
Author

joacub commented Nov 17, 2021

@dmonad this is the code modified to get all the attributes when snapshots are in the state:

/**
 * @param {AbstractType<any>} parent
 * @return {Object<string,Object<string,any>|number|Array<any>|string|Uint8Array|AbstractType<any>|undefined>}
 *
 * @private
 * @function
 */
const typeMapGetAll = (parent, getDeletionsAlso = false) => {
  /**
   * @type {Object<string,any>}
   */
  const res = {};
  parent._map.forEach((value, key) => {
    if (!value.deleted) {
      res[key] = value.content.getContent()[value.length - 1];
    } else if (getDeletionsAlso) {
      res[key] = value.content.getContent()[value.length - 1];
    }
  });
  return res;
};
const attrs = typeMapGetAll(
      el,
      (Boolean(snapshot) || Boolean(prevSnapshot) || Boolean(computeYChange))
    );

@dmonad
Copy link
Member

dmonad commented Nov 17, 2021

This code is incorrect and doesn't yield the desired results. Snapshots allow you to revert to a specific point in time. This simply returns all deleted attributes. Please help me to reproduce this issue. I'm happy about fixes as well, but we need to understand the problem first.

@joacub
Copy link
Author

joacub commented Nov 17, 2021

To reproduce the issue just put an atributte in the paragraph and delete the paragraph, when you try to restore the snapshot that paragraph has any more the attribute cause delete items are not getting in that function the attributes.

@TeemuKoivisto
Copy link
Contributor

Would you @joacub be able to write it to a codesandbox? I forked the one I used for my own library to include yjs plugins: https://codesandbox.io/s/thirsty-sunset-cwvyt

Maybe something similar could be included in the issue template @dmonad ?

@dmonad
Copy link
Member

dmonad commented Nov 18, 2021

This issue currently doesn't have a high priority for me. In 80% of cases, I try to reproduce an error for 30 minutes and then find out that everything works correctly (maybe this is just some kind of misunderstanding.. ). If you make a PR with a test case covering your scenario (e.g. like this one:

export const testEmptyNotSync = tc => {
) I will be able to quickly fix the issue and we will never have to deal with it again because we have a test-case for it.

@joacub
Copy link
Author

joacub commented Feb 9, 2022

you can test my version in webmediums.com, we have integrate finally all already working pretty good, the version works really well with some bugs but this bugs are in our side when sometimes rendering the version, is something that happens sometimes and not very importan for now.

You can test writing something in any post, all comments are collaborative and with version too, I will be writing a article soon explaining all.

thanks to yjs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants