Fix for the block-uncomment undo

[TomMalbran]

This should fix the need to do 2 undo to undo a block-uncomment. It just replaces the whole block-comment with the inside text and then recreates as much as possible the old selection.

[peterflynn]

@TomMalbran: did you try doing a general fix in Document.batchOperation()? That seems preferable since it would fix all commands at once without having to change all their implementations. And replacing the whole range isn't ideal, since it would blow away all bookmarks within the range (which for now means closing inline editors, but later could have other consequences too).

[TomMalbran]

No, I just did a replace to fix it, I thought it wouldn't be that bad and might been the easiest solution.

I couldn't get the cmv3 version to try with it either.

[peterflynn]

I think ideally we'd like to see a centralized, general patch like this:

    Document.prototype.batchOperation = function (doOperation) {
        this._ensureMasterEditor();

        var self = this;
        this._masterEditor._codeMirror.compoundChange(function () {
            self._masterEditor._codeMirror.operation(doOperation);
        });
    };

I did a quick test and this does indeed fix block-comment undo on master. But two things we should do before landing a change like that:

• Verify that it still works (and is still needed) on cmv3
• Do some testing of other editing features to make sure this doesn't break them (and run unit tests, of course)

Do you think you might have bandwidth to take on those tasks?

[TomMalbran]

I guess I could try to do it in a few days if nothing has been done about it yet.

After this is done, maybe I should change the comment part, so that it only adds the prefix and suffix and doesn't make a whole replace of the selected text?

[peterflynn]

Yes -- with the fix I suggested above, I think we won't need any changes to the existing (in master) block comment code for undo to start working properly.

[TomMalbran]

What I meant, is that the commenting part of block comment was already fixed in the first pull to work with undo doing only one replace of the text inside the selection and adding the prefix and suffix. Since replacing like this isn't a great idea, and it should work by only adding the prefix and suffix after the fix suggested above, I think it might be better to change that after the fix is implemented.

[peterflynn]

Oh, I see what you mean. Yeah, I agree -- changing comment to work like uncomment does seem like a good idea, once batchOperation() is fixed.

[TomMalbran]

Ok. I'll close this then and try to do the other fix. Thanks