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

Undo after API change is not correct #768

Closed
mmorearty opened this issue Jun 23, 2016 · 1 comment
Closed

Undo after API change is not correct #768

mmorearty opened this issue Jun 23, 2016 · 1 comment

Comments

@mmorearty
Copy link

mmorearty commented Jun 23, 2016

The undo manager has the ability to not record API-initiated changes on the undo stack. So when an API change comes in, it adjusts all existing entries on the undo and redo stacks to account for the API change. But in the scenario below, it seems to get it wrong.

Steps for Reproduction

Here is what the following test is doing:

  1. Start with this document: The lazy fox
  2. Modify it so that it starts with a link: https://www.google.com/ The lazy fox
  3. As an API-originated change, change the link so that its text is just "Google": Google The lazy fox
  4. Add an exclamation point: Google The lazy fox!
  5. Undo (should undo the exclamation point)
  6. Undo (should undo the insertion of the entire hyperlink, but it messes up)

The actual steps:

  1. Add the following test to test/unit/modules/history.js:

    it('transform api change 2', function() {
      let url = 'https://www.google.com/';
      this.quill.history.options.userOnly = true;
      this.quill.updateContents(new Delta().insert(url, { link: url }).insert(' '), Quill.sources.USER);
      this.quill.history.lastRecorded = 0;
      this.quill.updateContents(new Delta().delete(url.length).insert('Google', { link: url }), Quill.sources.API);
      this.quill.history.lastRecorded = 0;
      this.quill.updateContents(new Delta().retain(this.quill.getLength()-1).insert('!'), Quill.sources.USER);
      expect(this.quill.getContents()).toEqual(new Delta().insert('Google', { link: url }).insert(' The lazy fox!\n'));
      this.quill.history.undo();
      expect(this.quill.getContents()).toEqual(new Delta().insert('Google', { link: url }).insert(' The lazy fox\n'));
      this.quill.history.undo();
      expect(this.quill.getContents()).toEqual(new Delta().insert('The lazy fox\n'));
    });
    
  2. Run npm test

Expected behavior: It should pass. The final undo() should leave the document as just 'The lazy fox'

Actual behavior: After the final undo(), the document is: G The lazy fox

Platforms: Chrome 50.0.2661.102 (64-bit) on Mac 10.11.5

Version: tag v1.0.0-beta.6

@jhchen
Copy link
Member

jhchen commented Jun 27, 2016

Thank you for the detailed report. The issue is Quill tries to use the minimal number of changes, as measured by number of characters, and Google and https://www.google.com/ actually share a common "oogle" substring. The fix was to try to use the specified API change instead of always diffing.

Also the final correct state should actually be "Google The lazy fox!" since the linked Google text was added by source = api, the history module when set to userOnly should only undo changes made by source = user. This is most often used in a collaborative environment, for example when two users are working two different paragraphs. If I'm typing away on my paragraph and you are typing away on your paragraph, when I undo a bunch of times, only my paragraph should be affected. So if you inserted Google in your paragraph, me undoing should not remove it.

@jhchen jhchen closed this as completed in 885c9e8 Jul 7, 2016
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

No branches or pull requests

2 participants