From 3ebfb0be2c06a2f9f0cccfd369e9c4d145936f9f Mon Sep 17 00:00:00 2001 From: Will Thompson Date: Sat, 2 May 2015 10:07:29 +0100 Subject: [PATCH 1/3] Make updateFilter test more realistic Before, it depended on pretty specific knowledge of how updateFilter is implemented, which seems quite questionable! --- test/spec/ui/filter_spec.js | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/test/spec/ui/filter_spec.js b/test/spec/ui/filter_spec.js index ad49a28bb..624297976 100644 --- a/test/spec/ui/filter_spec.js +++ b/test/spec/ui/filter_spec.js @@ -124,15 +124,23 @@ describe('ui.filter.Filter', function () { } }; annotations = [{text: 'cat'}, {text: 'dog'}, {text: 'car'}]; + $.each(annotations, function(i, annotation) { + $('') + .text(annotation) + .data('annotation', annotation) + .appendTo(element); + }); plugin.filters = {'text': testFilter}; - plugin.highlights = { - map: function () { return annotations; } - }; + plugin.highlights = $(element).find('.annotator-hl'); sandbox.stub(plugin, 'updateHighlights'); sandbox.stub(plugin, 'resetHighlights'); sandbox.stub(plugin, 'filterHighlights'); }); + afterEach(function () { + $(element).empty(); + }); + it("should call Filter#updateHighlights()", function () { plugin.updateFilter(testFilter); assert(plugin.updateHighlights.calledOnce); From 4cbe49e4bfecdb184f2197d0def4c09079ff94a7 Mon Sep 17 00:00:00 2001 From: Will Thompson Date: Sat, 2 May 2015 10:09:16 +0100 Subject: [PATCH 2/3] Highlighter#draw: correct sense of _local.highlights test --- src/ui/highlighter.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ui/highlighter.js b/src/ui/highlighter.js index a4ebb5ce5..00f72c094 100644 --- a/src/ui/highlighter.js +++ b/src/ui/highlighter.js @@ -140,7 +140,7 @@ Highlighter.prototype.draw = function (annotation) { annotation._local = {}; } var hasHighlights = (typeof annotation._local.highlights !== 'undefined' && - annotation._local.highlights === null); + annotation._local.highlights !== null); if (!hasHighlights) { annotation._local.highlights = []; } From b097905ada54147c56a50c435418f59fd55e05b2 Mon Sep 17 00:00:00 2001 From: Will Thompson Date: Sat, 2 May 2015 10:12:38 +0100 Subject: [PATCH 3/3] Filter: deduplicate annotations before filtering MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If there are many spans of text for an annotation (many table cells, say), previously the filter would be applied once for each span, and matching annotations would appear n times in filter.annotations. In and of itself this is relatively harmless, but filterHighlights then maps back from annotations to highlights, and: for (var i = 0, len = filtered.length; i < len; i++) { highlights = highlights.not(filtered[i]._local.highlights); } is ruinous. This change dramatically improves filtering performance on my pet test case of big, overlapping highlights on a 5000×3 table. I have mixed feelings about using a new identifier in _local for this, but we don't have any guarantee that the id provided by the server is unique, or even present. I also have mixed feelings about setting the id in highlighter.js and using it in filter.js, but the same coupling exists for .data('annotation') already... --- src/ui/filter.js | 12 ++++++++---- src/ui/highlighter.js | 14 ++++++++++++++ test/spec/ui/filter_spec.js | 12 ++++++++---- 3 files changed, 30 insertions(+), 8 deletions(-) diff --git a/src/ui/filter.js b/src/ui/filter.js index 5e37c4513..29a8bf2ab 100644 --- a/src/ui/filter.js +++ b/src/ui/filter.js @@ -160,10 +160,14 @@ Filter.prototype.updateFilter = function (filter) { return; } - var annotations = this.highlights.map(function () { - return $(this).data('annotation'); - }); - annotations = $.makeArray(annotations); + var annotations = (function (highlights) { + var annotationsById = {}; + highlights.each(function () { + var a = $(this).data('annotation'); + annotationsById[a._local.id] = a; + }); + return $.map(annotationsById, function(a) { return a; }); + }(this.highlights)); for (var i = 0, len = annotations.length; i < len; i++) { var annotation = annotations[i], diff --git a/src/ui/highlighter.js b/src/ui/highlighter.js index 00f72c094..bc1b05a42 100644 --- a/src/ui/highlighter.js +++ b/src/ui/highlighter.js @@ -119,6 +119,15 @@ Highlighter.prototype.drawAll = function (annotations) { return p; }; +// local unique IDs for annotations, used to deduplicate when recovering them +// from highlights. +var id = (function () { + var counter = 0; + return function() { + return '_local_id_' + (++counter); + } +}()); + // Public: Draw highlights for the annotation. // // annotation - An annotation Object for which to draw highlights. @@ -144,6 +153,11 @@ Highlighter.prototype.draw = function (annotation) { if (!hasHighlights) { annotation._local.highlights = []; } + var hasLocalId = (typeof annotation._local.id !== 'undefined' && + annotation._local.id === null); + if (!hasLocalId) { + annotation._local.id = id(); + } for (var j = 0, jlen = normedRanges.length; j < jlen; j++) { var normed = normedRanges[j]; diff --git a/test/spec/ui/filter_spec.js b/test/spec/ui/filter_spec.js index 624297976..79d3f2c4f 100644 --- a/test/spec/ui/filter_spec.js +++ b/test/spec/ui/filter_spec.js @@ -125,10 +125,14 @@ describe('ui.filter.Filter', function () { }; annotations = [{text: 'cat'}, {text: 'dog'}, {text: 'car'}]; $.each(annotations, function(i, annotation) { - $('') - .text(annotation) - .data('annotation', annotation) - .appendTo(element); + this._local = {id: this.text}; + for (var j = 0; j < annotation.text.length; j++) { + var ch = annotation.text[j] + $('') + .text(ch) + .data('annotation', annotation) + .appendTo(element); + } }); plugin.filters = {'text': testFilter}; plugin.highlights = $(element).find('.annotator-hl');