Merge pull request #120 from bustlelabs/bold-button-state-reflection-119

Fix bug in getting activeMarkers, use editor#markupsInSelection

README.md

@@ -110,10 +110,10 @@ To change the post in code, use the `editor.run` API. For example, the
 following usage would mark currently selected text as bold:
 
 ```js
-let strongMarkup = editor.builder.createMarkup('strong');
-let markerRange = editor.cursor.offsets;
+const strongMarkup = editor.builder.createMarkup('strong');
+const range = editor.cursor.offsets;
 editor.run((postEditor) => {
-  postEditor.applyMarkupToMarkers(markerRange, strongMarkup);
+  postEditor.applyMarkupToRange(range, strongMarkup);
 });
 ```
 

src/js/commands/link.js

@@ -11,36 +11,23 @@ export default class LinkCommand extends TextFormatCommand {
   }
 
   isActive() {
-    return any(this.editor.activeMarkers, m => m.hasMarkup(this.tag));
+    return any(this.editor.markupsInSelection, m => m.hasTag(this.tag));
   }
 
   exec(url) {
     const range = this.editor.cursor.offsets;
-
-    let markers = this.editor.run(postEditor => {
+    this.editor.run(postEditor => {
       const markup = postEditor.builder.createMarkup('a', ['href', url]);
-      return postEditor.applyMarkupToMarkers(range, markup);
+      postEditor.applyMarkupToRange(range, markup);
     });
-
-    if (markers.length) {
-      let lastMarker = markers[markers.length - 1];
-      this.editor.cursor.moveToMarker(lastMarker, lastMarker.length);
-    } /* else {
-      // FIXME should handle the case when linking creating no new markers
-      // this.editor.cursor.moveToSection(range.head.section);
-    } */
+    this.editor.moveToPosition(range.tail);
   }
 
   unexec() {
     const range = this.editor.cursor.offsets;
-
-    const markers = this.editor.run(postEditor => {
-      return postEditor.removeMarkupFromMarkers(
-        range,
-        markup => markup.hasTag('a')
-      );
+    this.editor.run(postEditor => {
+      postEditor.removeMarkupFromRange(range, markup => markup.hasTag('a'));
     });
-
-    this.editor.selectMarkers(markers);
+    this.editor.selectRange(range);
   }
 }

src/js/commands/text-format.js

@@ -10,28 +10,25 @@ export default class TextFormatCommand extends Command {
 
   get markup() {
     if (this._markup) { return this._markup; }
-    const { builder } = this.editor;
-    this._markup = builder.createMarkup(this.tag);
+    this._markup = this.editor.builder.createMarkup(this.tag);
     return this._markup;
   }
 
   isActive() {
-    return any(this.editor.activeMarkers, m => m.hasMarkup(this.markup));
+    return any(this.editor.markupsInSelection, m => m === this.markup);
   }
 
   exec() {
-    const range = this.editor.cursor.offsets;
-    const markers = this.editor.run((postEditor) => {
-      return postEditor.applyMarkupToMarkers(range, this.markup);
-    });
-    this.editor.selectMarkers(markers);
+    const range = this.editor.cursor.offsets, { markup } = this;
+    this.editor.run(
+      postEditor => postEditor.applyMarkupToRange(range, markup));
+    this.editor.selectRange(range);
   }
 
   unexec() {
-    const range = this.editor.cursor.offsets;
-    const markers = this.editor.run((postEditor) => {
-      return postEditor.removeMarkupFromMarkers(range, this.markup);
-    });
-    this.editor.selectMarkers(markers);
+    const range = this.editor.cursor.offsets, { markup } = this;
+    this.editor.run(
+      postEditor => postEditor.removeMarkupFromRange(range, markup));
+    this.editor.selectRange(range);
   }
 }

src/js/editor/editor.js

@@ -295,9 +295,18 @@ class Editor {
     this.reportSelection();
   }
 
-  selectMarkers(markers) {
-    this.cursor.selectMarkers(markers);
-    this.reportSelection();
+  selectRange(range){
+    this.cursor.selectRange(range);
+    if (range.isCollapsed) {
+      this.reportNoSelection();
+    } else {
+      this.reportSelection();
+    }
+  }
+
+  moveToPosition(position) {
+    this.cursor.moveToPosition(position);
+    this.reportNoSelection();
   }
 
   get cursor() {
@@ -371,21 +380,13 @@ class Editor {
     return this.cursor.activeSections;
   }
 
-  get activeMarkers() {
-    const {
-      headMarker,
-      tailMarker
-    } = this.cursor.offsets;
-
-    let activeMarkers = [];
-
-    if (headMarker && tailMarker) {
-      this.post.markersFrom(headMarker, tailMarker, m => {
-        activeMarkers.push(m);
-      });
+  get markupsInSelection() {
+    if (this.cursor.hasSelection()) {
+      const range = this.cursor.offsets;
+      return this.post.markupsInRange(range);
+    } else {
+      return [];
     }
-
-    return activeMarkers;
   }
 
   /*

src/js/editor/post.js

@@ -145,18 +145,6 @@ class PostEditor {
     }
   }
 
-  removeMarkerRange(headMarker, tailMarker) {
-    let marker = headMarker;
-    while (marker) {
-      let nextMarker = marker.next;
-      this.removeMarker(marker);
-      if (marker === tailMarker) {
-        break;
-      }
-      marker = nextMarker;
-    }
-  }
-
   _coalesceMarkers(section) {
     filter(section.markers, m => m.isEmpty).forEach(marker => {
       this.removeMarker(marker);
@@ -357,8 +345,8 @@ class PostEditor {
 
   /**
    * Split markers at two positions, once at the head, and if necessary once
-   * at the tail. This method is designed to accept `editor.cursor.offsets`
-   * as an argument.
+   * at the tail. This method is designed to accept a range
+   * (e.g. `editor.cursor.offsets`) as an argument.
    *
    * Usage:
    *
@@ -371,18 +359,23 @@ class PostEditor {
    * provided. Markers on the outside of the split may also have been modified.
    *
    * @method splitMarkers
-   * @param {Object} markerRange Object with offsets, {headMarker, headMarkerOffset, tailMarker, tailMarkerOffset}
+   * @param {Range} markerRange
    * @return {Array} of markers that are inside the split
    * @public
    */
-  splitMarkers({headMarker, headMarkerOffset, tailMarker, tailMarkerOffset}) {
+  splitMarkers(range) {
     const { post } = this.editor;
+    const {
+      headSection,
+      tailSection,
+      headMarker,
+      headMarkerOffset,
+      tailMarker,
+      tailMarkerOffset
+    } = range;
     let selectedMarkers = [];
 
-    let headSection = headMarker.section;
-    let tailSection = tailMarker.section;
-
-    // These render will be removed by the split functions. Mark them
+    // These render nodes will be removed by the split functions. Mark them
     // for removal before doing that. FIXME this seems prime for
     // refactoring onto the postEditor as a split function
     headMarker.renderNode.scheduleForRemoval();
@@ -550,24 +543,24 @@ class PostEditor {
    *
    * Usage:
    *
-   *     let markerRange = editor.cursor.offsets;
-   *     let strongMarkup = editor.builder.createMarkup('strong');
+   *     const range = editor.cursor.offsets;
+   *     const strongMarkup = editor.builder.createMarkup('strong');
    *     editor.run((postEditor) => {
-   *       postEditor.applyMarkupToMarkers(markerRange, strongMarkup);
+   *       postEditor.applyMarkupToRange(range, strongMarkup);
    *     });
    *     // Will result some markers possibly being split, and the markup
    *     // being applied to all markers between the split.
    *
    * The return value will be all markers between the split, the same return
    * value as `splitMarkers`.
    *
-   * @method applyMarkupToMarkers
-   * @param {Object} markerRange Object with offsets
-   * @param {Object} markup A markup post abstract node
+   * @method applyMarkupToRange
+   * @param {Range} markerRange
+   * @param {Markup} markup A markup post abstract node
    * @return {Array} of markers that are inside the split
    * @public
    */
-  applyMarkupToMarkers(markerRange, markup) {
+  applyMarkupToRange(markerRange, markup) {
     const markers = this.splitMarkers(markerRange);
     markers.forEach(marker => {
       marker.addMarkup(markup);
@@ -587,25 +580,25 @@ class PostEditor {
    *
    * Usage:
    *
-   *     let markerRange = editor.cursor.offsets;
-   *     let markup = markerRange.headMarker.markups[0];
+   *     const range = editor.cursor.offsets;
+   *     const markup = markerRange.headMarker.markups[0];
    *     editor.run((postEditor) => {
-   *       postEditor.removeMarkupFromMarkers(markerRange, markup);
+   *       postEditor.removeMarkupFromRange(range, markup);
    *     });
    *     // Will result some markers possibly being split, and the markup
    *     // being removed from all markers between the split.
    *
    * The return value will be all markers between the split, the same return
    * value as `splitMarkers`.
    *
-   * @method removeMarkupFromMarkers
-   * @param {Object} markerRange Object with offsets
-   * @param {Object} markup A markup post abstract node
+   * @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
    * @public
    */
-  removeMarkupFromMarkers(markerRange, markupOrMarkupCallback) {
-    const markers = this.splitMarkers(markerRange);
+  removeMarkupFromRange(range, markupOrMarkupCallback) {
+    const markers = this.splitMarkers(range);
     markers.forEach(marker => {
       marker.removeMarkup(markupOrMarkupCallback);
       marker.section.renderNode.markDirty();

src/js/models/_markerable.js

@@ -1,5 +1,6 @@
 import { normalizeTagName } from '../utils/dom-utils';
 import { forEach, filter, reduce } from '../utils/array-utils';
+import Set from '../utils/set';
 
 import LinkedItem from '../utils/linked-item';
 import LinkedList from '../utils/linked-list';
@@ -124,27 +125,64 @@ export default class Markerable extends LinkedItem {
     return reduce(this.markers, (prev, m) => prev + m.value, '');
   }
 
+  get length() {
+    return this.text.length;
+  }
+
+  /**
+   * @return {Array} New markers that match the boundaries of the
+   * range.
+   */
   markersFor(headOffset, tailOffset) {
+    const range = {head: {section:this, offset:headOffset},
+                   tail: {section:this, offset:tailOffset}};
+
     let markers = [];
-    let adjustedHead = 0, adjustedTail = 0;
-    this.markers.forEach(m => {
-      adjustedTail += m.length;
-
-      // if current 'window' of [adjustedHead..adjustedTail] is within
-      // [headOffset..tailOffset] range
-      if (adjustedTail > headOffset && adjustedHead < tailOffset) {
-        let head = Math.max(headOffset - adjustedHead, 0);
-        let tail = m.length - Math.max(adjustedTail - tailOffset, 0);
-        let cloned = m.clone();
-
-        cloned.value = m.value.slice(head, tail);
-        markers.push(cloned);
-      }
-      adjustedHead += m.length;
+    this._markersInRange(range, (marker, {markerHead, markerTail}) => {
+      const cloned = marker.clone();
+      cloned.value = marker.value.slice(markerHead, markerTail);
+      markers.push(cloned);
     });
     return markers;
   }
 
+  markupsInRange(range) {
+    const markups = new Set();
+    this._markersInRange(range, marker => {
+      marker.markups.forEach(m => markups.add(m));
+    });
+    return markups.toArray();
+  }
+
+  // calls the callback with (marker, {markerHead, markerTail})
+  // for each marker that is wholly or partially contained in the range
+  _markersInRange(range, callback) {
+    const { head, tail } = range;
+    if (head.section !== this || tail.section !== this) {
+      throw new Error('Cannot call #_markersInRange if range expands beyond this');
+    }
+    const {offset:headOffset} = head, {offset:tailOffset} = tail;
+
+    let currentHead = 0, currentTail = 0, currentMarker = this.markers.head;
+
+    while (currentMarker) {
+      currentTail += currentMarker.length;
+
+      if (currentTail > headOffset && currentHead < tailOffset) {
+        let markerHead = Math.max(headOffset - currentHead, 0);
+        let markerTail = currentMarker.length -
+          Math.max(currentTail - tailOffset, 0);
+
+        callback(currentMarker, {markerHead, markerTail});
+      }
+
+      currentHead += currentMarker.length;
+      currentMarker = currentMarker.next;
+
+      if (currentHead > tailOffset) { break; }
+    }
+  }
+
   // mutates this by appending the other section's (cloned) markers to it
   join(otherSection) {
     let beforeMarker = this.markers.tail;

src/js/models/post.js

@@ -1,6 +1,7 @@
 export const POST_TYPE = 'post';
 import LinkedList from 'content-kit-editor/utils/linked-list';
-import { compact } from 'content-kit-editor/utils/array-utils';
+import { forEach, compact } from 'content-kit-editor/utils/array-utils';
+import Set from 'content-kit-editor/utils/set';
 
 export default class Post {
   constructor() {
@@ -62,6 +63,7 @@ export default class Post {
 
     return {changedSections, removedSections};
   }
+
   /**
    * Invoke `callbackFn` for all markers between the headMarker and tailMarker (inclusive),
    * across sections
@@ -83,6 +85,19 @@ export default class Post {
     }
   }
 
+  markupsInRange(range) {
+    const markups = new Set();
+
+    this.walkMarkerableSections(range, (section) => {
+      forEach(
+        section.markupsInRange(range.trimTo(section)),
+        m => markups.add(m)
+      );
+    });
+
+    return markups.toArray();
+  }
+
   walkMarkerableSections(range, callback) {
     const {head, tail} = range;