Merge pull request #3108 from hypothesis/256-focus-selected-annot-on-load

Switch to appropriate group and scroll to direct linked annotation when sidebar loads

h/static/scripts/directive/test/markdown-test.js

@@ -4,19 +4,7 @@ var angular = require('angular');
 var proxyquire = require('proxyquire');
 
 var util = require('./util');
-
-/**
- * Disable calling through to the original module for a stub.
- *
- * By default proxyquire will call through to the original module
- * for any methods not provided by a stub. This function disables
- * this behavior for a stub and returns the input stub.
- *
- * This prevents unintended usage of the original dependency.
- */
-function noCallThru(stub) {
-  return Object.assign(stub, {'@noCallThru':true});
-}
+var noCallThru = require('../../test/util').noCallThru;
 
 describe('markdown', function () {
   function isHidden(element) {
@@ -49,8 +37,8 @@ describe('markdown', function () {
 
   before(function () {
     angular.module('app', ['ngSanitize'])
-      .directive('markdown', proxyquire('../markdown', {
-        angular: noCallThru(angular),
+      .directive('markdown', proxyquire('../markdown', noCallThru({
+        angular: angular,
         katex: {
           renderToString: function (input) {
             return 'math:' + input.replace(/$$/g, '');
@@ -62,8 +50,7 @@ describe('markdown', function () {
           toggleSpanStyle: mockFormattingCommand,
           LinkType: require('../../markdown-commands').LinkType,
         },
-        '@noCallThru': true,
-      }))
+      })))
       .filter('converter', function () {
         return function (input) {
           return 'rendered:' + input;

h/static/scripts/events.js

+'use strict';
+
 /**
  * This module defines the set of global events that are dispatched
  * on $rootScope
  */
 module.exports = {
-  /** Broadcast when the currently selected group changes */
-  GROUP_FOCUSED: 'groupFocused',
-  /** Broadcast when the list of groups changes */
+  // Session state changes
+
+  /** The list of groups changed */
   GROUPS_CHANGED: 'groupsChanged',
-  /** Broadcast when the signed-in user changes */
+  /** The signed-in user changed */
   USER_CHANGED: 'userChanged',
-  /** Broadcast when the session state is updated.
-   * This event is NOT broadcast after the initial session load.
+  /**
+   * The session state was updated.
    */
   SESSION_CHANGED: 'sessionChanged',
+
+  // UI state changes
+
+  /** The currently selected group changed */
+  GROUP_FOCUSED: 'groupFocused',
+
+  // Annotation events
+
+  /** A new annotation has been created locally. */
+  BEFORE_ANNOTATION_CREATED: 'beforeAnnotationCreated',
+
+  /** Annotations were anchored in a connected document. */
+  ANNOTATIONS_SYNCED: 'sync',
+
+  /** An annotation was created on the server and assigned an ID. */
+  ANNOTATION_CREATED: 'annotationCreated',
+
+  /** An annotation was either deleted or unloaded. */
+  ANNOTATION_DELETED: 'annotationDeleted',
+
+  /** A set of annotations were loaded from the server. */
+  ANNOTATIONS_LOADED: 'annotationsLoaded',
 };

h/static/scripts/groups.js

@@ -25,7 +25,7 @@ function groups(localStorage, session, settings, $rootScope, $http) {
 
   function all() {
     return session.state.groups || [];
-  };
+  }
 
   // Return the full object for the group with the given id.
   function get(id) {
@@ -35,7 +35,7 @@ function groups(localStorage, session, settings, $rootScope, $http) {
         return gs[i];
       }
     }
-  };
+  }
 
   /** Leave the group with the given ID.
    * Returns a promise which resolves when the action completes.
@@ -51,7 +51,7 @@ function groups(localStorage, session, settings, $rootScope, $http) {
     // by optimistically updating the session state
 
     return response;
-  };
+  }
 
 
   /** Return the currently focused group. If no group is explicitly focused we
@@ -72,13 +72,16 @@ function groups(localStorage, session, settings, $rootScope, $http) {
 
   /** Set the group with the passed id as the currently focused group. */
   function focus(id) {
+    var prevFocused = focused();
     var g = get(id);
     if (g) {
       focusedGroup = g;
       localStorage.setItem(STORAGE_KEY, g.id);
+      if (prevFocused.id !== g.id) {
         $rootScope.$broadcast(events.GROUP_FOCUSED, g.id);
       }
     }
+  }
 
   // reset the focused group if the user leaves it
   $rootScope.$on(events.GROUPS_CHANGED, function () {

h/static/scripts/polyfills.js

@@ -2,6 +2,7 @@
 
 // ES2015 polyfills
 require('core-js/es6/promise');
+require('core-js/fn/array/find');
 require('core-js/fn/object/assign');
 
 // URL constructor, required by IE 10/11,

h/static/scripts/search-client.js

+'use strict';
+
+var EventEmitter = require('tiny-emitter');
+var inherits = require('inherits');
+
+/**
+ * Client for the Hypothesis search API.
+ *
+ * SearchClient handles paging through results, canceling search etc.
+ *
+ * @param {Object} resource - ngResource class instance for the /search API
+ * @param {Object} opts - Search options
+ * @constructor
+ */
+function SearchClient(resource, opts) {
+  opts = opts || {};
+
+  var DEFAULT_CHUNK_SIZE = 200;
+  this._resource = resource;
+  this._chunkSize = opts.chunkSize || DEFAULT_CHUNK_SIZE;
+  if (typeof opts.incremental !== 'undefined') {
+    this._incremental = opts.incremental;
+  } else {
+    this._incremental = true;
+  }
+  this._canceled = false;
+}
+inherits(SearchClient, EventEmitter);
+
+SearchClient.prototype._getBatch = function (query, offset) {
+  var searchQuery = Object.assign({
+    limit: this._chunkSize,
+    offset: offset,
+    sort: 'created',
+    order: 'asc',
+    _separate_replies: true,
+  }, query);
+
+  var self = this;
+  this._resource.get(searchQuery).$promise.then(function (results) {
+    if (self._canceled) {
+      return;
+    }
+
+    var chunk = results.rows.concat(results.replies || []);
+    if (self._incremental) {
+      self.emit('results', chunk);
+    } else {
+      self._results = self._results.concat(chunk);
+    }
+
+    var nextOffset = offset + results.rows.length;
+    if (results.total > nextOffset) {
+      self._getBatch(query, nextOffset);
+    } else {
+      if (!self._incremental) {
+        self.emit('results', self._results);
+      }
+      self.emit('end');
+    }
+  }).catch(function (err) {
+    if (self._canceled) {
+      return;
+    }
+    self.emit('error', err);
+  }).then(function () {
+    if (self._canceled) {
+      return;
+    }
+    self.emit('end');
+  });
+};
+
+/**
+ * Perform a search against the Hypothesis API.
+ *
+ * Emits a 'results' event with an array of annotations as they become
+ * available (in incremental mode) or when all annotations are available
+ * (in non-incremental mode).
+ *
+ * Emits an 'error' event if the search fails.
+ * Emits an 'end' event once the search completes.
+ */
+SearchClient.prototype.get = function (query) {
+  this._results = [];
+  this._getBatch(query, 0);
+};
+
+/**
+ * Cancel the current search and emit the 'end' event.
+ * No further events will be emitted after this.
+ */
+SearchClient.prototype.cancel = function () {
+  this._canceled = true;
+  this.emit('end');
+};
+
+module.exports = SearchClient;

h/static/scripts/test/groups-test.js

@@ -45,7 +45,7 @@ describe('groups', function() {
         }
       }
     };
-    fakeHttp = sandbox.stub()
+    fakeHttp = sandbox.stub();
   });
 
   afterEach(function () {
@@ -134,7 +134,7 @@ describe('groups', function() {
     });
   });
 
-  describe('.focus() method', function() {
+  describe('.focus()', function() {
     it('sets the focused group to the named group', function() {
       var s = service();
       s.focus('id2');
@@ -155,6 +155,20 @@ describe('groups', function() {
 
       assert.calledWithMatch(fakeLocalStorage.setItem, sinon.match.any, 'id3');
     });
+
+    it('emits the GROUP_FOCUSED event if the focused group changed', function () {
+      var s = service();
+      s.focus('id3');
+      assert.calledWith(fakeRootScope.$broadcast, events.GROUP_FOCUSED, 'id3');
+    });
+
+    it('does not emit GROUP_FOCUSED if the focused group did not change', function () {
+      var s = service();
+      s.focus('id3');
+      fakeRootScope.$broadcast = sinon.stub();
+      s.focus('id3');
+      assert.notCalled(fakeRootScope.$broadcast);
+    });
   });
 
   describe('.leave()', function () {

h/static/scripts/test/raven-test.js

 'use strict';
 
 var proxyquire = require('proxyquire');
-
-function noCallThru(stub) {
-  return Object.assign(stub, {'@noCallThru':true});
-}
+var noCallThru = require('./util').noCallThru;
 
 function fakeExceptionData(scriptURL) {
   return {
@@ -51,10 +48,10 @@ describe('raven', function () {
       Raven.setDataCallback(fakeAngularTransformer);
     });
 
-    raven = proxyquire('../raven', {
-      'raven-js': noCallThru(fakeRavenJS),
-      'raven-js/plugins/angular': noCallThru(fakeAngularPlugin),
-    });
+    raven = proxyquire('../raven', noCallThru({
+      'raven-js': fakeRavenJS,
+      'raven-js/plugins/angular': fakeAngularPlugin,
+    }));
   });
 
   describe('.install()', function () {

h/static/scripts/test/search-client-test.js

+'use strict';
+
+var SearchClient = require('../search-client');
+
+function await(emitter, event) {
+  return new Promise(function (resolve) {
+    emitter.on(event, resolve);
+  });
+}
+
+describe('SearchClient', function () {
+  var RESULTS = [
+    {id: 'one'},
+    {id: 'two'},
+    {id: 'three'},
+    {id: 'four'},
+  ];
+
+  var fakeResource;
+
+  beforeEach(function () {
+    fakeResource = {
+      get: sinon.spy(function (params) {
+        return {
+          $promise: Promise.resolve({
+            rows: RESULTS.slice(params.offset,
+              params.offset + params.limit),
+            total: RESULTS.length,
+          }),
+        };
+      }),
+    };
+  });
+
+  it('emits "results"', function () {
+    var client = new SearchClient(fakeResource);
+    var onResults = sinon.stub();
+    client.on('results', onResults);
+    client.get({uri: 'http://example.com'});
+    return await(client, 'end').then(function () {
+      assert.calledWith(onResults, RESULTS);
+    });
+  });
+
+  it('emits "results" with chunks in incremental mode', function () {
+    var client = new SearchClient(fakeResource, {chunkSize: 2});
+    var onResults = sinon.stub();
+    client.on('results', onResults);
+    client.get({uri: 'http://example.com'});
+    return await(client, 'end').then(function () {
+      assert.calledWith(onResults, RESULTS.slice(0,2));
+      assert.calledWith(onResults, RESULTS.slice(2,4));
+    });
+  });
+
+  it('emits "results" once in non-incremental mode', function () {
+    var client = new SearchClient(fakeResource,
+      {chunkSize: 2, incremental: false});
+    var onResults = sinon.stub();
+    client.on('results', onResults);
+    client.get({uri: 'http://example.com'});
+    return await(client, 'end').then(function () {
+      assert.calledOnce(onResults);
+      assert.calledWith(onResults, RESULTS);
+    });
+  });
+
+  it('does not emit "results" if canceled', function () {
+    var client = new SearchClient(fakeResource);
+    var onResults = sinon.stub();
+    var onEnd = sinon.stub();
+    client.on('results', onResults);
+    client.on('end', onEnd);
+    client.get({uri: 'http://example.com'});
+    client.cancel();
+    return Promise.resolve().then(function () {
+      assert.notCalled(onResults);
+      assert.called(onEnd);
+    });
+  });
+
+  it('emits "error" event if search fails', function () {
+    var err = new Error('search failed');
+    fakeResource.get = function () {
+      return {
+        $promise: Promise.reject(err),
+      };
+    };
+    var client = new SearchClient(fakeResource);
+    var onError = sinon.stub();
+    client.on('error', onError);
+    client.get({uri: 'http://example.com'});
+    return await(client, 'end').then(function () {
+      assert.calledWith(onError, err);
+    });
+  });
+});

h/static/scripts/test/util.js

+'use strict';
+
+/**
+ * Utility function for use with 'proxyquire' that prevents calls to
+ * stubs 'calling through' to the _original_ dependency if a particular
+ * function or property is not set on a stub, which is proxyquire's default
+ * but usually undesired behavior.
+ *
+ * See https://github.com/thlorenz/proxyquireify#nocallthru
+ *
+ * Usage:
+ *   var moduleUnderTest = proxyquire('./module-under-test', noCallThru({
+ *     './dependency-foo': fakeFoo,
+ *   }));
+ *
+ * @param {Object} stubs - A map of dependency paths to stubs, or a single
+ *   stub.
+ */
+function noCallThru(stubs) {
+  // This function is trivial but serves as documentation for why
+  // '@noCallThru' is used.
+  return Object.assign(stubs, {'@noCallThru':true});
+}
+
+module.exports = {
+  noCallThru: noCallThru,
+};

h/static/scripts/threading.coffee

 angular = require('angular')
 mail = require('./vendor/jwz')
+events = require('./events')
 
 # The threading service provides the model for the currently loaded
 # set of annotations, structured as a tree of annotations and replies.
@@ -31,10 +32,10 @@ module.exports = class Threading
 
     # Create a root container.
     @root = mail.messageContainer()
-    $rootScope.$on('beforeAnnotationCreated', this.beforeAnnotationCreated)
-    $rootScope.$on('annotationCreated', this.annotationCreated)
-    $rootScope.$on('annotationDeleted', this.annotationDeleted)
-    $rootScope.$on('annotationsLoaded', this.annotationsLoaded)
+    $rootScope.$on(events.BEFORE_ANNOTATION_CREATED, this.beforeAnnotationCreated)
+    $rootScope.$on(events.ANNOTATION_CREATED, this.annotationCreated)
+    $rootScope.$on(events.ANNOTATION_DELETED, this.annotationDeleted)
+    $rootScope.$on(events.ANNOTATIONS_LOADED, this.annotationsLoaded)
 
   # TODO: Refactor the jwz API for progressive updates.
   # Right now the idTable is wiped when `messageThread.thread()` is called and

h/static/scripts/widget-controller.js

 'use strict';
 
-var angular = require('angular');
-
 var events = require('./events');
+var SearchClient = require('./search-client');
+
+/**
+ * Returns the group ID of the first annotation in `results` whose
+ * ID is a key in `selection`.
+ */
+function groupIDFromSelection(selection, results) {
+  var id = Object.keys(selection)[0];
+  var annot = results.find(function (annot) {
+    return annot.id === id;
+  });
+  if (!annot) {
+    return;
+  }
+  return annot.group;
+}
 
 // @ngInject
 module.exports = function WidgetController(
@@ -12,78 +26,165 @@ module.exports = function WidgetController(
   $scope.threadRoot = threading.root;
   $scope.sortOptions = ['Newest', 'Oldest', 'Location'];
 
-  var DEFAULT_CHUNK_SIZE = 200;
-  var loaded = [];
+  function focusAnnotation(annotation) {
+    var highlights = [];
+    if (annotation) {
+      highlights = [annotation.$$tag];
+    }
+    crossframe.call('focusAnnotations', highlights);
+  }
+
+  function scrollToAnnotation(annotation) {
+    if (!annotation) {
+      return;
+    }
+    crossframe.call('scrollToAnnotation', annotation.$$tag);
+  }
 
-  var _resetAnnotations = function () {
+  /**
+   * Returns the Annotation object for the first annotation in the
+   * selected annotation set. Note that 'first' refers to the order
+   * of annotations passed to annotationUI when selecting annotations,
+   * not the order in which they appear in the document.
+   */
+  function firstSelectedAnnotation() {
+    if (annotationUI.selectedAnnotationMap) {
+      var id = Object.keys(annotationUI.selectedAnnotationMap)[0];
+      return threading.idTable[id] && threading.idTable[id].message;
+    } else {
+      return null;
+    }
+  }
+
+  function _resetAnnotations() {
     // Unload all the annotations
     annotationMapper.unloadAnnotations(threading.annotationList());
     // Reload all the drafts
     threading.thread(drafts.unsaved());
-  };
+  }
 
-  var _loadAnnotationsFrom = function (query, offset) {
-    var queryCore = {
-      limit: $scope.chunkSize || DEFAULT_CHUNK_SIZE,
-      offset: offset,
-      sort: 'created',
-      order: 'asc',
-      group: groups.focused().id
-    };
-    var q = angular.extend(queryCore, query);
-    q._separate_replies = true;
+  var searchClients = [];
 
-    store.SearchResource.get(q, function (results) {
-      var total = results.total;
-      offset += results.rows.length;
-      if (offset < total) {
-        _loadAnnotationsFrom(query, offset);
+  function _loadAnnotationsFor(uri, group) {
+    var searchClient = new SearchClient(store.SearchResource, {
+      // If no group is specified, we are fetching annotations from
+      // all groups in order to find out which group contains the selected
+      // annotation, therefore we need to load all chunks before processing
+      // the results
+      incremental: !!group,
+    });
+    searchClients.push(searchClient);
+    searchClient.on('results', function (results) {
+      if (annotationUI.hasSelectedAnnotations()) {
+        // Focus the group containing the selected annotation and filter
+        // annotations to those from this group
+        var groupID = groupIDFromSelection(annotationUI.selectedAnnotationMap,
+          results);
+        if (!groupID) {
+          // If the selected annotation is not available, fall back to
+          // loading annotations for the currently focused group
+          groupID = groups.focused().id;
+        }
+        results = results.filter(function (result) {
+          return result.group === groupID;
+        });
+        groups.focus(groupID);
       }
 
-      annotationMapper.loadAnnotations(results.rows, results.replies);
+      if (results.length) {
+        annotationMapper.loadAnnotations(results);
+      }
+    });
+    searchClient.on('end', function () {
+      // Remove client from list of active search clients
+      searchClients.splice(searchClients.indexOf(searchClient), 1);
+    });
+    searchClient.get({uri: uri, group: group});
+  }
+
+  /**
+   * Load annotations for all URLs associated with @p frames.
+   *
+   * @param {Array<{uri:string}>} frames - Hypothesis client frames
+   *        to load annotations for.
+   */
+  function loadAnnotations(frames) {
+    _resetAnnotations();
+
+    searchClients.forEach(function (client) {
+      client.cancel();
     });
-  };
 
-  var loadAnnotations = function (frames) {
-    for (var i = 0, f; i < frames.length; i++) {
-      f = frames[i];
-      var ref;
-      if (ref = f.uri, loaded.indexOf(ref) >= 0) {
-        continue;
+    var urls = frames.reduce(function (urls, frame) {
+      if (urls.indexOf(frame.uri) !== -1) {
+        return urls;
+      } else {
+        return urls.concat(frame.uri);
       }
-      loaded.push(f.uri);
-      _loadAnnotationsFrom({uri: f.uri}, 0);
+    }, []);
+
+    // If there is no selection, load annotations only for the focused group.
+    //
+    // If there is a selection, we load annotations for all groups, find out
+    // which group the first selected annotation is in and then filter the
+    // results on the client by that group.
+    //
+    // In the common case where the total number of annotations on
+    // a page that are visible to the user is not greater than
+    // the batch size, this saves an extra roundtrip to the server
+    // to fetch the selected annotation in order to determine which group
+    // it is in before fetching the remaining annotations.
+    var group = annotationUI.hasSelectedAnnotations() ?
+      null : groups.focused().id;
+
+    for (var i=0; i < urls.length; i++) {
+      _loadAnnotationsFor(urls[i], group);
     }
 
-    if (loaded.length > 0) {
-      streamFilter.resetFilter().addClause('/uri', 'one_of', loaded);
+    if (urls.length > 0) {
+      streamFilter.resetFilter().addClause('/uri', 'one_of', urls);
       streamer.setConfig('filter', {filter: streamFilter.getFilter()});
     }
-  };
+  }
+
+  // When a direct-linked annotation is successfully anchored in the page,
+  // focus and scroll to it
+  $rootScope.$on(events.ANNOTATIONS_SYNCED, function (event, tags) {
+    var selectedAnnot = firstSelectedAnnotation();
+    if (!selectedAnnot) {
+      return;
+    }
+    var matchesSelection = tags.some(function (tag) {
+      return tag.tag === selectedAnnot.$$tag;
+    });
+    if (!matchesSelection) {
+      return;
+    }
+    focusAnnotation(selectedAnnot);
+    scrollToAnnotation(selectedAnnot);
+  });
 
   $scope.$on(events.GROUP_FOCUSED, function () {
-    _resetAnnotations(annotationMapper, drafts, threading);
-    loaded = [];
+    // The focused group may be changed during loading annotations (in which
+    // case, searchClients.length > 0), as a result of switching to the group
+    // containing the selected annotation.
+    //
+    // In that case, we don't want to trigger reloading annotations again.
+    if (searchClients.length) {
+      return;
+    }
+
+    annotationUI.clearSelectedAnnotations();
     return loadAnnotations(crossframe.frames);
   });
 
+  // Watch anything that may require us to reload annotations.
   $scope.$watchCollection(function () {
     return crossframe.frames;
   }, loadAnnotations);
 
-  $scope.focus = function (annotation) {
-    var highlights = [];
-    if (angular.isObject(annotation)) {
-      highlights = [annotation.$$tag];
-    }
-    return crossframe.call('focusAnnotations', highlights);
-  };
-
-  $scope.scrollTo = function (annotation) {
-    if (angular.isObject(annotation)) {
-      return crossframe.call('scrollToAnnotation', annotation.$$tag);
-    }
-  };
+  $scope.focus = focusAnnotation;
+  $scope.scrollTo = scrollToAnnotation;
 
   $scope.hasFocus = function (annotation) {
     if (!annotation || !$scope.focusedAnnotations) {
@@ -92,7 +193,7 @@ module.exports = function WidgetController(
     return annotation.$$tag in $scope.focusedAnnotations;
   };
 
-  $rootScope.$on('beforeAnnotationCreated', function (event, data) {
+  $rootScope.$on(events.BEFORE_ANNOTATION_CREATED, function (event, data) {
     if (data.$highlight || (data.references && data.references.length > 0)) {
       return;
     }