Skip to content

Commit

Permalink
Refactor deleteRange
Browse files Browse the repository at this point in the history 
Also refactor `cutSection`, change it to accept positions rather than
offsets.
Remove `splitMarker` (now-unused).

Fix one incorrect acceptance test for list items.
Add more unit tests for `deleteRange`.
  • Loading branch information
@bantic
bantic committed Aug 3, 2016
1 parent 93a5709 commit 54f56cc
Show file tree
Hide file tree
Showing 5 changed files with 121 additions and 299 deletions.
257 changes: 71 additions & 186 deletions src/js/editor/post.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,197 +78,106 @@ class PostEditor {
* ```
* let { range } = editor;
* editor.run((postEditor) => {
* postEditor.deleteRange(range);
* let nextPosition = postEditor.deleteRange(range);
* postEditor.setRange(new Range(nextPosition));
* });
* ```
* @param {Range} range Cursor Range object with head and tail Positions
* @return {Position} The position where the cursor would go after deletion
* @public
*/
deleteRange(range) {
// types of selection deletion:
// * a selection starts at the beginning of a section
// -- cursor should end up at the beginning of that section
// -- if the section not longer has markers, add a blank one for the cursor to focus on
// * a selection is entirely within a section
// -- split the markers with the selection, remove those new markers from their section
// -- cursor goes at end of the marker before the selection start, or if the
// -- selection was at the start of the section, cursor goes at section start
// * a selection crosses multiple sections
// -- remove all the sections that are between (exclusive) selection start and end
// -- join the start and end sections
// -- mark the end section for removal
// -- cursor goes at end of marker before the selection start

if (range.isCollapsed) {
return range.head;
}

let {
head, head: {section: headSection, offset: headSectionOffset},
tail, tail: {section: tailSection, offset: tailSectionOffset}
head, head: {section: headSection},
tail, tail: {section: tailSection}
} = range;
const { post } = this.editor;

let nextPosition;
let { editor: { post } } = this;

if (headSection === tailSection) {
nextPosition = this.cutSection(headSection, headSectionOffset, tailSectionOffset);
} else {
let removedSections = [];
let appendSection = headSection;

let recursiveRangeToDelete = null;
return this.cutSection(headSection, head, tail);
}

post.walkLeafSections(range, section => {
if (recursiveRangeToDelete) {
// avoid walking the entire post if we will recurse
return;
}
let nextSection = headSection.nextLeafSection();

switch (section) {
case headSection:
if (section.isCardSection) {
// Create a section to join tailSection with if necessary
appendSection = this.builder.createMarkupSection();

if (head.isHead()) {
// Remove he entire card because we are deleting from its head
// position into section(s) after it
removedSections.push(section);
nextPosition = appendSection.headPosition();
} else {
// Set the position to be the card's tail
nextPosition = section.tailPosition();
}
} else if (section.isBlank) {
// Grab the next leaf section before removing blank head
let nextHeadSection = section.nextLeafSection();

// Remove the blank head section now
this.removeSection(section);

// Advance the range and recursively delete it
recursiveRangeToDelete = new Range(nextHeadSection.headPosition(), range.tail);
} else {
nextPosition = this.cutSection(section, headSectionOffset, section.length);
}
break;
case tailSection:
if (section.isCardSection) {
if (tail.isTail()) {
// If the range extends to the end of the tail section, remove it
removedSections.push(section);
}
} else {

// join the tail section's markers (afer `tailSectionOffset`) to
// the appendSection
section.markersFor(tailSectionOffset, section.text.length).forEach(marker => {
appendSection.markers.append(marker);
});

// If we've modified headSection, ensure it is marked dirty so that it is re-rendered.
// If appendSection !== headSection, it's a new section that will be rendered regardless.
if (appendSection === headSection) {
this._markDirty(headSection);
}

removedSections.push(section);
}
break;
default:
// Default is to remove sections in between head and tail sections
removedSections.push(section);
}
});
let nextPos = this.cutSection(headSection, head, headSection.tailPosition());
// cutSection can replace the section, so re-read headSection here
headSection = nextPos.section;

if (recursiveRangeToDelete) {
// If we are recursing, return early here
return this.deleteRange(recursiveRangeToDelete);
}
// Remove sections in the middle of the range
while (nextSection !== tailSection) {
let tmp = nextSection;
nextSection = nextSection.nextLeafSection();
this.removeSection(tmp);
}

removedSections.forEach(section => this.removeSection(section) );
let tailPos = this.cutSection(tailSection, tailSection.headPosition(), tail);
// cutSection can replace the section, so re-read tailSection here
tailSection = tailPos.section;

if (headSection !== appendSection) {
if (!appendSection.isBlank) {
// if the appendSection is not blank, make sure to add it
this.insertSectionBefore(post.sections, appendSection, headSection.next);
} else if (post.isBlank) {
// if we've removed all the sections from the post, add the blank append section
this.insertSectionBefore(post.sections, appendSection, headSection.next);
}
if (tailSection.isBlank) {
this.removeSection(tailSection);
} else {
// If head and tail sections are markerable, join them
// Note: They may not be the same section type. E.g. this may join
// a tail section that was a list item onto a markup section, or vice versa.
// (This is the desired behavior.)
if (headSection.isMarkerable && tailSection.isMarkerable) {
headSection.join(tailSection);
this._markDirty(headSection);
this.removeSection(tailSection);
} else if (headSection.isBlank) {
this.removeSection(headSection);
nextPos = tailPos;
}
}

return nextPosition;
if (post.isBlank) {
post.sections.append(this.builder.createMarkupSection('p'));
nextPos = post.headPosition();
}

return nextPos;
}

/**
* Note: This method may replace `section` with a different section.
*
* "Cut" out the part of the section inside `headOffset` and `tailOffset`.
* If the section is markerable this removes markers that are wholly inside the offsets,
* and splits markers that straddle the head or tail.
* If section is markerable this splits markers that straddle the head or tail (if necessary),
* and removes markers that are wholly inside the offsets.
* If section is a card, this may replace it with a blank markup section if the
* positions contain the entire card.
*
* @param {Section} section
* @param {Number} headOffset
* @param {Number} tailOffset
* @param {Position} head
* @param {Position} tail
* @return {Position}
* @private
*/
cutSection(section, headOffset, tailOffset) {
if (section.isBlank || headOffset === tailOffset) {
return new Position(section, headOffset);
cutSection(section, head, tail) {
assert('Must pass head position and tail position to `cutSection`',
head instanceof Position && tail instanceof Position);
assert('Must pass positions within same section to `cutSection`',
head.section === tail.section);

if (section.isBlank || head.isEqual(tail)) {
return head;
}
if (section.isCardSection) {
let newSection = this.builder.createMarkupSection();
this.replaceSection(section, newSection);
return newSection.headPosition();
}

let adjustedHead = 0,
marker = section.markers.head,
adjustedTail = marker.length;

// Walk to the first node inside the headOffset, splitting
// a marker if needed. Leave marker as the first node inside.
while (marker) {
if (adjustedTail >= headOffset) {
let splitOffset = headOffset - adjustedHead;
let { afterMarker } = this.splitMarker(marker, splitOffset);
adjustedHead = adjustedHead + splitOffset;
// FIXME: That these two loops cannot agree on adjustedTail being
// incremented at the start or end seems prime for refactoring.
adjustedTail = adjustedHead;
marker = afterMarker;
break;
}
adjustedHead += marker.length;
marker = marker.next;
if (marker) {
adjustedTail += marker.length;
if (head.isHead() && tail.isTail()) {
let newSection = this.builder.createMarkupSection();
this.replaceSection(section, newSection);
return newSection.headPosition();
} else {
return tail;
}
}

// Walk each marker inside, removing it if needed. when the last is
// reached split it and remove the part inside the tailOffset
while (marker) {
adjustedTail += marker.length;
if (adjustedTail >= tailOffset) {
let splitOffset = marker.length - (adjustedTail - tailOffset);
let {
beforeMarker
} = this.splitMarker(marker, splitOffset);
if (beforeMarker) {
this.removeMarker(beforeMarker);
}
break;
}
adjustedHead += marker.length;
let nextMarker = marker.next;
this.removeMarker(marker);
marker = nextMarker;
}
let range = new Range(head, tail);
this.splitMarkers(range).forEach(m => this.removeMarker(m));

return new Position(section, headOffset);
return head;
}

_coalesceMarkers(section) {
Expand Down Expand Up @@ -469,18 +378,17 @@ class PostEditor {

/**
* Split markers at two positions, once at the head, and if necessary once
* at the tail. This method is designed to accept a range
* (e.g. `editor.range`) as an argument.
* at the tail.
*
* Usage:
* ```
* let markerRange = this.cursor.offsets;
* let range = editor.range;
* editor.run((postEditor) => {
* postEditor.splitMarkers(markerRange);
* postEditor.splitMarkers(range);
* });
* ```
* The return value will be marker object completely inside the offsets
* provided. Markers on the outside of the split may also have been modified.
* provided. Markers outside of the split may also have been modified.
*
* @param {Range} markerRange
* @return {Array} of markers that are inside the split
Expand All @@ -501,29 +409,6 @@ class PostEditor {
edit.removed.forEach(m => this.removeMarker(m));
}

splitMarker(marker, offset) {
let beforeMarker, afterMarker;

if (offset === 0) {
beforeMarker = marker.prev;
afterMarker = marker;
} else if (offset === marker.length) {
beforeMarker = marker;
afterMarker = marker.next;
} else {
let { builder } = this.editor,
{ section } = marker;
beforeMarker = builder.createMarker(marker.value.substring(0, offset), marker.markups);
afterMarker = builder.createMarker(marker.value.substring(offset, marker.length), marker.markups);
section.markers.splice(marker, 1, [beforeMarker, afterMarker]);

this.removeMarker(marker);
this._markDirty(section);
}

return { beforeMarker, afterMarker };
}

/**
* Split the section at the position.
*
Expand All @@ -541,7 +426,7 @@ class PostEditor {
* headMarkerOffset is 0.
*
* @param {Position} position
* @return {Array} new sections, one for the first half and one for the second
* @return {Array} new sections, one for the first half and one for the second (either one can be null)
* @public
*/
splitSection(position) {
Expand Down
9 changes: 4 additions & 5 deletions tests/acceptance/editor-list-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -304,7 +304,7 @@ test('forward-delete in empty li with li after it joins with li', (assert) => {
assert.hasElement('#editor li:contains(Xabc)', 'inserts text at right spot');
});

test('forward-delete in empty li with markup section after it deletes li', (assert) => {
test('forward-delete in empty li with markup section after it joins markup section', (assert) => {
const mobiledoc = Helpers.mobiledoc.build(builder => {
const {post, listSection, listItem, markupSection, marker} = builder;
return post([
Expand All @@ -319,13 +319,12 @@ test('forward-delete in empty li with markup section after it deletes li', (asse
Helpers.dom.moveCursorTo(editor, node, 0);
Helpers.dom.triggerForwardDelete(editor);

assert.hasNoElement('#editor li', 'li is removed');
assert.hasElement('#editor p:contains(abc)', 'p remains');
assert.hasElement('#editor li:contains(abc)', 'joins markup section');
assert.hasNoElement('#editor p', 'p is removed');

Helpers.dom.insertText(editor, 'X');

assert.hasElement('#editor p:contains(Xabc)', 'inserts text at right spot');

assert.hasElement('#editor li:contains(Xabc)', 'inserts text at right spot');
});

test('forward-delete end of li with nothing after', (assert) => {
Expand Down
Loading

0 comments on commit 54f56cc

Please sign in to comment.