Merge pull request #2 from hypothesis/fix-new-annot-selection

Improve behavior when selecting pre-created annotations

h/static/scripts/annotation-sync.coffee

@@ -53,11 +53,6 @@ module.exports = class AnnotationSync
       this._syncCache(channel)
     @bridge.onConnect(onConnect)
 
-  # Provide a public interface to the annotation cache so that other
-  # sync services can lookup annotations by tag.
-  getAnnotationForTag: (tag) ->
-    @cache[tag] or null
-
   sync: (annotations) ->
     annotations = (this._format a for a in annotations)
     @bridge.call 'sync', annotations, (err, annotations = []) =>

h/static/scripts/annotation-ui-controller.js

@@ -21,7 +21,7 @@ function AnnotationUIController($rootScope, $scope, annotationUI) {
   });
 
   $rootScope.$on(events.ANNOTATION_DELETED, function (event, annotation) {
-    annotationUI.removeSelectedAnnotation(annotation);
+    annotationUI.removeSelectedAnnotation(annotation.id);
   });
 }
 

h/static/scripts/annotation-ui-sync.js

@@ -9,7 +9,6 @@ var uiConstants = require('./ui-constants');
  * @name AnnotationUISync
  * @param {$window} $window An Angular window service.
  * @param {Bridge} bridge
- * @param {AnnotationSync} annotationSync
  * @param {AnnotationUI} annotationUI An instance of the AnnotatonUI service
  * @description
  * Listens for incoming events over the bridge concerning the annotation
@@ -17,29 +16,36 @@ var uiConstants = require('./ui-constants');
  * that the messages are broadcast out to other frames.
  */
 // @ngInject
