Refactor document delta handling for v1.2 #1819
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Refactor delta handling code to: - Combine the "insertText" and "insertLines" delta types into a single "insert" delta type - Combine the "removeText" and "removeLines" delta types into a single "remove" delta type - Make all document mutations in a single applyDelta function. - Add basic delta validation (more needed . . . see TODOs) - Rework anchor logic to handle new delta types (also simplified) - Rename "insert()" to "insertText()" and "remove()" to "removeText()" - Rename "insertLines()" to "insertFullLines()" and "removeLines()" to "removeFullLines()" See related issue for more information. All tests are passing and the changes appear functional under preliminary testing, but careful review and testing will be necessary.
This seeks to keep the public API in-tact while improving method names within ace by keeping the old methods as wrappers around the new better-named methods. For example, document.insert() now simply calls document.insertText() and warns the caller via a console.log() that they are using a deprecated method. I've also updated the coding style of my changes (where I noticed discrepancies) to match the rest of Ace.
Both were introduced in 2e6f127.
console.warn makes better sense than console.log and matches similar warnings in ace (see gutter.js for example).
2e6f127 slowed down the application of deltas that only affect a single line. The slow-down, though trivial for a single line, is significant for operations than separately modify thousands of rows (such as indenting a large document). This commit speeds up single-line deltas by avoiding unnecessary calls to splitLine() and joinLineWithNext().
This makes it possible to break out helper functions without exposing them to the rest of the document class. Also, long term, we may want to have a stand-alone test suite for applyDelta, so it makes sense in its own file. All other changes involve syntax corrections (some syntax issues were mine, others pre-existed) to make the documentation compilation work.
Since .apply() can't handle more than 65535 parameters, splice.apply() is brittle. It's also hard to read. This replaces splice.apply() calls throughout ace code with lang.spliceIntoArray().
This reverts commit 8624ab8.
Matches previous naming convention.
Avoids an extra $split call.
Also brings back the functionality where large deltas are split into smaller deltas so that .splice.apply() calls will work.
This uncovered the fact that until now delta.range had not always been a Range object. This inconsistency has been resolved by my changes in mirror.js.
Set it to true in insertInLine/removeInLine. Also sped up indent/dedent by using insertInLine and removeInLine.
Stores single-line delta content as .text instead of .lines in undo history. This is done without modifying the original delta object in case the caller still retains a handle to the original.
- Fix unconventional '{' formatting
- Reformat `UndoManager` changes
- Revert change from `insertInLine` to `insert` in text.js
This should be faster since we don't have to re-initialize the helper functions each time Anchor.onChange is fired.
Refactor document delta handling
Conflicts: lib/ace/line_widgets.js
Conflicts: lib/ace/document.js lib/ace/editor.js lib/ace/undomanager.js
Conflicts: build lib/ace/document.js
Conflicts: lib/ace/anchor.js lib/ace/keyboard/vim/maps/operators.js
Conflicts: build lib/ace/document.js lib/ace/editor.js
|
@nightwing LGTM, appears to work well. I see there is still an open action item above? Also:
|
Conflicts: lib/ace/commands/command_manager.js
Conflicts: build lib/ace/multi_select_test.js
Conflicts: lib/ace/document.js
|
v-1.2.0-pre it is! |
lennartcl
added a commit
that referenced
this pull request
May 21, 2015
Refactor document delta handling for v1.2
|
@lennartcl, @nightwing - Great to see this merged! @nightwing - Just a heads-up in case there's some loose ends here - looks like you disabled the delta validation in ad54d2c since it "it breaks autocompletion popup". If the auto-complete popup is sending invalid deltas, then silencing the validation is probably not the long-term answer. :) If you do turn off validation, you'll want to also remove the |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
this is continuation of #1745
todo