Undo/Redo should coalesce edits #300

Closed
failurite opened this Issue Jun 14, 2011 · 14 comments

Comments

Projects
None yet
10 participants

Most IDE's (VS, IntelliJ, NetBeans, Eclipse) combine key typing events together until some other event occurs such as a paste, tab, newline, etc.

Currently ACE adds an undo event for each character typed or deleted. It would be good if it did a better job of combining some undo's together.
@gissues:{"order":69.56521739130449,"status":"backlog"}

Contributor

AndreasMadsen commented Jul 8, 2011

This would be really nice +1

fjakobs was assigned Jul 23, 2011

Fabian, I have implemented this behavior in our version of Ace (from January).

Hopefully this helps...

/**
 * This class is intended to be used in place of the
 * ace/lib/ace/undomanager.js.
 * This version will combine single character inserts or
 * removals per line into one edit that can be undone or redone.
 */
dojo.declare("Texteditor.ace.UndoManager", null, {

    /**
     * Creates the UndoManager with an empty undo and redo stack.
     */
    constructor: function () {
        this.$undoStack = [];
        this.$redoStack = [];
    },

    /**
     * Takes in an array of delta's that should be added to the undo stack.
     * If other deltas exist on the undo stack we determine if
     * these can be merged and if so combine the two arrays.
     * @param options array of deltas to add to undo stack.
     */
    execute: function (options) {
        var oldDeltas;
        var newDeltas = options.args[0];

        //If we have an undo stack pop off the top and see if it should be merged.
        if (this.$undoStack.length > 0) {
            oldDeltas = this.$undoStack.pop();
        }

        this.$doc = options.args[1];

        if (oldDeltas && this._canMerge(oldDeltas, newDeltas)) {
            //Push the merged deltas onto the stack.
            this.$undoStack.push(oldDeltas.concat(newDeltas));
        } else {
            //Only push last deltas back on if there is something to push.
            if (oldDeltas) {
                //Push last deltas back on the stack.
                this.$undoStack.push(oldDeltas);
            }

            //Push the current deltas on the stack with no merge.
            this.$undoStack.push(newDeltas);
        }
    },

    /**
     * Perform an undo, this has the effect of informing the document to
     * do the last deltas added to the array and it pushes that set of
     * delta's onto the redo stack.
     */
    undo: function () {
        var deltas = this.$undoStack.pop();
        if (deltas) {
            this.$doc.undoChanges(deltas);
            this.$redoStack.push(deltas);
        }
    },

    /**
     * Perform a redo, this has the effect of informing the document
     * to do a redo with the last set of deltas on the redo stack and
     * pushes those deltas onto the undo stack.
     */
    redo: function () {
        var deltas = this.$redoStack.pop();
        if (deltas) {
            this.$doc.redoChanges(deltas);
            this.$undoStack.push(deltas);
        }
    },

    /**
     * Looks at two sets of deltas and determines if the last delta in
     * the old set and the first delta in the new set are compatible to
     * be merged together.
     *
     * Being compatible means...
     * <UL>
     *     <LI>Their actions are the same</LI>
     *     <LI>They both are eligible to be merged on their own</LI>
     *     <LI>They are on the same row</LI>
     *     <LI>They are directly next to each other (no cursor move)</LI>
     * </UL>
     * @param oldDeltas The old set of deltas to check
     * @param newDeltas the new set of deltas to check
     * @return true if these two deltas can be merged together
     */
    _canMerge: function (oldDeltas, newDeltas) {
        //There may be many new or old deltas but we are seeing if we can merge
        // the last old delta with the first new delta to determine if it is ok
        // to merge the two lists.
        var newDelta = newDeltas[0];
        var oldDelta = oldDeltas[oldDeltas.length - 1];

        var oldRange = oldDelta.range;
        var newRange = newDelta.range;

        //If one or both are not mergable, we can't merge
        if (!this._checkMergable(oldDelta) || !this._checkMergable(newDelta)) {
            return false;
        }

        //If we don't have similar actions we can't merge
        if (oldDelta.action !== newDelta.action) {
            return false;
        }


        //If we are on different rows we can't merge.
        if (oldRange.start.row !== newRange.start.row) {
            return false;
        }

        //If the ranges don't connect (new follows the old for an insert,
        // old follows new for a remove), we can't merge
        if (oldDelta.action === "insertText") {
            if (oldRange.end.column !== newRange.start.column) {
                return false;
            }
        } else { //else must be removeText
            //If this is a remove text we have two scenarios, delete and backspace
            //If it is a backspace the newRange end column should equal oldRange start column.
            //If it is a delete the end and start column should be the same for old and new.
            if (!this._isDelete(oldRange, newRange) && !this._isBackspace(oldRange, newRange)) {
                return false;
            }
        }

        //Everything looks good, lets merge!
        return true;
    },

    /**
     * Checks if two change ranges represent a backspace.
     * For this to be true the start column of the old
     * change and the end column of the new change must be the same.
     * @return true if the specified ranges indicate that this was a backspace action
     */
    _isBackspace: function (oldRange, newRange) {
        //TODO: Should we just have a backspace event type?
        return newRange.end.column === oldRange.start.column;
    },

    /**
     * Checks if two change ranges represent a delete.
     * For this to be true the ranges must be the same.
     * @return true if the specified ranges indicate that this was a delete action
     */
    _isDelete: function (oldRange, newRange) {
        //TODO: Should we just have a delete event type?
        //Start and end of old range should equal start and end of new range.
        return oldRange.start.column === newRange.start.column &&
            oldRange.end.column === newRange.end.column;
    },

    /**
     * Checks if a specific delta can be merged.
     *
     * The rules for merging are...
     * <UL>
     *    <LI>If the action type is insertText or removeText
     *    <LI>If it is not a new line
     *    <LI>If it is 1 character long, or it is all spaces
     *    <LI>If this change is all on one row
     * </UL>
     * @return true if the specified delta is able to be merged.
     */
    _checkMergable: function (delta) {
        //TODO: add new events for cut, paste, tab so that they will not be merged.
        var action = delta.action;
        var start = delta.range.start;
        var end = delta.range.end;
        var text = delta.text;

        //If not an insert or remove, no merge
        if (!(action === "insertText" || action === "removeText")) {
            return false;
        }

        var isOnlySpaces = dojo.trim(text).length === 0;
        //If we have more then one character and it isn't all spaces, no merge
        //TODO: This check can be removed once we distinguish key typed versus other events.
        if (text.length > 1 && !isOnlySpaces) {
            return false;
        }

        //If the change spans a row, no merge
        if (start.row !== end.row) {
            return false;
        }

        //Otherwise we have either a single character that isn't a
        // newline or we have a bunch of spaces..either way merge.
        return true;
    }
});
@ghost