-function AnnotationUISync($rootScope, $window, bridge, annotationSync,
-  annotationUI) {
-  // Retrieves annotations from the annotationSync cache.
-  var getAnnotationsByTags = function (tags) {
-    return tags.map(annotationSync.getAnnotationForTag, annotationSync);
-  };
+function AnnotationUISync($rootScope, $window, annotationUI, bridge) {
+  function lookupByTag(tag) {
+    return annotationUI.getState().annotations.find(function (annot) {
+      return annot.$$tag === tag;
+    });
+  }
+
+  function findIDsForTags(tags) {
+    var ids = [];
+    tags.forEach(function (tag) {
+      var annot = lookupByTag(tag);
+      if (annot && annot.id) {
+        ids.push(annot.id);
+      }
+    });
+    return ids;
+  }
 
   var channelListeners = {
     showAnnotations: function (tags) {
-      tags = tags || [];
-      var annotations = getAnnotationsByTags(tags);
-      annotationUI.selectAnnotations(annotations);
+      var ids = findIDsForTags(tags || []);
+      annotationUI.selectAnnotations(ids);
       annotationUI.selectTab(uiConstants.TAB_ANNOTATIONS);
     },
     focusAnnotations: function (tags) {
-      tags = tags || [];
-      var annotations = getAnnotationsByTags(tags);
-      annotationUI.focusAnnotations(annotations);
+      annotationUI.focusAnnotations(tags || []);
     },
     toggleAnnotationSelection: function (tags) {
-      tags = tags || [];
-      var annotations = getAnnotationsByTags(tags);
-      annotationUI.toggleSelectedAnnotations(annotations);
+      var ids = findIDsForTags(tags || []);
+      annotationUI.toggleSelectedAnnotations(ids);
     },
     setVisibleHighlights: function (state) {
       if (typeof state !== 'boolean') {

h/static/scripts/annotation-ui.js

@@ -20,6 +20,13 @@ function freeze(selection) {
   }
 }
 
+function toSet(list) {
+  return list.reduce(function (set, key) {
+    set[key] = true;
+    return set;
+  }, {});
+}
+
 function initialSelection(settings) {
   var selection = {};
   if (settings.annotations) {
@@ -197,17 +204,12 @@ module.exports = function (settings) {
     /**
      * Sets which annotations are currently focused.
      *
-     * @param {Array<Annotation>} annotations
+     * @param {Array<string>} Tags of annotations to focus
      */
-    focusAnnotations: function (annotations) {
-      var selection = {};
-      for (var i = 0, annotation; i < annotations.length; i++) {
-        annotation = annotations[i];
-        selection[annotation.$$tag] = true;
-      }
+    focusAnnotations: function (tags) {
       store.dispatch({
         type: types.FOCUS_ANNOTATIONS,
-        focused: freeze(selection),
+        focused: freeze(toSet(tags)),
       });
     },
 
@@ -258,28 +260,16 @@ module.exports = function (settings) {
 
     /**
      * Set the currently selected annotation IDs.
-     *
-     * @param {Array<string|{id:string}>} annotations - Annotations or IDs
-     *        of annotations to select.
      */
-    selectAnnotations: function (annotations) {
-      var selection = {};
-      for (var i = 0; i < annotations.length; i++) {
-        if (typeof annotations[i] === 'string') {
-          selection[annotations[i]] = true;
-        } else {
-          selection[annotations[i].id] = true;
-        }
-      }
-      select(selection);
+    selectAnnotations: function (ids) {
+      select(toSet(ids));
     },
 
     /** Toggle whether annotations are selected or not. */
-    toggleSelectedAnnotations: function (annotations) {
+    toggleSelectedAnnotations: function (ids) {
       var selection = Object.assign({}, store.getState().selectedAnnotationMap);
-      for (var i = 0, annotation; i < annotations.length; i++) {
-        annotation = annotations[i];
-        var id = annotation.id;
+      for (var i = 0; i < ids.length; i++) {
+        var id = ids[i];
         if (selection[id]) {
           delete selection[id];
         } else {
@@ -290,12 +280,13 @@ module.exports = function (settings) {
     },
 
     /** De-select an annotation. */
-    removeSelectedAnnotation: function (annotation) {
+    removeSelectedAnnotation: function (id) {
       var selection = Object.assign({}, store.getState().selectedAnnotationMap);
-      if (selection) {
-        delete selection[annotation.id];
-        select(selection);
+      if (!selection || !id) {
+        return;
       }
+      delete selection[id];
+      select(selection);
     },
 
     /** De-select all annotations. */

h/static/scripts/cross-frame.coffee

@@ -39,7 +39,7 @@ module.exports = class CrossFrame
       new AnnotationSync(bridge, options)
 
     createAnnotationUISync = (annotationSync) ->
-      new AnnotationUISync($rootScope, $window, bridge, annotationSync, annotationUI)
+      new AnnotationUISync($rootScope, $window, annotationUI, bridge)
 
     addFrame = (channel) =>
       channel.call 'getDocumentInfo', (err, info) =>

h/static/scripts/test/annotation-sync-test.coffee

@@ -58,20 +58,6 @@ describe 'AnnotationSync', ->
 
       assert.notCalled(channel.call)
 
-  describe '.getAnnotationForTag', ->
-    it 'returns the annotation if present in the cache', ->
-      ann = {id: 1, $$tag: 'tag1'}
-      annSync = createAnnotationSync()
-      annSync.cache['tag1'] = ann
-
-      cached = annSync.getAnnotationForTag('tag1')
-      assert.equal(cached, ann)
-
-    it 'returns null if not present in the cache', ->
-      annSync = createAnnotationSync()
-      cached = annSync.getAnnotationForTag('tag1')
-      assert.isNull(cached)
-
   describe 'channel event handlers', ->
     assertBroadcast = (channelEvent, publishEvent) ->
       it 'broadcasts the "' + publishEvent + '" event over the local event bus', ->

h/static/scripts/test/annotation-ui-controller-test.js

@@ -53,10 +53,7 @@ describe('AnnotationUIController', function () {
   });
 
   it('updates the focused annotations when the focus map changes', function () {
-    annotationUI.focusAnnotations([
-      {$$tag: '1'},
-      {$$tag: '2'},
-    ]);
+    annotationUI.focusAnnotations(['1', '2']);
     assert.deepEqual($scope.focusedAnnotations, { 1: true, 2: true });
   });
 

h/static/scripts/test/annotation-ui-sync-test.js

@@ -10,7 +10,6 @@ describe('AnnotationUISync', function () {
   var publish;
   var fakeBridge;
   var annotationUI;
-  var fakeAnnotationSync;
   var createAnnotationUISync;
   var createChannel = function () {
     return { call: sandbox.stub() };
@@ -43,20 +42,15 @@ describe('AnnotationUISync', function () {
       ]
     };
 
-    fakeAnnotationSync = {
-      getAnnotationForTag: function (tag) {
-        return {
-          id: Number(tag.replace('tag', '')),
-          $$tag: tag,
-        };
-      }
-    };
-
     annotationUI = annotationUIFactory({});
+    annotationUI.addAnnotations([
+      {id: 'id1', $$tag: 'tag1'},
+      {id: 'id2', $$tag: 'tag2'},
+      {id: 'id3', $$tag: 'tag3'},
+    ]);
     createAnnotationUISync = function () {
       new AnnotationUISync(
-        $rootScope, fakeWindow, fakeBridge, fakeAnnotationSync,
-        annotationUI
+        $rootScope, fakeWindow, annotationUI, fakeBridge
       );
     };
   }));
@@ -89,14 +83,18 @@ describe('AnnotationUISync', function () {
   });
 
   describe('on "showAnnotations" event', function () {
-    it('updates the annotationUI to include the shown annotations', function () {
+    it('selects annotations which have an ID', function () {
       createAnnotationUISync();
+      annotationUI.selectAnnotations = sinon.stub();
       publish('showAnnotations', ['tag1', 'tag2', 'tag3']);
-      assert.deepEqual(annotationUI.getState().selectedAnnotationMap, {
-        1: true,
-        2: true,
-        3: true,
-      });
+      assert.calledWith(annotationUI.selectAnnotations, ['id1', 'id2', 'id3']);
+    });
+
+    it('does not select annotations which do not have an ID', function () {
+      createAnnotationUISync();
+      annotationUI.selectAnnotations = sinon.stub();
+      publish('showAnnotations', ['tag1', 'tag-for-a-new-annotation']);
+      assert.calledWith(annotationUI.selectAnnotations, ['id1']);
     });
 
     it('triggers a digest', function () {
@@ -107,7 +105,7 @@ describe('AnnotationUISync', function () {
   });
 
   describe('on "focusAnnotations" event', function () {
-    it('updates the annotationUI to show the provided annotations', function () {
+    it('focuses the annotations', function () {
       createAnnotationUISync();
       publish('focusAnnotations', ['tag1', 'tag2', 'tag3']);
       assert.deepEqual(annotationUI.getState().focusedAnnotationMap, {
@@ -125,14 +123,11 @@ describe('AnnotationUISync', function () {
   });
 
   describe('on "toggleAnnotationSelection" event', function () {
-    it('updates the annotationUI to show the provided annotations', function () {
+    it('toggles the selected state of the annotations', function () {
       createAnnotationUISync();
+      annotationUI.toggleSelectedAnnotations = sinon.stub();
       publish('toggleAnnotationSelection', ['tag1', 'tag2', 'tag3']);
-      assert.deepEqual(annotationUI.getState().selectedAnnotationMap, {
-        1: true,
-        2: true,
-        3: true,
-      });
+      assert.calledWith(annotationUI.toggleSelectedAnnotations, ['id1', 'id2', 'id3']);
     });
 
     it('triggers a digest', function () {

h/static/scripts/test/annotation-ui-test.js

@@ -116,22 +116,22 @@ describe('annotationUI', function () {
 
   describe('#focusAnnotations()', function () {
     it('adds the passed annotations to the focusedAnnotationMap', function () {
-      annotationUI.focusAnnotations([{ $$tag: 1 }, { $$tag: 2 }, { $$tag: 3 }]);
+      annotationUI.focusAnnotations([1, 2, 3]);
       assert.deepEqual(annotationUI.getState().focusedAnnotationMap, {
         1: true, 2: true, 3: true
       });
     });
 
     it('replaces any annotations originally in the map', function () {
-      annotationUI.focusAnnotations([{ $$tag: 1 }]);
-      annotationUI.focusAnnotations([{ $$tag: 2 }, { $$tag: 3 }]);
+      annotationUI.focusAnnotations([1]);
+      annotationUI.focusAnnotations([2, 3]);
       assert.deepEqual(annotationUI.getState().focusedAnnotationMap, {
         2: true, 3: true
       });
     });
 
     it('nulls the map if no annotations are focused', function () {
-      annotationUI.focusAnnotations([{$$tag: 1}]);
+      annotationUI.focusAnnotations([1]);
       annotationUI.focusAnnotations([]);
       assert.isNull(annotationUI.getState().focusedAnnotationMap);
     });
@@ -139,7 +139,7 @@ describe('annotationUI', function () {
 
   describe('#hasSelectedAnnotations', function () {
     it('returns true if there are any selected annotations', function () {
-      annotationUI.selectAnnotations([{id: 1}]);
+      annotationUI.selectAnnotations([1]);
       assert.isTrue(annotationUI.hasSelectedAnnotations());
     });
 
@@ -150,12 +150,12 @@ describe('annotationUI', function () {
 
   describe('#isAnnotationSelected', function () {
     it('returns true if the id provided is selected', function () {
-      annotationUI.selectAnnotations([{id: 1}]);
+      annotationUI.selectAnnotations([1]);
       assert.isTrue(annotationUI.isAnnotationSelected(1));
     });
 
     it('returns false if the id provided is not selected', function () {
-      annotationUI.selectAnnotations([{id: 1}]);
+      annotationUI.selectAnnotations([1]);
       assert.isFalse(annotationUI.isAnnotationSelected(2));
     });
 
@@ -166,22 +166,22 @@ describe('annotationUI', function () {
 
   describe('#selectAnnotations()', function () {
     it('adds the passed annotations to the selectedAnnotationMap', function () {
-      annotationUI.selectAnnotations([{ id: 1 }, { id: 2 }, { id: 3 }]);
+      annotationUI.selectAnnotations([1, 2, 3]);
       assert.deepEqual(annotationUI.getState().selectedAnnotationMap, {
         1: true, 2: true, 3: true
       });
     });
 
     it('replaces any annotations originally in the map', function () {
-      annotationUI.selectAnnotations([{ id:1 }]);
-      annotationUI.selectAnnotations([{ id: 2 }, { id: 3 }]);
+      annotationUI.selectAnnotations([1]);
+      annotationUI.selectAnnotations([2, 3]);
       assert.deepEqual(annotationUI.getState().selectedAnnotationMap, {
         2: true, 3: true
       });
     });
 
     it('nulls the map if no annotations are selected', function () {
-      annotationUI.selectAnnotations([{id:1}]);
+      annotationUI.selectAnnotations([1]);
       annotationUI.selectAnnotations([]);
       assert.isNull(annotationUI.getState().selectedAnnotationMap);
     });
@@ -189,45 +189,45 @@ describe('annotationUI', function () {
 
   describe('#toggleSelectedAnnotations()', function () {
     it('adds annotations missing from the selectedAnnotationMap', function () {
-      annotationUI.selectAnnotations([{ id: 1 }, { id: 2}]);
-      annotationUI.toggleSelectedAnnotations([{ id: 3 }, { id: 4 }]);
+      annotationUI.selectAnnotations([1, 2]);
+      annotationUI.toggleSelectedAnnotations([3, 4]);
       assert.deepEqual(annotationUI.getState().selectedAnnotationMap, {
         1: true, 2: true, 3: true, 4: true
       });
     });
 
     it('removes annotations already in the selectedAnnotationMap', function () {
-      annotationUI.selectAnnotations([{id: 1}, {id: 3}]);
-      annotationUI.toggleSelectedAnnotations([{ id: 1 }, { id: 2 }]);
+      annotationUI.selectAnnotations([1, 3]);
+      annotationUI.toggleSelectedAnnotations([1, 2]);
       assert.deepEqual(annotationUI.getState().selectedAnnotationMap, { 2: true, 3: true });
     });
 
     it('nulls the map if no annotations are selected', function () {
-      annotationUI.selectAnnotations([{id: 1}]);
-      annotationUI.toggleSelectedAnnotations([{ id: 1 }]);
+      annotationUI.selectAnnotations([1]);
+      annotationUI.toggleSelectedAnnotations([1]);
       assert.isNull(annotationUI.getState().selectedAnnotationMap);
     });
   });
 
   describe('#removeSelectedAnnotation()', function () {
     it('removes an annotation from the selectedAnnotationMap', function () {
-      annotationUI.selectAnnotations([{id: 1}, {id: 2}, {id: 3}]);
-      annotationUI.removeSelectedAnnotation({ id: 2 });
+      annotationUI.selectAnnotations([1, 2, 3]);
+      annotationUI.removeSelectedAnnotation(2);
       assert.deepEqual(annotationUI.getState().selectedAnnotationMap, {
         1: true, 3: true
       });
     });
 
     it('nulls the map if no annotations are selected', function () {
-      annotationUI.selectAnnotations([{id: 1}]);
-      annotationUI.removeSelectedAnnotation({ id: 1 });
+      annotationUI.selectAnnotations([1]);
+      annotationUI.removeSelectedAnnotation(1);
       assert.isNull(annotationUI.getState().selectedAnnotationMap);
     });
   });
 
   describe('#clearSelectedAnnotations()', function () {
     it('removes all annotations from the selection', function () {
-      annotationUI.selectAnnotations([{id: 1}]);
+      annotationUI.selectAnnotations([1]);
       annotationUI.clearSelectedAnnotations();
       assert.isNull(annotationUI.getState().selectedAnnotationMap);
     });