Skip to content

Commit

Permalink
Filter: deduplicate annotations before filtering
Browse files Browse the repository at this point in the history
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...
  • Loading branch information
wjt committed May 3, 2015
1 parent 4cbe49e commit b097905
Show file tree
Hide file tree
Showing 3 changed files with 30 additions and 8 deletions.
12 changes: 8 additions & 4 deletions src/ui/filter.js
Original file line number Diff line number Diff line change
Expand Up @@ -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],
Expand Down
14 changes: 14 additions & 0 deletions src/ui/highlighter.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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];
Expand Down
12 changes: 8 additions & 4 deletions test/spec/ui/filter_spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -125,10 +125,14 @@ describe('ui.filter.Filter', function () {
};
annotations = [{text: 'cat'}, {text: 'dog'}, {text: 'car'}];
$.each(annotations, function(i, annotation) {
$('<span class="annotator-hl">')
.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]
$('<span class="annotator-hl">')
.text(ch)
.data('annotation', annotation)
.appendTo(element);
}
});
plugin.filters = {'text': testFilter};
plugin.highlights = $(element).find('.annotator-hl');
Expand Down

0 comments on commit b097905

Please sign in to comment.