ghost commented Nov 18, 2011

I agree, this would be a great feature!

Contributor

danyaPostfactum commented Dec 18, 2012

Is it hard to implement ?

Contributor

gjtorikian commented Dec 18, 2012

@danyaPostfactum No, in fact, Stypi has implemented such behavior for Ace already (they use it on their site). There's some discussion about pulling their code into core.

Meanwhile, though, this issue is a dupe of #541

gjtorikian closed this Dec 18, 2012

Contributor

adamjimenez commented Dec 18, 2012

Please re-open. #541 (which is closed) is a dupe of #300 and not the other way around.

gjtorikian reopened this Dec 18, 2012

Contributor

gjtorikian commented Dec 18, 2012

Technically, 300 came before 541--but I just noticed 300 was closed 4 months ago, so okay.

Contributor

adamjimenez commented Dec 18, 2012

I think you mean 541 was closed 4 months ago. This issue is 300, thanks for re-opening.

Contributor

gjtorikian commented Dec 18, 2012

Oh, whoops! Wow, I'm really not paying attention this morning...

Contributor

danyaPostfactum commented Jan 30, 2013

Any updates here?

Contributor

gjtorikian commented Jan 31, 2013

I just emailed one of the Stypi devs who had originally contacted us about a fix for this. Hope to hear from him soon.

Chris78 commented May 28, 2013

+1 for this issue

nightwing closed this Jul 12, 2013

So... why did this get re-closed? This seems to still be the case, and has to be one of its most annoying issues as a long-time vim user.

Member

nightwing commented Dec 15, 2016

@friedmag this have been fixed long time ago, and there is an option controlling the merging behavior https://github.com/ajaxorg/ace/blob/v1.2.6/lib/ace/editor.js#L2642-L2645.
If it doesn't work the way you expect, please create a new issue with detailed description of what you do and what expect to happen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment