From a83b176e259225acb414bec9b81ddafab6d0dd5c Mon Sep 17 00:00:00 2001 From: Cory Forsyth Date: Thu, 17 Sep 2015 14:41:23 -0400 Subject: [PATCH] Coalesce markers that have identical markups This prevents some drift in the mobiledoc that comes with a lot of editing. Prior to this, when contiguous markers were changed so that their markups were identically, they continued to be stored as separate markers (and rendered as contiguous textNodes). The resulted in a messy rendered mobiledoc, with many vacuously similar markers. Keeping markers coordinated should also reduce memory pressure in large documents. --- README.md | 14 ++- src/js/editor/editor.js | 11 ++- src/js/editor/post.js | 107 +++++++++++++-------- src/js/utils/array-utils.js | 22 ++++- src/js/utils/linked-list.js | 3 + tests/acceptance/editor-selections-test.js | 18 ++-- tests/unit/editor/post-test.js | 95 ++++++++++++------ 7 files changed, 171 insertions(+), 99 deletions(-) diff --git a/README.md b/README.md index 4157d8934..3f51bbf3e 100644 --- a/README.md +++ b/README.md @@ -92,7 +92,7 @@ editor.didUpdatePost(postEditor => { The available lifecycle hooks are: -* `editor.didUpdatePost(postEditor => {})` - An opprotunity to use the +* `editor.didUpdatePost(postEditor => {})` - An opportunity to use the `postEditor` and possibly change the post before rendering begins. * `editor.willRender()` - After all post mutation has finished, but before the DOM is updated. @@ -107,13 +107,11 @@ the creation of completely custom interfaces for buttons, hot-keys, and other interactions. To change the post in code, use the `editor.run` API. For example, the -following usage would mark currently selected text as bold: +following usage would mark currently selected text as "strong": ```js -const strongMarkup = editor.builder.createMarkup('strong'); -const range = editor.cursor.offsets; -editor.run((postEditor) => { - postEditor.applyMarkupToRange(range, strongMarkup); +editor.run(postEditor => { + postEditor.toggleMarkup('strong'); }); ``` @@ -121,10 +119,10 @@ It is important that you make changes to posts, sections, and markers through the `run` and `postEditor` API. This API allows Content-Kit to conserve and better understand changes being made to the post. -For more details on the API of `postEditor`, see the [API documentation](https://github.com/mixonic/content-kit-editor/blob/master/src/js/editor/post.js). +For more details on the API of `postEditor`, see the [API documentation](https://github.com/bustlelabs/content-kit-editor/blob/master/src/js/editor/post.js). For more details on the API for the builder, required to create new sections -and markers, see the [builder API](https://github.com/mixonic/content-kit-editor/blob/master/src/js/models/post-node-builder.js). +and markers, see the [builder API](https://github.com/bustlelabs/content-kit-editor/blob/master/src/js/models/post-node-builder.js). ### Contributing diff --git a/src/js/editor/editor.js b/src/js/editor/editor.js index 09cafcc5a..f6b34220a 100644 --- a/src/js/editor/editor.js +++ b/src/js/editor/editor.js @@ -224,10 +224,10 @@ class Editor { handleNewline(event) { if (!this.cursor.hasCursor()) { return; } - const range = this.cursor.offsets; event.preventDefault(); - const cursorSection = this.run((postEditor) => { + const range = this.cursor.offsets; + const cursorSection = this.run(postEditor => { if (!range.isCollapsed) { postEditor.deleteRange(range); if (range.head.section.isBlank) { @@ -262,9 +262,8 @@ class Editor { cancelSelection() { if (this._hasSelection) { - // FIXME perhaps restore cursor position to end of the selection? - this.cursor.clearSelection(); - this.reportNoSelection(); + const range = this.cursor.offsets; + this.moveToPosition(range.tail); } } @@ -339,6 +338,8 @@ class Editor { this.cursor.moveToSection(headSection, headSectionOffset); } + // FIXME this should be able to be removed now -- if any sections are detached, + // it's due to a bug in the code. _removeDetachedSections() { forEach( filter(this.post.sections, s => !s.renderNode.isAttached()), diff --git a/src/js/editor/post.js b/src/js/editor/post.js index 6ed92e03b..7fda948d1 100644 --- a/src/js/editor/post.js +++ b/src/js/editor/post.js @@ -3,7 +3,9 @@ import { } from '../models/markup-section'; import { POST_TYPE, MARKUP_SECTION_TYPE, LIST_ITEM_TYPE } from '../models/types'; import Position from '../utils/cursor/position'; -import { any, filter, compact } from '../utils/array-utils'; +import { + isArrayEqual, any, forEach, filter, compact +} from '../utils/array-utils'; import { DIRECTION } from '../utils/key'; function isMarkupSection(section) { @@ -26,10 +28,13 @@ class PostEditor { constructor(editor) { this.editor = editor; this.builder = this.editor.builder; - this._completionWorkQueue = []; - this._afterRenderQueue = []; - this._didRerender = false; - this._didUpdate = false; + this._queues = { + beforeCompletion: [], + completion: [], + afterCompletion: [] + }; + this._didScheduleRerender = false; + this._didScheduleUpdate = false; this._didComplete = false; } @@ -92,14 +97,10 @@ class PostEditor { }); removedSections.forEach(section => this.removeSection(section) ); } - - this._coalesceMarkers(headSection); } cutSection(section, headSectionOffset, tailSectionOffset) { - if (section.markers.isEmpty) { - return; - } + if (section.isBlank) { return; } let adjustedHead = 0, marker = section.markers.head, @@ -147,9 +148,32 @@ class PostEditor { } _coalesceMarkers(section) { - filter(section.markers, m => m.isEmpty).forEach(marker => { - this.removeMarker(marker); - }); + this._removeEmptyMarkers(section); + this._joinSimilarMarkers(section); + } + + _removeEmptyMarkers(section) { + forEach( + filter(section.markers, m => m.isEmpty), + m => this.removeMarker(m) + ); + } + + // joins markers that have identical markups + _joinSimilarMarkers(section) { + let marker = section.markers.head; + let nextMarker; + while (marker && marker.next) { + nextMarker = marker.next; + + if (isArrayEqual(marker.markups, nextMarker.markups)) { + nextMarker.value = marker.value + nextMarker.value; + this._markDirty(nextMarker); + this.removeMarker(marker); + } + + marker = nextMarker; + } } removeMarker(marker) { @@ -167,6 +191,9 @@ class PostEditor { this.scheduleRerender(); this.scheduleDidUpdate(); } + if (isMarkerable(postNode)) { + this._queues.beforeCompletion.push(() => this._coalesceMarkers(postNode)); + } } _markDirty(postNode) { @@ -176,6 +203,12 @@ class PostEditor { this.scheduleRerender(); this.scheduleDidUpdate(); } + if (postNode.section) { + this._markDirty(postNode.section); + } + if (isMarkerable(postNode)) { + this._queues.beforeCompletion.push(() => this._coalesceMarkers(postNode)); + } } /** @@ -305,7 +338,6 @@ class PostEditor { } else { marker.deleteValueAtOffset(offset); this._markDirty(marker); - this._coalesceMarkers(marker.section); } return nextPosition; @@ -344,7 +376,6 @@ class PostEditor { marker.deleteValueAtOffset(offsetToDeleteAt); nextPosition.offset -= 1; this._markDirty(marker); - this._coalesceMarkers(marker.section); return nextPosition; } @@ -511,19 +542,15 @@ class PostEditor { * value as `splitMarkers`. * * @method applyMarkupToRange - * @param {Range} markerRange + * @param {Range} range * @param {Markup} markup A markup post abstract node - * @return {Array} of markers that are inside the split * @public */ - applyMarkupToRange(markerRange, markup) { - const markers = this.splitMarkers(markerRange); - markers.forEach(marker => { + applyMarkupToRange(range, markup) { + this.splitMarkers(range).forEach(marker => { marker.addMarkup(markup); this._markDirty(marker.section); }); - - return markers; } /** @@ -547,7 +574,6 @@ class PostEditor { * @method removeMarkupFromRange * @param {Range} range Object with offsets * @param {Markup} markup A markup post abstract node - * @return {Array} of markers that are inside the split * @private */ removeMarkupFromRange(range, markupOrMarkupCallback) { @@ -556,8 +582,6 @@ class PostEditor { marker.removeMarkup(markupOrMarkupCallback); this._markDirty(marker.section); }); - - return markers; } /** @@ -709,7 +733,7 @@ class PostEditor { if (this._didComplete) { throw new Error('Work can only be scheduled before a post edit has completed'); } - this._completionWorkQueue.push(callback); + this._queues.completion.push(callback); } /** @@ -719,12 +743,10 @@ class PostEditor { * @public */ scheduleRerender() { - this.schedule(() => { - if (!this._didRerender) { - this._didRerender = true; - this.editor.rerender(); - } - }); + if (!this._didScheduleRerender) { + this.schedule(() => this.editor.rerender()); + this._didScheduleRerender = true; + } } /** @@ -734,16 +756,14 @@ class PostEditor { * @public */ scheduleDidUpdate() { - this.schedule(() => { - if (!this._didUpdate) { - this._didUpdate = true; - this.editor.didUpdate(); - } - }); + if (!this._didScheduleUpdate) { + this.schedule(() => this.editor.didUpdate()); + this._didScheduleUpdate = true; + } } scheduleAfterRender(callback) { - this._afterRenderQueue.push(callback); + this._queues.afterCompletion.push(callback); } /** @@ -757,9 +777,14 @@ class PostEditor { if (this._didComplete) { throw new Error('Post editing can only be completed once'); } + + this._runCallbacks([this._queues.beforeCompletion]); this._didComplete = true; - this._completionWorkQueue.forEach(cb => cb()); - this._afterRenderQueue.forEach(cb => cb()); + this._runCallbacks([this._queues.completion, this._queues.afterCompletion]); + } + + _runCallbacks(queues) { + queues.forEach(queue => queue.forEach(cb => cb())); } } diff --git a/src/js/utils/array-utils.js b/src/js/utils/array-utils.js index 73c2af100..c6c9d97d8 100644 --- a/src/js/utils/array-utils.js +++ b/src/js/utils/array-utils.js @@ -10,9 +10,11 @@ function detect(enumerable, callback) { } } -function any(array, callback) { - for (let i=0; i { diff --git a/tests/acceptance/editor-selections-test.js b/tests/acceptance/editor-selections-test.js index 3ed5d4ef5..7eff2978a 100644 --- a/tests/acceptance/editor-selections-test.js +++ b/tests/acceptance/editor-selections-test.js @@ -63,8 +63,8 @@ test('selecting an entire section and deleting removes it', (assert) => { Helpers.dom.selectText('second section', editorElement); Helpers.dom.triggerDelete(editor); - assert.hasElement('p:contains(first section)'); - assert.hasNoElement('p:contains(second section)', 'deletes contents of second section'); + assert.hasElement('#editor p:contains(first section)'); + assert.hasNoElement('#editor p:contains(second section)', 'deletes contents of second section'); assert.equal($('#editor p').length, 2, 'still has 2 sections'); Helpers.dom.insertText(editor, 'X'); @@ -81,16 +81,12 @@ test('selecting text in a section and deleting deletes it', (assert) => { Helpers.dom.selectText('cond sec', editorElement); Helpers.dom.triggerDelete(editor); - assert.hasElement('p:contains(first section)', 'first section unchanged'); - assert.hasNoElement('p:contains(second section)', 'second section is no longer there'); - assert.hasElement('p:contains(setion)', 'second section has correct text'); + assert.hasElement('#editor p:contains(first section)', 'first section unchanged'); + assert.hasNoElement('#editor p:contains(second section)', 'second section is no longer there'); + assert.hasElement('#editor p:contains(setion)', 'second section has correct text'); - let textNode = $('p:contains(setion)')[0].childNodes[0]; - assert.equal(textNode.textContent, 'se', 'precond - has correct text node'); - let charOffset = 2; // after the 'e' in 'se' - - assert.deepEqual(Helpers.dom.getCursorPosition(), - {node: textNode, offset: charOffset}); + Helpers.dom.insertText(editor, 'Z'); + assert.hasElement('#editor p:contains(seZtion)', 'text inserted correctly'); }); test('selecting text across sections and deleting joins sections', (assert) => { diff --git a/tests/unit/editor/post-test.js b/tests/unit/editor/post-test.js index f4eddf47c..a54979002 100644 --- a/tests/unit/editor/post-test.js +++ b/tests/unit/editor/post-test.js @@ -97,11 +97,8 @@ test('#deleteFrom offset 0 joins section with previous if first marker', (assert const nextPosition = postEditor.deleteFrom(position); postEditor.complete(); - assert.equal(editor.post.sections.length, 1, - 'sections joined'); - assert.equal(getSection(0).markers.length, 2, - 'joined section has 2 markers'); - + assert.equal(editor.post.sections.length, 1, 'sections joined'); + assert.equal(getSection(0).markers.length, 1, 'joined section has 1 marker'); assert.equal(getSection(0).text, 'abcdef', 'text is joined'); assert.ok(nextPosition.section === getSection(0), 'correct position section'); assert.equal(nextPosition.offset, 'abc'.length, 'correct position offset'); @@ -120,11 +117,8 @@ test('#deleteFrom (FORWARD) end of marker joins section with next if last marker const nextPosition = postEditor.deleteFrom(position, FORWARD); postEditor.complete(); - assert.equal(editor.post.sections.length, 1, - 'sections joined'); - assert.equal(getSection(0).markers.length, 2, - 'joined section has 2 markers'); - + assert.equal(editor.post.sections.length, 1, 'sections joined'); + assert.equal(getSection(0).markers.length, 1, 'joined section has 1 marker'); assert.equal(getSection(0).text, 'abcdef', 'text is joined'); assert.ok(nextPosition.section === getSection(0), 'correct position section'); assert.equal(nextPosition.offset, 'abc'.length, 'correct position offset'); @@ -316,58 +310,49 @@ test('#cutSection with one marker', (assert) => { }); renderBuiltAbstract(post); - postEditor.cutSection(section, 1, 2); - postEditor.complete(); assert.equal(post.sections.head.text, 'ac'); assert.equal(post.sections.length, 1, 'only 1 section remains'); - assert.equal(post.sections.head.markers.length, 2, 'two markers remain'); + assert.equal(post.sections.head.markers.length, 1, 'markers are joined'); }); test('#cutSection at boundaries across markers', (assert) => { let post, section; Helpers.postAbstract.build(({marker, markupSection, post: buildPost}) => { - section = markupSection('p', [ - marker('a'), - marker('b'), - marker('c'), - marker('d') - ]); - post = buildPost([ section ]); + const markers = "abcd".split('').map(l => marker(l)); + section = markupSection('p', markers); + post = buildPost([section]); }); renderBuiltAbstract(post); - + assert.equal(post.sections.head.text, 'abcd'); //precond + assert.equal(post.sections.head.markers.length, 4); //precond postEditor.cutSection(section, 1, 3); - postEditor.complete(); assert.equal(post.sections.head.text, 'ad'); assert.equal(post.sections.length, 1, 'only 1 section remains'); - assert.equal(post.sections.head.markers.length, 2, 'two markers remain'); + assert.equal(post.sections.head.markers.length, 1, 'markers are joined'); }); test('#cutSection in head marker', (assert) => { let post, section; Helpers.postAbstract.build(({marker, markupSection, post: buildPost}) => { - section = markupSection('p', [ - marker('a'), - marker('bc') - ]); + section = markupSection('p', [marker('a'), marker('bc')]); post = buildPost([ section ]); }); renderBuiltAbstract(post); - + assert.equal(section.text, 'abc'); //precond + assert.equal(section.markers.length, 2); //precond postEditor.cutSection(section, 2, 3); - postEditor.complete(); assert.equal(post.sections.head.text, 'ab'); assert.equal(post.sections.length, 1, 'only 1 section remains'); - assert.equal(post.sections.head.markers.length, 2, 'two markers remain'); + assert.equal(post.sections.head.markers.length, 1, 'markers are joined'); }); test('#cutSection in tail marker', (assert) => { @@ -596,3 +581,53 @@ test('#insertSectionAtEnd inserts the section at the end of the mobiledoc', (ass assert.equal(post.sections.length, 2, 'new section added'); assert.equal(post.sections.tail.text, '123', 'new section added at end'); }); + +test('markers with identical non-attribute markups get coalesced after applying or removing markup', (assert) => { + let strong, section; + const post = Helpers.postAbstract.build(({post, markupSection, marker, markup}) => { + strong = markup('strong'); + section = markupSection('p', [marker('a'), marker('b',[strong]), marker('c')]); + return post([section]); + }); + renderBuiltAbstract(post); + + // removing the strong from the "b" + let range = Range.create(section, 1, section, 2); + postEditor = new PostEditor(mockEditor); + postEditor.removeMarkupFromRange(range, strong); + postEditor.complete(); + + assert.equal(section.markers.length, 1, 'similar markers are coalesced'); + assert.equal(section.markers.head.value, 'abc', 'marker value is correct'); + assert.ok(!section.markers.head.hasMarkup(strong), 'marker has no bold'); + + // adding strong to each of the characters individually + postEditor = new PostEditor(mockEditor); + for (let i=0; i < section.length; i++) { + range = Range.create(section, i, section, i+1); + postEditor.applyMarkupToRange(range, strong); + } + postEditor.complete(); + + assert.equal(section.markers.length, 1, 'bold markers coalesced'); + assert.equal(section.markers.head.value, 'abc', 'bold marker value is correct'); + assert.ok(section.markers.head.hasMarkup(strong), 'bold marker has bold'); +}); + +test('markers with identical markups get coalesced after deletion', (assert) => { + let strong, section; + const post = Helpers.postAbstract.build(({post, markupSection, marker, markup}) => { + strong = markup('strong'); + section = markupSection('p', [marker('a'), marker('b',[strong]), marker('c')]); + return post([section]); + }); + renderBuiltAbstract(post); + + let range = Range.create(section, 1, section, 2); + postEditor = new PostEditor(mockEditor); + postEditor.deleteRange(range); + postEditor.complete(); + + assert.equal(section.markers.length, 1, 'similar markers are coalesced'); + assert.equal(section.markers.head.value, 'ac', 'marker value is correct'); +});