Convert filters used only once in the annotation directive to plain f…

…unctions The annotation directive used a set of filters for formatting document domains, titles and usernames. Converting these to plain functions eliminates a bunch of boilerplate and as noted at https://www.binpress.com/tutorial/speeding-up-angular-js-with-simple-optimizations/135 is a small performance optimization.
hypothesis · Feb 15, 2016 · 4abe02f · 4abe02f
1 parent 2b10317
commit 4abe02f
Show file tree

Hide file tree

Showing 12 changed files with 114 additions and 157 deletions.
diff --git a/h/static/scripts/app.coffee b/h/static/scripts/app.coffee
@@ -113,10 +113,6 @@ module.exports = angular.module('h', [
 .directive('topBar', require('./directive/top-bar'))
 
 .filter('converter', require('./filter/converter'))
-.filter('persona', require('./filter/persona').filter)
-.filter('urlencode', require('./filter/urlencode'))
-.filter('documentTitle', require('./filter/document-title'))
-.filter('documentDomain', require('./filter/document-domain'))
 
 .provider('identity', require('./identity'))
 

diff --git a/h/static/scripts/directive/annotation.js b/h/static/scripts/directive/annotation.js
@@ -2,7 +2,10 @@
 'use strict';
 
 var dateUtil = require('../date-util');
+var documentDomain = require('../filter/document-domain');
+var documentTitle = require('../filter/document-title');
 var events = require('../events');
+var persona = require('../filter/persona');
 
 /** Return a domainModel tags array from the given vm tags array.
  *
@@ -190,6 +193,10 @@ function updateViewModel($scope, time, domainModel, vm, permissions) {
      });
     updateTimestamp();
   }
+
+  var documentMetadata = extractDocumentMetadata(domainModel);
+  vm.documentTitle = documentTitle(documentMetadata);
+  vm.documentDomain = documentDomain(documentMetadata);
 }
 
 /** Return a vm tags array from the given domainModel tags array.
@@ -463,11 +470,6 @@ function AnnotationController(
     }, true);
   };
 
-  /** Return some metadata extracted from the annotated document. */
-  vm.document = function() {
-    return extractDocumentMetadata(domainModel);
-  };
-
   /**
     * @ngdoc method
     * @name annotation.AnnotationController#edit
@@ -728,6 +730,10 @@ function AnnotationController(
     return $q.when(tags.filter(query));
   };
 
+  vm.tagStreamURL = function(tag) {
+    return vm.serviceUrl + 'stream?q=tag:' + encodeURIComponent(tag);
+  };
+
   vm.target = function() {
     return domainModel.target;
   };
@@ -738,7 +744,11 @@ function AnnotationController(
 
   vm.user = function() {
     return domainModel.user;
-  }
+  };
+
+  vm.username = function() {
+    return persona.username(domainModel.user);
+  };
 
   /** Sets whether or not the controls for
    * expanding/collapsing the body of lengthy annotations

diff --git a/h/static/scripts/directive/test/annotation-test.js b/h/static/scripts/directive/test/annotation-test.js
@@ -533,7 +533,7 @@ describe('annotation', function() {
     }
 
     before(function() {
-      angular.module('h', [])
+      angular.module('h', ['ngSanitize'])
         .directive('annotation', require('../annotation').directive);
     });
 
@@ -588,16 +588,6 @@ describe('annotation', function() {
         setDefault: sandbox.stub()
       };
 
-      fakePersonaFilter = sandbox.stub().returnsArg(0);
-
-      fakeDocumentTitleFilter = function(arg) {
-        return '';
-      };
-
-      fakeDocumentDomainFilter = function(arg) {
-        return '';
-      };
-
       fakeSession = {
         state: {
           userid: 'acct:bill@localhost'
@@ -618,10 +608,6 @@ describe('annotation', function() {
         decayingInterval: function () {},
       };
 
-      fakeUrlEncodeFilter = function(v) {
-        return encodeURIComponent(v);
-      };
-
       fakeGroups = {
         focused: function() {
           return {};
@@ -636,14 +622,10 @@ describe('annotation', function() {
       $provide.value('flash', fakeFlash);
       $provide.value('momentFilter', fakeMomentFilter);
       $provide.value('permissions', fakePermissions);
-      $provide.value('personaFilter', fakePersonaFilter);
-      $provide.value('documentTitleFilter', fakeDocumentTitleFilter);
-      $provide.value('documentDomainFilter', fakeDocumentDomainFilter);
       $provide.value('session', fakeSession);
       $provide.value('settings', fakeSettings);
       $provide.value('tags', fakeTags);
       $provide.value('time', fakeTime);
-      $provide.value('urlencodeFilter', fakeUrlEncodeFilter);
       $provide.value('groups', fakeGroups);
     }));
 
@@ -1595,7 +1577,7 @@ describe('annotation', function() {
 
   describe('AnnotationController', function() {
     before(function() {
-      angular.module('h', [])
+      angular.module('h', ['ngSanitize'])
         .directive('annotation', require('../annotation').directive);
     });
 

diff --git a/h/static/scripts/filter/document-domain.js b/h/static/scripts/filter/document-domain.js
@@ -2,34 +2,31 @@
 
 var escapeHtml = require('escape-html');
 
-module.exports = function() {
-  /**
-   * Return a nice displayable string representation of a document's domain.
-   *
-   * @returns {String} The document's domain in braces, e.g. '(example.com)'.
-   *   Returns '' if the document has no domain or if the document's domain is
-   *   the same as its title (because we assume that the title is already
-   *   displayed elsewhere and displaying it twice would be redundant).
-   *
-   */
-  function documentDomainFilter(document) {
-    var uri = escapeHtml(document.uri || '');
-    var domain = escapeHtml(document.domain || '');
-    var title = escapeHtml(document.title || '');
+/**
+ * Return a nice displayable string representation of a document's domain.
+ *
+ * @returns {String} The document's domain in braces, e.g. '(example.com)'.
+ *   Returns '' if the document has no domain or if the document's domain is
+ *   the same as its title (because we assume that the title is already
+ *   displayed elsewhere and displaying it twice would be redundant).
+ *
+ */
+module.exports = function documentDomain(document) {
+  var uri = escapeHtml(document.uri || '');
+  var domain = escapeHtml(document.domain || '');
+  var title = escapeHtml(document.title || '');
 
-    if (uri.indexOf('file://') === 0 && title) {
-      var parts = uri.split('/');
-      var filename = parts[parts.length - 1];
-      if (filename) {
-        return '(' + decodeURIComponent(filename) + ')';
-      }
+  if (uri.indexOf('file://') === 0 && title) {
+    var parts = uri.split('/');
+    var filename = parts[parts.length - 1];
+    if (filename) {
+      return '(' + decodeURIComponent(filename) + ')';
     }
+  }
 
-    if (domain && domain !== title) {
-      return '(' + decodeURIComponent(domain) + ')';
-    } else {
-      return '';
-    }
+  if (domain && domain !== title) {
+    return '(' + decodeURIComponent(domain) + ')';
+  } else {
+    return '';
   }
-  return documentDomainFilter;
 };
diff --git a/h/static/scripts/filter/document-title.js b/h/static/scripts/filter/document-title.js
@@ -2,32 +2,29 @@
 
 var escapeHtml  = require('escape-html');
 
-module.exports = function() {
-  /**
-   * Return a nice displayable string representation of a document's title.
-   *
-   * @returns {String} The document's title preceded on "on " and hyperlinked
-   *   to the document's URI. If the document has no http(s) URI then don't
-   *   hyperlink the title. If the document has no title then return ''.
-   *
-   */
-  function documentTitleFilter(document) {
-    var title = escapeHtml(document.title || '');
-    var uri = escapeHtml(document.uri || '');
+/**
+ * Return a nice displayable string representation of a document's title.
+ *
+ * @returns {String} The document's title preceded on "on " and hyperlinked
+ *   to the document's URI. If the document has no http(s) URI then don't
+ *   hyperlink the title. If the document has no title then return ''.
+ *
+ */
+module.exports = function documentTitle(document) {
+  var title = escapeHtml(document.title || '');
+  var uri = escapeHtml(document.uri || '');
 
-    if (uri && !(uri.indexOf('http://') === 0 || uri.indexOf('https://') === 0)) {
-      // We only link to http(s) URLs.
-      uri = null;
-    }
+  if (uri && !(uri.indexOf('http://') === 0 || uri.indexOf('https://') === 0)) {
+    // We only link to http(s) URLs.
+    uri = null;
+  }
 
-    if (title && uri) {
-      return ('on &ldquo;<a target="_blank" href="' + uri + '">' + title +
-              '</a>&rdquo;');
-    } else if (title) {
-      return 'on &ldquo;' + title + '&rdquo;';
-    } else {
-      return '';
-    }
+  if (title && uri) {
+    return ('on &ldquo;<a target="_blank" href="' + uri + '">' + title +
+            '</a>&rdquo;');
+  } else if (title) {
+    return 'on &ldquo;' + title + '&rdquo;';
+  } else {
+    return '';
   }
-  return documentTitleFilter;
 };
diff --git a/h/static/scripts/filter/persona.js b/h/static/scripts/filter/persona.js
@@ -17,17 +17,18 @@ function parseAccountID(user) {
   };
 }
 
+/**
+ * Returns the username part of an account ID or an empty string.
+ */
+function username(user) {
+  var account = parseAccountID(user);
+  if (!account) {
+    return '';
+  }
+  return account.username;
+}
+
 module.exports = {
   parseAccountID: parseAccountID,
-
-  /** Export parseAccountID() as an Angular filter */
-  filter: function () {
-    return function (user, part) {
-      var account = parseAccountID(user);
-      if (!account) {
-        return null;
-      }
-      return account[part || 'username'];
-    };
-  }
+  username: username,
 };
diff --git a/h/static/scripts/filter/test/document-domain-test.js b/h/static/scripts/filter/test/document-domain-test.js
@@ -1,19 +1,19 @@
 'use strict';
 
-var documentDomainFilterProvider = require('../document-domain');
+var documentDomain = require('../document-domain');
 
 describe('documentDomain', function() {
 
   it('returns the domain in braces', function() {
-    var domain = documentDomainFilterProvider()({
+    var domain = documentDomain({
       domain: 'example.com'
     });
 
     assert(domain === '(example.com)');
   });
 
   it('returns an empty string if domain and title are the same', function() {
-    var domain = documentDomainFilterProvider()({
+    var domain = documentDomain({
       domain: 'example.com',
       title: 'example.com'
     });
@@ -22,15 +22,15 @@ describe('documentDomain', function() {
   });
 
   it('returns an empty string if the document has no domain', function() {
-    var domain = documentDomainFilterProvider()({
+    var domain = documentDomain({
       title: 'example.com'
     });
 
     assert(domain === '');
   });
 
   it('returns the filename for local documents with titles', function() {
-    var domain = documentDomainFilterProvider()({
+    var domain = documentDomain({
       title: 'example.com',
       uri: 'file:///home/seanh/MyFile.pdf'
     });
@@ -39,7 +39,7 @@ describe('documentDomain', function() {
   });
 
   it('replaces %20 with " "', function() {
-    var domain = documentDomainFilterProvider()({
+    var domain = documentDomain({
       title: 'example.com',
       uri: 'file:///home/seanh/My%20File.pdf'
     });
@@ -50,7 +50,7 @@ describe('documentDomain', function() {
   it('escapes HTML in the document domain', function() {
     var spamLink = '<a href="http://example.com/rubies">Buy rubies!!!</a>';
 
-    var domain = documentDomainFilterProvider()({
+    var domain = documentDomain({
       title: 'title',
       domain: '</a>' + spamLink
     });
@@ -61,7 +61,7 @@ describe('documentDomain', function() {
   it('escapes HTML in the document uri', function() {
     var spamLink = '<a href="http://example.com/rubies">Buy rubies!!!</a>';
 
-    var domain = documentDomainFilterProvider()({
+    var domain = documentDomain({
       title: 'title',
       uri: 'file:///home/seanh/' + spamLink
     });