Merge pull request #1717 from hypothesis/1238-restore-diff-behavior

Restore visual diff to original behaviour.

h/static/scripts/directives/annotation.coffee

@@ -128,6 +128,19 @@ AnnotationController = [
         @action = 'view'
         @editing = false
 
+    # Calculates the visual diff flags from the targets
+    #
+    # hasDiff is set to true is there are any targets with a difference
+    # shouldShowDiff is set to true if there are some meaningful differences
+    #  - that is, more than just uppercase / lowercase
+    diffFromTargets = (targets) ->
+      hasDiff = targets.some (t) ->
+        t.diffHTML?
+      shouldShowDiff = hasDiff and targets.some (t) ->
+        t.diffHTML? and not t.diffCaseOnly
+
+      {hasDiff, shouldShowDiff}
+
     ###*
     # @ngdoc method
     # @name annotation.AnnotationController#save
@@ -215,13 +228,15 @@ AnnotationController = [
       @annotation.tags = ({text} for text in (model.tags or []))
 
       # Calculate the visual diff flags
-      @hasDiff = false
-      for t in @annotation.target or []
-        if t.diffHTML? and not t.diffCaseOnly
-          @hasDiff = t.hasDiff = true
-        else
-          t.hasDiff = false
-      @showDiff ?= @hasDiff or undefined
+      diffFlags = diffFromTargets(@annotation.target)
+      @hasDiff = diffFlags.hasDiff
+      if @hasDiff
+        # We don't want to override the showDiff value manually changed
+        # by the user, that's why we use a conditional assignment here,
+        # instead of directly setting showDiff to the calculated value
+        @showDiff ?= diffFlags.shouldShowDiff
+      else
+        @showDiff = undefined
 
     updateTimestamp = (repeat=false) =>
       @timestamp = timeHelpers.toFuzzyString model.updated

h/templates/client/annotation.html

@@ -44,14 +44,14 @@
 <section class="annotation-target"
          ng-repeat="target in vm.annotation.target track by $index">
   <blockquote class="annotation-quote"
-              ng-hide="target.hasDiff && vm.showDiff"
+              ng-hide="target.diffHTML && vm.showDiff"
               ng-bind-html="selector.exact"
               ng-repeat="selector in target.selector
                          | filter : {'type': 'TextQuoteSelector'}
                          track by $index"></blockquote>
   <blockquote class="annotation-quote"
               ng-bind-html="target.diffHTML"
-              ng-show="target.hasDiff && vm.showDiff"></blockquote>
+              ng-show="target.diffHTML && vm.showDiff"></blockquote>
 </section>
 <div class="small pull-right"
      ng-show="vm.hasDiff">

tests/js/directives/annotation-test.coffee

@@ -127,20 +127,50 @@ describe 'h.directives.annotation', ->
       controller.render()
       assert.isNull(controller.document)
 
-    describe 'when targets have the same selection text as the anchor', ->
+    describe 'when a single target has text identical to what was in the selectors', ->
       it 'sets `showDiff` to undefined and `hasDiff` to false', ->
         controller.render()
         assert.isFalse(controller.hasDiff)
         assert.isUndefined(controller.showDiff)
 
-    describe 'when targets have different selection text from the anchor', ->
+    describe 'when a single target has different text, compared to what was in the selector', ->
+      targets = null
+
+      beforeEach ->
+        annotation.target = [
+          {diffHTML: "This is <ins>not</ins> the same quote", diffCaseOnly: false},
+        ]
+        controller.render()
+        targets = controller.annotation.target
+
+      it 'sets both `hasDiff` and `showDiff` to true', ->
+        controller.render()
+        assert.isTrue(controller.hasDiff)
+        assert.isTrue(controller.showDiff)
+
+    describe 'when thre are only upper case/lower case difference between the text in the single target, and what was saved in a selector', ->
+      targets = null
+
+      beforeEach ->
+        annotation.target = [
+          {diffHTML: "<ins>s</ins><del>S</del>tuff", diffCaseOnly: true},
+        ]
+        controller.render()
+        targets = controller.annotation.target
+
+      it 'sets `hasDiff` to true and `showDiff` to false', ->
+        controller.render()
+        assert.isTrue(controller.hasDiff)
+        assert.isFalse(controller.showDiff)
+
+    describe 'when there are multiple targets, some with differences in text', ->
       targets = null
 
       beforeEach ->
         annotation.target = [
           {otherProperty: 'bar'},
-          {diffHTML: "things"},
-          {diffHTML: "stuff", diffCaseOnly: true},
+          {diffHTML: "This is <ins>not</ins> the same quote", diffCaseOnly: false},
+          {diffHTML: "<ins>s</ins><del>S</del>tuff", diffCaseOnly: true},
         ]
         controller.render()
         targets = controller.annotation.target
@@ -151,13 +181,6 @@ describe 'h.directives.annotation', ->
       it 'sets `showDiff` to true', ->
         assert.isTrue(controller.showDiff)
 
-      it 'sets `hasDiff` to true on targets with differences', ->
-        assert.isFalse(targets[0].hasDiff)
-        assert.isTrue(targets[1].hasDiff)
-
-      it 'sets `hasDiff` to false on targets with only case differences', ->
-        assert.isFalse(targets[2].hasDiff)
-
       it 'preserves the `showDiff` value on update', ->
         controller.showDiff = false
         annotation.target = annotation.target.slice(1)
@@ -169,6 +192,34 @@ describe 'h.directives.annotation', ->
         controller.render()
         assert.isFalse(controller.hasDiff)
 
+    describe 'when there are multiple targets, some with differences, but only upper case / lower case', ->
+      targets = null
+
+      beforeEach ->
+        annotation.target = [
+          {otherProperty: 'bar'},
+          {diffHTML: "<ins>s</ins><del>S</del>tuff", diffCaseOnly: true},
+        ]
+        controller.render()
+        targets = controller.annotation.target
+
+      it 'sets `hasDiff` to true', ->
+        assert.isTrue(controller.hasDiff)
+
+      it 'sets `showDiff` to false', ->
+        assert.isFalse(controller.showDiff)
+
+      it 'preserves the `showDiff` value on update', ->
+        controller.showDiff = true
+        annotation.target = annotation.target.slice(1)
+        controller.render()
+        assert.isTrue(controller.showDiff)
+
+      it 'unsets `hasDiff` if differences go away', ->
+        annotation.target = annotation.target.splice(0, 1)
+        controller.render()
+        assert.isFalse(controller.hasDiff)
+
     describe 'timestamp', ->
       clock = null