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

execute undo on VDOM #239

Merged
merged 6 commits into from
Mar 29, 2022
Merged

execute undo on VDOM #239

merged 6 commits into from
Mar 29, 2022

Conversation

nvdk
Copy link
Member

@nvdk nvdk commented Mar 23, 2022

while reviewing #236 I noticed the undo handler is not preventing default browser behaviour at the moment, this fixes that.
edit: Turns out fixing that involved porting this to work on the VDOM, so updated the title to reflect that
NEEDS EXTENSIVE TESTING

@nvdk nvdk added the bug Something isn't working label Mar 23, 2022
@nvdk nvdk requested a review from abeforgit March 23, 2022 20:20
@nvdk
Copy link
Member Author

nvdk commented Mar 23, 2022

looks like cursor handling is broken now

@nvdk nvdk marked this pull request as draft March 23, 2022 20:36
@nvdk nvdk force-pushed the bugfix/undo-handler branch from 4cfefb6 to 5ec6546 Compare March 23, 2022 20:52
@nvdk
Copy link
Member Author

nvdk commented Mar 23, 2022

Turned out it was easier to get this working on the VDOM than getting the old version working. Still some (minor?) issues to work out, looks like I'm missing a few edits in history still at the moment.

@@ -4,8 +4,7 @@ import { parseXmlSiblings } from '@lblod/ember-rdfa-editor/model/util/xml-utils'
import Model from '@lblod/ember-rdfa-editor/model/model';
import { logExecute } from '@lblod/ember-rdfa-editor/utils/logging-utils';
import ModelRange from '@lblod/ember-rdfa-editor/model/model-range';
import ModelNode from '../model/model-node';
import ModelElement from '../model/model-element';
import setNodeAndChildDirty from '@lblod/ember-rdfa-editor/model/util/set-node-and-child-dirty';
Copy link
Member Author

Choose a reason for hiding this comment

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

moved to a helper, because it turns out restoring a snapshot is also a special case

if (writeBack) {
this.write();
this.write(this.rootModelNode, false);
this._selection = snapshot.modelSelection.clone(this.rootModelNode);
Copy link
Member Author

Choose a reason for hiding this comment

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

didn't seem to work without this clone, though I don't really get why...

@nvdk nvdk changed the title undo handler should not allow default browser action execute undo on VDOM Mar 23, 2022
@nvdk nvdk added enhancement New feature or request and removed bug Something isn't working labels Mar 23, 2022
@nvdk nvdk marked this pull request as ready for review March 24, 2022 07:25
@nvdk nvdk requested a review from lagartoverde March 24, 2022 07:26
@lagartoverde
Copy link
Contributor

Demo.undo720.mov

So weird things happening with the undo, when I remove selecting a portion of the text I cannot undo it, and also when I'm undoing I can't undo the first character I deleted

@lagartoverde
Copy link
Contributor

The problem with undoing the first action also happens when inserting

@nvdk
Copy link
Member Author

nvdk commented Mar 24, 2022

Think I resolved that now @lagartoverde, I was creating a snapshot after a change was applied but I should have been creating it before as we do now

Copy link
Member

@abeforgit abeforgit left a comment

Choose a reason for hiding this comment

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

Works well, some issues with non-collapsed selections:

  • select range of text
  • backspace
  • undo
    resulting selection is somewhere unexpected
    Not really blocking though

EDIT: on development branch, selection always ends up at the start of the document, so it's not really a regression

@nvdk nvdk merged commit 8bbcf3a into development Mar 29, 2022
@nvdk nvdk deleted the bugfix/undo-handler branch March 29, 2022 07:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants