Fixed integration with undo #54
Conversation
|
Redo is still funky, but undo works fine. TC:
|
|
What's more – it's completely impossible to redo to the full text (xxxyyyzzz) on this branch. But this is something we might live with for a while. |
|
@scofalik, if fixing redo is not feasible now, this PR LGTM in terms of behaviour. |
src/inlineautoformatediting.js
Outdated
| continue; | ||
| } | ||
| // Do nothing if selection is not collapsed. | ||
| if ( !selection.isCollapsed || !selection.focus || !selection.focus.parent ) { |
There was a problem hiding this comment.
I don't recall ever checking .focus not to say .focus.parent in selection checks anywhere in the code base. Are these needed? When do they matter?
src/inlineautoformatediting.js
Outdated
| // Use enqueueChange to create new batch to separate typing batch from the auto-format changes. | ||
| editor.model.enqueueChange( writer => { | ||
| const validRanges = editor.model.schema.getValidRanges( rangesToFormat, attributeKey ); | ||
| rangesToFormat.push( LiveRange.createFromParentsAndOffsets( |
There was a problem hiding this comment.
Do we need to use LiveRanges anymore? I understand that earlier we had to use them, cause the callback was added to (something like) applyOperation and there might be more operations incoming and the range could change. However, now, we fire callback after all changes are done, so the ranges should not change. Additionally, we are reversing the order to prevent messing up positions. Maybe LiveRanges are not needed anymore.
There was a problem hiding this comment.
Good call. It looks like live ranges are not needed anymore.
src/inlineautoformatediting.js
Outdated
| // Detach ranges used to apply Autoformat. Prevents memory leaks. #39 | ||
| rangesToFormat.forEach( range => range.detach() ); | ||
| // Reverse order to not mix the offsets while removing. | ||
| ranges.remove.slice().reverse().forEach( range => { |
There was a problem hiding this comment.
Why .slice() here? (I know this is not originally your code).
There was a problem hiding this comment.
This and above arrays transformation look alike. Maybe helper function?
There was a problem hiding this comment.
Why .slice() here? (I know this is not originally your code).
Probably to create a copy of the array since reverse() is working in place. I will try to simplify it and use a helper.
src/inlineautoformatediting.js
Outdated
| continue; | ||
| } | ||
| // Typing is represented by only a single change. | ||
| if ( changes.length != 1 || entry.type !== 'insert' || entry.name != '$text' ) { |
There was a problem hiding this comment.
Check if there was only one letter inserted (change size is 1).
src/inlineautoformatediting.js
Outdated
| // @param {module:engine/model/element~Element} block | ||
| // @param {Array.<Array>} arrays | ||
| function testOutputToRanges( block, arrays ) { | ||
| return arrays.map( array => { |
There was a problem hiding this comment.
Wouldn't it be easier to read, if .filter was first and then .map?
Suggested merge commit message (convention)
Fix: Fixed integration with undo. Closes ckeditor/ckeditor5#2397.