From b3f9be06398e8872cc64a966f99866b67e18c337 Mon Sep 17 00:00:00 2001
From: Robert Speicher <rspeicher@gmail.com>
Date: Sat, 6 Jun 2015 23:15:32 -0400
Subject: [PATCH 1/7] Refactor and spec BlobView JS

---
 app/assets/javascripts/blob/blob.js.coffee | 229 +++++++++++++++------
 app/views/shared/_file_highlight.html.haml |   4 +-
 spec/javascripts/blob/blob_spec.js.coffee  | 169 +++++++++++++++
 spec/javascripts/fixtures/blob.html.haml   |   9 +
 4 files changed, 351 insertions(+), 60 deletions(-)
 create mode 100644 spec/javascripts/blob/blob_spec.js.coffee
 create mode 100644 spec/javascripts/fixtures/blob.html.haml

diff --git a/app/assets/javascripts/blob/blob.js.coffee b/app/assets/javascripts/blob/blob.js.coffee
index 37a175fdbc..b7caae23f3 100644
--- a/app/assets/javascripts/blob/blob.js.coffee
+++ b/app/assets/javascripts/blob/blob.js.coffee
@@ -1,73 +1,186 @@
+# BlobView
+#
+# Handles single- and multi-line selection and highlight for blob views.
+#
+#= require jquery.scrollTo
+#
+# ### Example Markup
+#
+#   <div id="tree-content-holder">
+#     <div class="file-content">
+#       <div class="line-numbers">
+#         <a href="#L1" id="L1" data-line-number="1">1</a>
+#         <a href="#L2" id="L2" data-line-number="2">2</a>
+#         <a href="#L3" id="L3" data-line-number="3">3</a>
+#         <a href="#L4" id="L4" data-line-number="4">4</a>
+#         <a href="#L5" id="L5" data-line-number="5">5</a>
+#       </div>
+#       <pre class="code highlight">
+#         <code>
+#           <span id="LC1" class="line">...</span>
+#           <span id="LC2" class="line">...</span>
+#           <span id="LC3" class="line">...</span>
+#           <span id="LC4" class="line">...</span>
+#           <span id="LC5" class="line">...</span>
+#         </code>
+#       </pre>
+#     </div>
+#   </div>
 class @BlobView
-  constructor: ->
-    # handle multi-line select
-    handleMultiSelect = (e) ->
-      [ first_line, last_line ] = parseSelectedLines()
-      [ line_number ] = parseSelectedLines($(this).attr("id"))
-      hash = "L#{line_number}"
+  # Internal copy of location.hash so we're not dependent on `location` in tests
+  @_hash = ''
 
-      if e.shiftKey and not isNaN(first_line) and not isNaN(line_number)
-        if line_number < first_line
-          last_line = first_line
-          first_line = line_number
-        else
-          last_line = line_number
+  # Initialize a BlobView object
+  #
+  # hash - String URL hash for dependency injection in tests
+  constructor: (hash = location.hash) ->
+    @_hash = hash
 
-        hash = if first_line == last_line then "L#{first_line}" else "L#{first_line}-#{last_line}"
+    @bindEvents()
 
-      setHash(hash)
-      e.preventDefault()
+    unless hash == ''
+      range = @hashToRange(hash)
 
-    # See if there are lines selected
-    # "#L12" and "#L34-56" supported
-    highlightBlobLines = (e) ->
-      [ first_line, last_line ] = parseSelectedLines()
+      unless isNaN(range[0])
+        @highlightRange(range)
 
-      unless isNaN first_line
-        $("#tree-content-holder .highlight .line").removeClass("hll")
-        $("#LC#{line}").addClass("hll") for line in [first_line..last_line]
-        $.scrollTo("#L#{first_line}", offset: -50) unless e?
+        # Scroll to the first highlighted line on initial load
+        # Offset -50 for the sticky top bar, and another -100 for some context
+        $.scrollTo("#L#{range[0]}", offset: -150)
 
-    # parse selected lines from hash
-    # always return first and last line (initialized to NaN)
-    parseSelectedLines = (str) ->
-      first_line = NaN
-      last_line = NaN
-      hash = str || window.location.hash
+  bindEvents: ->
+    $('#tree-content-holder').on 'mousedown', 'a[data-line-number]', @clickHandler
 
-      if hash isnt ""
-        matches = hash.match(/\#?L(\d+)(\-(\d+))?/)
-        first_line = parseInt(matches?[1])
-        last_line = parseInt(matches?[3])
-        last_line = first_line if isNaN(last_line)
+    # While it may seem odd to bind to the mousedown event and then throw away
+    # the click event, there is a method to our madness.
+    #
+    # If not done this way, the line number anchor will sometimes keep its
+    # active state even when the event is cancelled, resulting in an ugly border
+    # around the link and/or a persisted underline text decoration.
 
-      [ first_line, last_line ]
+    $('#tree-content-holder').on 'click', 'a[data-line-number]', (event) ->
+      event.preventDefault()
 
-    setHash = (hash) ->
-      hash = hash.replace(/^\#/, "")
-      nodes = $("#" + hash)
-      # if any nodes are using this id, they must be temporarily changed
-      # also, add a temporary div at the top of the screen to prevent scrolling
-      if nodes.length > 0
-        scroll_top = $(document).scrollTop()
-        nodes.attr("id", "")
-        tmp = $("<div></div>")
-          .css({ position: "absolute", visibility: "hidden", top: scroll_top + "px" })
-          .attr("id", hash)
-          .appendTo(document.body)
+  clickHandler: (event) =>
+    event.preventDefault()
 
-      window.location.hash = hash
+    lineNumber = $(event.target).data('line-number')
+    current = @hashToRange(@_hash)
 
-      # restore the nodes
-      if nodes.length > 0
-        tmp.remove()
-        nodes.attr("id", hash)
+    # Unhighlight previously highlighted lines
+    $('.hll').removeClass('hll')
 
-    # initialize multi-line select
-    $("#tree-content-holder .line-numbers a[id^=L]").on("click", handleMultiSelect)
+    if isNaN(current[0]) or !event.shiftKey
+      # If there's no current selection, or there is but Shift wasn't held,
+      # treat this like a single-line selection.
+      @setHash(lineNumber)
+      @highlightLine(lineNumber)
+    else if event.shiftKey
+      if lineNumber < current[0]
+        range = [lineNumber, current[0]]
+      else
+        range = [current[0], lineNumber]
 
-    # Highlight the correct lines on load
-    highlightBlobLines()
+      @setHash(range[0], range[1])
+      @highlightRange(range)
 
-    # Highlight the correct lines when the hash part of the URL changes
-    $(window).on("hashchange", highlightBlobLines)
+  # Convert a URL hash String into line numbers
+  #
+  # hash - Hash String
+  #
+  # Examples:
+  #
+  #   hashToRange('#L5')    # => [5, NaN]
+  #   hashToRange('#L5-15') # => [5, 15]
+  #   hashToRange('#foo')   # => [NaN, NaN]
+  #
+  # Returns an Array
+  hashToRange: (hash) ->
+    first = parseInt(hash.replace(/^#L(\d+)/, '$1'))
+    last  = parseInt(hash.replace(/^#L\d+-(\d+)/, '$1'))
+
+    [first, last]
+
+  # Highlight a single line
+  #
+  # lineNumber - Number to highlight. Must be parsable as an Integer.
+  #
+  # Returns undefined if lineNumber is not parsable as an Integer.
+  highlightLine: (lineNumber) ->
+    return if isNaN(parseInt(lineNumber))
+
+    $("#LC#{lineNumber}").addClass('hll')
+
+  # Highlight all lines within a range
+  #
+  # range - An Array of starting and ending line numbers.
+  #
+  # Examples:
+  #
+  #   # Highlight lines 5 through 15
+  #   highlightRange([5, 15])
+  #
+  #   # The first value is required, and must be a number
+  #   highlightRange(['foo', 15]) # Invalid, returns undefined
+  #   highlightRange([NaN, NaN])  # Invalid, returns undefined
+  #
+  #   # The second value is optional; if omitted, only highlights the first line
+  #   highlightRange([5, NaN]) # Valid
+  #
+  # Returns undefined if the first line is NaN.
+  highlightRange: (range) ->
+    return if isNaN(range[0])
+
+    if isNaN(range[1])
+      @highlightLine(range[0])
+    else
+      for lineNumber in [range[0]..range[1]]
+        @highlightLine(lineNumber)
+
+  setHash: (firstLineNumber, lastLineNumber) =>
+    return if isNaN(parseInt(firstLineNumber))
+
+    if isNaN(parseInt(lastLineNumber))
+      hash = "#L#{firstLineNumber}"
+    else
+      hash = "#L#{firstLineNumber}-#{lastLineNumber}"
+
+    @setHashWithoutScroll(hash)
+
+  # Prevents the page from scrolling when `location.hash` is set
+  #
+  # This is accomplished by removing the `id` attribute of the matching element,
+  # creating a temporary div at the top of the current viewport, setting the
+  # hash, and then removing the div and restoring the `id` attribute.
+  #
+  # See http://stackoverflow.com/a/1489802/223897
+  #
+  # FIXME (rspeicher): This is still super buggy for me.
+  setHashWithoutScroll: (hash) ->
+    @_hash = hash
+
+    # Extract the first ID, in case we were given a range
+    firstID = hash.replace(/-\d+$/, '')
+
+    $node = $(firstID)
+    $node.removeAttr('id')
+
+    $tmp = $('<div></div>')
+      .css(
+        position: 'absolute'
+        top: "#{$(window).scrollTop()}px"
+        visibility: 'hidden'
+      )
+      .attr('id', firstID)
+      .appendTo($('body'))
+
+    @__setLocationHash__(hash)
+
+    $tmp.remove()
+    $node.attr('id', firstID)
+
+  # Make the actual `location.hash` change
+  #
+  # This method is stubbed in tests.
+  __setLocationHash__: (value) ->
+    location.hash = value
diff --git a/app/views/shared/_file_highlight.html.haml b/app/views/shared/_file_highlight.html.haml
index 86921f0a77..ab70f4770b 100644
--- a/app/views/shared/_file_highlight.html.haml
+++ b/app/views/shared/_file_highlight.html.haml
@@ -4,8 +4,8 @@
       - blob.data.lines.to_a.size.times do |index|
         - offset = defined?(first_line_number) ? first_line_number : 1
         - i = index + offset
-        / We're not using `link_to` because it is too slow once we get to thousands of lines.
-        %a{href: "#L#{i}", id: "L#{i}", rel: "#L#{i}"}
+        -# We're not using `link_to` because it is too slow once we get to thousands of lines.
+        %a{href: "#L#{i}", id: "L#{i}", 'data-line-number' => i}
           %i.fa.fa-link
           = i
   :preserve
diff --git a/spec/javascripts/blob/blob_spec.js.coffee b/spec/javascripts/blob/blob_spec.js.coffee
new file mode 100644
index 0000000000..a6f68a53f9
--- /dev/null
+++ b/spec/javascripts/blob/blob_spec.js.coffee
@@ -0,0 +1,169 @@
+#= require blob/blob
+
+describe 'BlobView', ->
+  fixture.preload('blob.html')
+
+  clickLine = (number, eventData = {}) ->
+    if $.isEmptyObject(eventData)
+      $("#L#{number}").mousedown().click()
+    else
+      e = $.Event 'mousedown', eventData
+      $("#L#{number}").trigger(e).click()
+
+  beforeEach ->
+    fixture.load('blob.html')
+    @class = new BlobView()
+    @spies = {
+      __setLocationHash__: spyOn(@class, '__setLocationHash__').and.callFake ->
+    }
+
+  describe 'behavior', ->
+    it 'highlights one line given in the URL hash', ->
+      new BlobView('#L13')
+      expect($('#LC13')).toHaveClass('hll')
+
+    it 'highlights a range of lines given in the URL hash', ->
+      new BlobView('#L5-25')
+      expect($('.hll').length).toBe(21)
+      expect($("#LC#{line}")).toHaveClass('hll') for line in [5..25]
+
+    it 'scrolls to the first highlighted line on initial load', ->
+      spy = spyOn($, 'scrollTo')
+      new BlobView('#L5-25')
+      expect(spy).toHaveBeenCalledWith('#L5', jasmine.anything())
+
+    it 'discards click events', ->
+      spy = spyOnEvent('a[data-line-number]', 'click')
+      clickLine(13)
+      expect(spy).toHaveBeenPrevented()
+
+    it 'handles garbage input from the hash', ->
+      func = -> new BlobView('#tree-content-holder')
+      expect(func).not.toThrow()
+
+  describe '#clickHandler', ->
+    it 'discards the mousedown event', ->
+      spy = spyOnEvent('a[data-line-number]', 'mousedown')
+      clickLine(13)
+      expect(spy).toHaveBeenPrevented()
+
+    describe 'without shiftKey', ->
+      it 'highlights one line when clicked', ->
+        clickLine(13)
+        expect($('#LC13')).toHaveClass('hll')
+
+      it 'unhighlights previously highlighted lines', ->
+        clickLine(13)
+        clickLine(20)
+
+        expect($('#LC13')).not.toHaveClass('hll')
+        expect($('#LC20')).toHaveClass('hll')
+
+      it 'sets the hash', ->
+        spy = spyOn(@class, 'setHash').and.callThrough()
+        clickLine(13)
+        expect(spy).toHaveBeenCalledWith(13)
+
+    describe 'with shiftKey', ->
+      it 'sets the hash', ->
+        spy = spyOn(@class, 'setHash').and.callThrough()
+        clickLine(13)
+        clickLine(20, shiftKey: true)
+        expect(spy).toHaveBeenCalledWith(13)
+        expect(spy).toHaveBeenCalledWith(13, 20)
+
+      describe 'without existing highlight', ->
+        it 'highlights the clicked line', ->
+          clickLine(13, shiftKey: true)
+          expect($('#LC13')).toHaveClass('hll')
+          expect($('.hll').length).toBe(1)
+
+        it 'sets the hash', ->
+          spy = spyOn(@class, 'setHash')
+          clickLine(13, shiftKey: true)
+          expect(spy).toHaveBeenCalledWith(13)
+
+      describe 'with existing single-line highlight', ->
+        it 'uses existing line as last line when target is lesser', ->
+          clickLine(20)
+          clickLine(15, shiftKey: true)
+          expect($('.hll').length).toBe(6)
+          expect($("#LC#{line}")).toHaveClass('hll') for line in [15..20]
+
+        it 'uses existing line as first line when target is greater', ->
+          clickLine(5)
+          clickLine(10, shiftKey: true)
+          expect($('.hll').length).toBe(6)
+          expect($("#LC#{line}")).toHaveClass('hll') for line in [5..10]
+
+      describe 'with existing multi-line highlight', ->
+        beforeEach ->
+          clickLine(10, shiftKey: true)
+          clickLine(13, shiftKey: true)
+
+        it 'uses target as first line when it is less than existing first line', ->
+          clickLine(5, shiftKey: true)
+          expect($('.hll').length).toBe(6)
+          expect($("#LC#{line}")).toHaveClass('hll') for line in [5..10]
+
+        it 'uses target as last line when it is greater than existing first line', ->
+          clickLine(15, shiftKey: true)
+          expect($('.hll').length).toBe(6)
+          expect($("#LC#{line}")).toHaveClass('hll') for line in [10..15]
+
+  describe '#hashToRange', ->
+    beforeEach ->
+      @subject = @class.hashToRange
+
+    it 'extracts a single line number from the hash', ->
+      expect(@subject('#L5')).toEqual([5, NaN])
+
+    it 'extracts a range of line numbers from the hash', ->
+      expect(@subject('#L5-15')).toEqual([5, 15])
+
+    it 'returns [NaN, NaN] when the hash is not a line number', ->
+      expect(@subject('#foo')).toEqual([NaN, NaN])
+
+  describe '#highlightLine', ->
+    beforeEach ->
+      @subject = @class.highlightLine
+
+    it 'highlights the specified line', ->
+      @subject(13)
+      expect($('#LC13')).toHaveClass('hll')
+
+    it 'accepts a String-based number', ->
+      @subject('13')
+      expect($('#LC13')).toHaveClass('hll')
+
+    it 'returns undefined when given NaN', ->
+      expect(@subject(NaN)).toBe(undefined)
+      expect(@subject('foo')).toBe(undefined)
+
+  describe '#highlightRange', ->
+    beforeEach ->
+      @subject = @class.highlightRange
+
+    it 'returns undefined when first line is NaN', ->
+      expect(@subject([NaN, 15])).toBe(undefined)
+      expect(@subject(['foo', 15])).toBe(undefined)
+
+    it 'returns undefined when given an invalid first line', ->
+      expect(@subject(['foo', 15])).toBe(undefined)
+      expect(@subject([NaN, NaN])).toBe(undefined)
+      expect(@subject('foo')).toBe(undefined)
+
+  describe '#setHash', ->
+    beforeEach ->
+      @subject = @class.setHash
+
+    it 'returns undefined when given an invalid first line', ->
+      expect(@subject('foo', 15)).toBe(undefined)
+
+    it 'sets the location hash for a single line', ->
+      @subject(5)
+      expect(@spies.__setLocationHash__).toHaveBeenCalledWith('#L5')
+
+    it 'sets the location hash for a range', ->
+      @subject(5, 15)
+      expect(@spies.__setLocationHash__).toHaveBeenCalledWith('#L5-15')
diff --git a/spec/javascripts/fixtures/blob.html.haml b/spec/javascripts/fixtures/blob.html.haml
new file mode 100644
index 0000000000..15ad1d8968
--- /dev/null
+++ b/spec/javascripts/fixtures/blob.html.haml
@@ -0,0 +1,9 @@
+#tree-content-holder
+  .file-content
+    .line-numbers
+      - 1.upto(25) do |i|
+        %a{href: "#L#{i}", id: "L#{i}", 'data-line-number' => i}= i
+    %pre.code.highlight
+      %code
+        - 1.upto(25) do |i|
+          %span.line{id: "LC#{i}"}= "Line #{i}"

From 15582293b9e602f5352a6fe88afd9934c9447dad Mon Sep 17 00:00:00 2001
From: Robert Speicher <rspeicher@gmail.com>
Date: Mon, 15 Jun 2015 17:31:41 -0400
Subject: [PATCH 2/7] Use `pushState` instead of the temporary div hack

---
 app/assets/javascripts/blob/blob.js.coffee | 46 +++++-----------------
 1 file changed, 10 insertions(+), 36 deletions(-)

diff --git a/app/assets/javascripts/blob/blob.js.coffee b/app/assets/javascripts/blob/blob.js.coffee
index b7caae23f3..6df5e870d8 100644
--- a/app/assets/javascripts/blob/blob.js.coffee
+++ b/app/assets/javascripts/blob/blob.js.coffee
@@ -64,12 +64,11 @@ class @BlobView
   clickHandler: (event) =>
     event.preventDefault()
 
+    @clearHighlight()
+
     lineNumber = $(event.target).data('line-number')
     current = @hashToRange(@_hash)
 
-    # Unhighlight previously highlighted lines
-    $('.hll').removeClass('hll')
-
     if isNaN(current[0]) or !event.shiftKey
       # If there's no current selection, or there is but Shift wasn't held,
       # treat this like a single-line selection.
@@ -84,6 +83,10 @@ class @BlobView
       @setHash(range[0], range[1])
       @highlightRange(range)
 
+  # Unhighlight previously highlighted lines
+  clearHighlight: ->
+    $('.hll').removeClass('hll')
+
   # Convert a URL hash String into line numbers
   #
   # hash - Hash String
@@ -145,42 +148,13 @@ class @BlobView
     else
       hash = "#L#{firstLineNumber}-#{lastLineNumber}"
 
-    @setHashWithoutScroll(hash)
-
-  # Prevents the page from scrolling when `location.hash` is set
-  #
-  # This is accomplished by removing the `id` attribute of the matching element,
-  # creating a temporary div at the top of the current viewport, setting the
-  # hash, and then removing the div and restoring the `id` attribute.
-  #
-  # See http://stackoverflow.com/a/1489802/223897
-  #
-  # FIXME (rspeicher): This is still super buggy for me.
-  setHashWithoutScroll: (hash) ->
     @_hash = hash
-
-    # Extract the first ID, in case we were given a range
-    firstID = hash.replace(/-\d+$/, '')
-
-    $node = $(firstID)
-    $node.removeAttr('id')
-
-    $tmp = $('<div></div>')
-      .css(
-        position: 'absolute'
-        top: "#{$(window).scrollTop()}px"
-        visibility: 'hidden'
-      )
-      .attr('id', firstID)
-      .appendTo($('body'))
-
     @__setLocationHash__(hash)
 
-    $tmp.remove()
-    $node.attr('id', firstID)
-
-  # Make the actual `location.hash` change
+  # Make the actual hash change in the browser
   #
   # This method is stubbed in tests.
   __setLocationHash__: (value) ->
-    location.hash = value
+    # We're using pushState instead of assigning location.hash directly to
+    # prevent the page from scrolling on the hashchange event
+    history.pushState({turbolinks: false, url: value}, document.title, value)

From da15428340e94e1c99c60e6e6f794d4c25d8e0a2 Mon Sep 17 00:00:00 2001
From: Robert Speicher <rspeicher@gmail.com>
Date: Mon, 15 Jun 2015 17:32:37 -0400
Subject: [PATCH 3/7] Simplify line numbering

---
 app/views/shared/_file_highlight.html.haml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/views/shared/_file_highlight.html.haml b/app/views/shared/_file_highlight.html.haml
index ab70f4770b..d6a2e177da 100644
--- a/app/views/shared/_file_highlight.html.haml
+++ b/app/views/shared/_file_highlight.html.haml
@@ -1,7 +1,7 @@
 .file-content.code{class: user_color_scheme_class}
   .line-numbers
     - if blob.data.present?
-      - blob.data.lines.to_a.size.times do |index|
+      - blob.data.lines.each_index do |index|
         - offset = defined?(first_line_number) ? first_line_number : 1
         - i = index + offset
         -# We're not using `link_to` because it is too slow once we get to thousands of lines.

From 1f88d9b56f02ab05aa1ea055a53627b4c934cf51 Mon Sep 17 00:00:00 2001
From: Robert Speicher <rspeicher@gmail.com>
Date: Mon, 15 Jun 2015 17:33:19 -0400
Subject: [PATCH 4/7] Remove "Multiselect Blob" feature specs

These are well covered by the new Jasmine tests, and faster!
---
 .../project/source/multiselect_blob.feature   | 85 -------------------
 .../steps/project/source/multiselect_blob.rb  | 58 -------------
 2 files changed, 143 deletions(-)
 delete mode 100644 features/project/source/multiselect_blob.feature
 delete mode 100644 features/steps/project/source/multiselect_blob.rb

diff --git a/features/project/source/multiselect_blob.feature b/features/project/source/multiselect_blob.feature
deleted file mode 100644
index 63b7cb77a9..0000000000
--- a/features/project/source/multiselect_blob.feature
+++ /dev/null
@@ -1,85 +0,0 @@
-Feature: Project Source Multiselect Blob
-  Background:
-    Given I sign in as a user
-    And I own project "Shop"
-    And I visit ".gitignore" file in repo
-
-  @javascript
-  Scenario: I click line 1 in file
-    When I click line 1 in file
-    Then I should see "L1" as URI fragment
-    And I should see line 1 highlighted
-
-  @javascript
-  Scenario: I shift-click line 1 in file
-    When I shift-click line 1 in file
-    Then I should see "L1" as URI fragment
-    And I should see line 1 highlighted
-
-  @javascript
-  Scenario: I click line 1 then click line 2 in file
-    When I click line 1 in file
-    Then I should see "L1" as URI fragment
-    And I should see line 1 highlighted
-    Then I click line 2 in file
-    Then I should see "L2" as URI fragment
-    And I should see line 2 highlighted
-
-  @javascript
-  Scenario: I click various line numbers to test multiselect
-    Then I click line 1 in file
-    Then I should see "L1" as URI fragment
-    And I should see line 1 highlighted
-    Then I shift-click line 2 in file
-    Then I should see "L1-2" as URI fragment
-    And I should see lines 1-2 highlighted
-    Then I shift-click line 3 in file
-    Then I should see "L1-3" as URI fragment
-    And I should see lines 1-3 highlighted
-    Then I click line 3 in file
-    Then I should see "L3" as URI fragment
-    And I should see line 3 highlighted
-    Then I shift-click line 1 in file
-    Then I should see "L1-3" as URI fragment
-    And I should see lines 1-3 highlighted
-    Then I shift-click line 5 in file
-    Then I should see "L1-5" as URI fragment
-    And I should see lines 1-5 highlighted
-    Then I shift-click line 4 in file
-    Then I should see "L1-4" as URI fragment
-    And I should see lines 1-4 highlighted
-    Then I click line 5 in file
-    Then I should see "L5" as URI fragment
-    And I should see line 5 highlighted
-    Then I shift-click line 3 in file
-    Then I should see "L3-5" as URI fragment
-    And I should see lines 3-5 highlighted
-    Then I shift-click line 1 in file
-    Then I should see "L1-3" as URI fragment
-    And I should see lines 1-3 highlighted
-    Then I shift-click line 1 in file
-    Then I should see "L1" as URI fragment
-    And I should see line 1 highlighted
-
-  @javascript
-  Scenario: I multiselect lines 1-5 and then go back and forward in history
-    When I click line 1 in file
-    And I shift-click line 3 in file
-    And I shift-click line 2 in file
-    And I shift-click line 5 in file
-    Then I should see "L1-5" as URI fragment
-    And I should see lines 1-5 highlighted
-    Then I go back in history
-    Then I should see "L1-2" as URI fragment
-    And I should see lines 1-2 highlighted
-    Then I go back in history
-    Then I should see "L1-3" as URI fragment
-    And I should see lines 1-3 highlighted
-    Then I go back in history
-    Then I should see "L1" as URI fragment
-    And I should see line 1 highlighted
-    Then I go forward in history
-    And I go forward in history
-    And I go forward in history
-    Then I should see "L1-5" as URI fragment
-    And I should see lines 1-5 highlighted
diff --git a/features/steps/project/source/multiselect_blob.rb b/features/steps/project/source/multiselect_blob.rb
deleted file mode 100644
index 8e14623b89..0000000000
--- a/features/steps/project/source/multiselect_blob.rb
+++ /dev/null
@@ -1,58 +0,0 @@
-class Spinach::Features::ProjectSourceMultiselectBlob < Spinach::FeatureSteps
-  include SharedAuthentication
-  include SharedProject
-  include SharedPaths
-
-  class << self
-    def click_line_steps(*line_numbers)
-      line_numbers.each do |line_number|
-        step "I click line #{line_number} in file" do
-          find("#L#{line_number}").click
-        end
-
-        step "I shift-click line #{line_number} in file" do
-          script = "$('#L#{line_number}').trigger($.Event('click', { shiftKey: true }));"
-          execute_script(script)
-        end
-      end
-    end
-
-    def check_state_steps(*ranges)
-      ranges.each do |range|
-        fragment = range.kind_of?(Array) ? "L#{range.first}-#{range.last}" : "L#{range}"
-        pluralization = range.kind_of?(Array) ? "s" : ""
-
-        step "I should see \"#{fragment}\" as URI fragment" do
-          expect(URI.parse(current_url).fragment).to eq fragment
-        end
-
-        step "I should see line#{pluralization} #{fragment[1..-1]} highlighted" do
-          ids = Array(range).map { |n| "LC#{n}" }
-          extra = false
-
-          highlighted = page.all("#tree-content-holder .highlight .line.hll")
-          highlighted.each do |element|
-            extra ||= ids.delete(element[:id]).nil?
-          end
-
-          expect(extra).to be_false and ids.should be_empty
-        end
-      end
-    end
-  end
-
-  click_line_steps *Array(1..5)
-  check_state_steps *Array(1..5), Array(1..2), Array(1..3), Array(1..4), Array(1..5), Array(3..5)
-
-  step 'I go back in history' do
-    go_back
-  end
-
-  step 'I go forward in history' do
-    go_forward
-  end
-
-  step 'I click on ".gitignore" file in repo' do
-    click_link ".gitignore"
-  end
-end

From 32366d18118281b32b5e770824d637a01d15093b Mon Sep 17 00:00:00 2001
From: Robert Speicher <rspeicher@gmail.com>
Date: Mon, 15 Jun 2015 17:53:39 -0400
Subject: [PATCH 5/7] Rename BlobView to LineHighlighter

---
 app/assets/javascripts/dispatcher.js.coffee    |  2 +-
 ...ob.js.coffee => line_highlighter.js.coffee} |  6 +++---
 ...ob.html.haml => line_highlighter.html.haml} |  0
 ....coffee => line_highlighter_spec.js.coffee} | 18 +++++++++---------
 4 files changed, 13 insertions(+), 13 deletions(-)
 rename app/assets/javascripts/{blob/blob.js.coffee => line_highlighter.js.coffee} (98%)
 rename spec/javascripts/fixtures/{blob.html.haml => line_highlighter.html.haml} (100%)
 rename spec/javascripts/{blob/blob_spec.js.coffee => line_highlighter_spec.js.coffee} (93%)

diff --git a/app/assets/javascripts/dispatcher.js.coffee b/app/assets/javascripts/dispatcher.js.coffee
index b7ebe6a5c8..84873e389e 100644
--- a/app/assets/javascripts/dispatcher.js.coffee
+++ b/app/assets/javascripts/dispatcher.js.coffee
@@ -87,7 +87,7 @@ class Dispatcher
         new TreeView()
         shortcut_handler = new ShortcutsNavigation()
       when 'projects:blob:show'
-        new BlobView()
+        new LineHighlighter()
         shortcut_handler = new ShortcutsNavigation()
       when 'projects:labels:new', 'projects:labels:edit'
         new Labels()
diff --git a/app/assets/javascripts/blob/blob.js.coffee b/app/assets/javascripts/line_highlighter.js.coffee
similarity index 98%
rename from app/assets/javascripts/blob/blob.js.coffee
rename to app/assets/javascripts/line_highlighter.js.coffee
index 6df5e870d8..a60a04783a 100644
--- a/app/assets/javascripts/blob/blob.js.coffee
+++ b/app/assets/javascripts/line_highlighter.js.coffee
@@ -1,4 +1,4 @@
-# BlobView
+# LineHighlighter
 #
 # Handles single- and multi-line selection and highlight for blob views.
 #
@@ -26,11 +26,11 @@
 #       </pre>
 #     </div>
 #   </div>
-class @BlobView
+class @LineHighlighter
   # Internal copy of location.hash so we're not dependent on `location` in tests
   @_hash = ''
 
-  # Initialize a BlobView object
+  # Initialize a LineHighlighter object
   #
   # hash - String URL hash for dependency injection in tests
   constructor: (hash = location.hash) ->
diff --git a/spec/javascripts/fixtures/blob.html.haml b/spec/javascripts/fixtures/line_highlighter.html.haml
similarity index 100%
rename from spec/javascripts/fixtures/blob.html.haml
rename to spec/javascripts/fixtures/line_highlighter.html.haml
diff --git a/spec/javascripts/blob/blob_spec.js.coffee b/spec/javascripts/line_highlighter_spec.js.coffee
similarity index 93%
rename from spec/javascripts/blob/blob_spec.js.coffee
rename to spec/javascripts/line_highlighter_spec.js.coffee
index a6f68a53f9..d9a1ff2d5b 100644
--- a/spec/javascripts/blob/blob_spec.js.coffee
+++ b/spec/javascripts/line_highlighter_spec.js.coffee
@@ -1,7 +1,7 @@
-#= require blob/blob
+#= require line_highlighter
 
-describe 'BlobView', ->
-  fixture.preload('blob.html')
+describe 'LineHighlighter', ->
+  fixture.preload('line_highlighter.html')
 
   clickLine = (number, eventData = {}) ->
     if $.isEmptyObject(eventData)
@@ -11,25 +11,25 @@ describe 'BlobView', ->
       $("#L#{number}").trigger(e).click()
 
   beforeEach ->
-    fixture.load('blob.html')
-    @class = new BlobView()
+    fixture.load('line_highlighter.html')
+    @class = new LineHighlighter()
     @spies = {
       __setLocationHash__: spyOn(@class, '__setLocationHash__').and.callFake ->
     }
 
   describe 'behavior', ->
     it 'highlights one line given in the URL hash', ->
-      new BlobView('#L13')
+      new LineHighlighter('#L13')
       expect($('#LC13')).toHaveClass('hll')
 
     it 'highlights a range of lines given in the URL hash', ->
-      new BlobView('#L5-25')
+      new LineHighlighter('#L5-25')
       expect($('.hll').length).toBe(21)
       expect($("#LC#{line}")).toHaveClass('hll') for line in [5..25]
 
     it 'scrolls to the first highlighted line on initial load', ->
       spy = spyOn($, 'scrollTo')
-      new BlobView('#L5-25')
+      new LineHighlighter('#L5-25')
       expect(spy).toHaveBeenCalledWith('#L5', jasmine.anything())
 
     it 'discards click events', ->
@@ -38,7 +38,7 @@ describe 'BlobView', ->
       expect(spy).toHaveBeenPrevented()
 
     it 'handles garbage input from the hash', ->
-      func = -> new BlobView('#tree-content-holder')
+      func = -> new LineHighlighter('#tree-content-holder')
       expect(func).not.toThrow()
 
   describe '#clickHandler', ->

From e59aad6e83cbdafcaf100bb86f6fb925f2fb779e Mon Sep 17 00:00:00 2001
From: Robert Speicher <rspeicher@gmail.com>
Date: Fri, 19 Jun 2015 02:01:53 -0400
Subject: [PATCH 6/7] Refactor LineHighlighter

---
 .../javascripts/line_highlighter.js.coffee    | 72 ++++++++-----------
 .../line_highlighter_spec.js.coffee           | 63 ++++++----------
 2 files changed, 52 insertions(+), 83 deletions(-)

diff --git a/app/assets/javascripts/line_highlighter.js.coffee b/app/assets/javascripts/line_highlighter.js.coffee
index a60a04783a..d7de551642 100644
--- a/app/assets/javascripts/line_highlighter.js.coffee
+++ b/app/assets/javascripts/line_highlighter.js.coffee
@@ -26,9 +26,13 @@
 #       </pre>
 #     </div>
 #   </div>
+#
 class @LineHighlighter
+  # CSS class applied to highlighted lines
+  highlightClass: 'hll'
+
   # Internal copy of location.hash so we're not dependent on `location` in tests
-  @_hash = ''
+  _hash: ''
 
   # Initialize a LineHighlighter object
   #
@@ -41,7 +45,7 @@ class @LineHighlighter
     unless hash == ''
       range = @hashToRange(hash)
 
-      unless isNaN(range[0])
+      if range[0]
         @highlightRange(range)
 
         # Scroll to the first highlighted line on initial load
@@ -69,7 +73,7 @@ class @LineHighlighter
     lineNumber = $(event.target).data('line-number')
     current = @hashToRange(@_hash)
 
-    if isNaN(current[0]) or !event.shiftKey
+    unless current[0] and event.shiftKey
       # If there's no current selection, or there is but Shift wasn't held,
       # treat this like a single-line selection.
       @setHash(lineNumber)
@@ -85,7 +89,7 @@ class @LineHighlighter
 
   # Unhighlight previously highlighted lines
   clearHighlight: ->
-    $('.hll').removeClass('hll')
+    $(".#{@highlightClass}").removeClass(@highlightClass)
 
   # Convert a URL hash String into line numbers
   #
@@ -93,60 +97,44 @@ class @LineHighlighter
   #
   # Examples:
   #
-  #   hashToRange('#L5')    # => [5, NaN]
+  #   hashToRange('#L5')    # => [5, null]
   #   hashToRange('#L5-15') # => [5, 15]
-  #   hashToRange('#foo')   # => [NaN, NaN]
+  #   hashToRange('#foo')   # => [null, null]
   #
   # Returns an Array
   hashToRange: (hash) ->
-    first = parseInt(hash.replace(/^#L(\d+)/, '$1'))
-    last  = parseInt(hash.replace(/^#L\d+-(\d+)/, '$1'))
+    matches = hash.match(/^#?L(\d+)(?:-(\d+))?$/)
 
-    [first, last]
+    if matches and matches.length
+      first = parseInt(matches[1])
+      last  = matches[2] and parseInt(matches[2]) or null
+
+      [first, last]
+    else
+      [null, null]
 
   # Highlight a single line
   #
-  # lineNumber - Number to highlight. Must be parsable as an Integer.
-  #
-  # Returns undefined if lineNumber is not parsable as an Integer.
-  highlightLine: (lineNumber) ->
-    return if isNaN(parseInt(lineNumber))
-
-    $("#LC#{lineNumber}").addClass('hll')
+  # lineNumber - Line number to highlight
+  highlightLine: (lineNumber) =>
+    $("#LC#{lineNumber}").addClass(@highlightClass)
 
   # Highlight all lines within a range
   #
-  # range - An Array of starting and ending line numbers.
-  #
-  # Examples:
-  #
-  #   # Highlight lines 5 through 15
-  #   highlightRange([5, 15])
-  #
-  #   # The first value is required, and must be a number
-  #   highlightRange(['foo', 15]) # Invalid, returns undefined
-  #   highlightRange([NaN, NaN])  # Invalid, returns undefined
-  #
-  #   # The second value is optional; if omitted, only highlights the first line
-  #   highlightRange([5, NaN]) # Valid
-  #
-  # Returns undefined if the first line is NaN.
+  # range - Array containing the starting and ending line numbers
   highlightRange: (range) ->
-    return if isNaN(range[0])
-
-    if isNaN(range[1])
-      @highlightLine(range[0])
-    else
+    if range[1]
       for lineNumber in [range[0]..range[1]]
         @highlightLine(lineNumber)
-
-  setHash: (firstLineNumber, lastLineNumber) =>
-    return if isNaN(parseInt(firstLineNumber))
-
-    if isNaN(parseInt(lastLineNumber))
-      hash = "#L#{firstLineNumber}"
     else
+      @highlightLine(range[0])
+
+  # Set the URL hash string
+  setHash: (firstLineNumber, lastLineNumber) =>
+    if lastLineNumber
       hash = "#L#{firstLineNumber}-#{lastLineNumber}"
+    else
+      hash = "#L#{firstLineNumber}"
 
     @_hash = hash
     @__setLocationHash__(hash)
diff --git a/spec/javascripts/line_highlighter_spec.js.coffee b/spec/javascripts/line_highlighter_spec.js.coffee
index d9a1ff2d5b..14fa487ff7 100644
--- a/spec/javascripts/line_highlighter_spec.js.coffee
+++ b/spec/javascripts/line_highlighter_spec.js.coffee
@@ -13,6 +13,7 @@ describe 'LineHighlighter', ->
   beforeEach ->
     fixture.load('line_highlighter.html')
     @class = new LineHighlighter()
+    @css   = @class.highlightClass
     @spies = {
       __setLocationHash__: spyOn(@class, '__setLocationHash__').and.callFake ->
     }
@@ -20,12 +21,12 @@ describe 'LineHighlighter', ->
   describe 'behavior', ->
     it 'highlights one line given in the URL hash', ->
       new LineHighlighter('#L13')
-      expect($('#LC13')).toHaveClass('hll')
+      expect($('#LC13')).toHaveClass(@css)
 
     it 'highlights a range of lines given in the URL hash', ->
       new LineHighlighter('#L5-25')
-      expect($('.hll').length).toBe(21)
-      expect($("#LC#{line}")).toHaveClass('hll') for line in [5..25]
+      expect($(".#{@css}").length).toBe(21)
+      expect($("#LC#{line}")).toHaveClass(@css) for line in [5..25]
 
     it 'scrolls to the first highlighted line on initial load', ->
       spy = spyOn($, 'scrollTo')
@@ -50,14 +51,14 @@ describe 'LineHighlighter', ->
     describe 'without shiftKey', ->
       it 'highlights one line when clicked', ->
         clickLine(13)
-        expect($('#LC13')).toHaveClass('hll')
+        expect($('#LC13')).toHaveClass(@css)
 
       it 'unhighlights previously highlighted lines', ->
         clickLine(13)
         clickLine(20)
 
-        expect($('#LC13')).not.toHaveClass('hll')
-        expect($('#LC20')).toHaveClass('hll')
+        expect($('#LC13')).not.toHaveClass(@css)
+        expect($('#LC20')).toHaveClass(@css)
 
       it 'sets the hash', ->
         spy = spyOn(@class, 'setHash').and.callThrough()
@@ -75,8 +76,8 @@ describe 'LineHighlighter', ->
       describe 'without existing highlight', ->
         it 'highlights the clicked line', ->
           clickLine(13, shiftKey: true)
-          expect($('#LC13')).toHaveClass('hll')
-          expect($('.hll').length).toBe(1)
+          expect($('#LC13')).toHaveClass(@css)
+          expect($(".#{@css}").length).toBe(1)
 
         it 'sets the hash', ->
           spy = spyOn(@class, 'setHash')
@@ -87,14 +88,14 @@ describe 'LineHighlighter', ->
         it 'uses existing line as last line when target is lesser', ->
           clickLine(20)
           clickLine(15, shiftKey: true)
-          expect($('.hll').length).toBe(6)
-          expect($("#LC#{line}")).toHaveClass('hll') for line in [15..20]
+          expect($(".#{@css}").length).toBe(6)
+          expect($("#LC#{line}")).toHaveClass(@css) for line in [15..20]
 
         it 'uses existing line as first line when target is greater', ->
           clickLine(5)
           clickLine(10, shiftKey: true)
-          expect($('.hll').length).toBe(6)
-          expect($("#LC#{line}")).toHaveClass('hll') for line in [5..10]
+          expect($(".#{@css}").length).toBe(6)
+          expect($("#LC#{line}")).toHaveClass(@css) for line in [5..10]
 
       describe 'with existing multi-line highlight', ->
         beforeEach ->
@@ -103,26 +104,26 @@ describe 'LineHighlighter', ->
 
         it 'uses target as first line when it is less than existing first line', ->
           clickLine(5, shiftKey: true)
-          expect($('.hll').length).toBe(6)
-          expect($("#LC#{line}")).toHaveClass('hll') for line in [5..10]
+          expect($(".#{@css}").length).toBe(6)
+          expect($("#LC#{line}")).toHaveClass(@css) for line in [5..10]
 
         it 'uses target as last line when it is greater than existing first line', ->
           clickLine(15, shiftKey: true)
-          expect($('.hll').length).toBe(6)
-          expect($("#LC#{line}")).toHaveClass('hll') for line in [10..15]
+          expect($(".#{@css}").length).toBe(6)
+          expect($("#LC#{line}")).toHaveClass(@css) for line in [10..15]
 
   describe '#hashToRange', ->
     beforeEach ->
       @subject = @class.hashToRange
 
     it 'extracts a single line number from the hash', ->
-      expect(@subject('#L5')).toEqual([5, NaN])
+      expect(@subject('#L5')).toEqual([5, null])
 
     it 'extracts a range of line numbers from the hash', ->
       expect(@subject('#L5-15')).toEqual([5, 15])
 
-    it 'returns [NaN, NaN] when the hash is not a line number', ->
-      expect(@subject('#foo')).toEqual([NaN, NaN])
+    it 'returns [null, null] when the hash is not a line number', ->
+      expect(@subject('#foo')).toEqual([null, null])
 
   describe '#highlightLine', ->
     beforeEach ->
@@ -130,36 +131,16 @@ describe 'LineHighlighter', ->
 
     it 'highlights the specified line', ->
       @subject(13)
-      expect($('#LC13')).toHaveClass('hll')
+      expect($('#LC13')).toHaveClass(@css)
 
     it 'accepts a String-based number', ->
       @subject('13')
-      expect($('#LC13')).toHaveClass('hll')
-
-    it 'returns undefined when given NaN', ->
-      expect(@subject(NaN)).toBe(undefined)
-      expect(@subject('foo')).toBe(undefined)
-
-  describe '#highlightRange', ->
-    beforeEach ->
-      @subject = @class.highlightRange
-
-    it 'returns undefined when first line is NaN', ->
-      expect(@subject([NaN, 15])).toBe(undefined)
-      expect(@subject(['foo', 15])).toBe(undefined)
-
-    it 'returns undefined when given an invalid first line', ->
-      expect(@subject(['foo', 15])).toBe(undefined)
-      expect(@subject([NaN, NaN])).toBe(undefined)
-      expect(@subject('foo')).toBe(undefined)
+      expect($('#LC13')).toHaveClass(@css)
 
   describe '#setHash', ->
     beforeEach ->
       @subject = @class.setHash
 
-    it 'returns undefined when given an invalid first line', ->
-      expect(@subject('foo', 15)).toBe(undefined)
-
     it 'sets the location hash for a single line', ->
       @subject(5)
       expect(@spies.__setLocationHash__).toHaveBeenCalledWith('#L5')

From 7f5b255c08593200797f5d29793e375e96f320ef Mon Sep 17 00:00:00 2001
From: Robert Speicher <rspeicher@gmail.com>
Date: Fri, 19 Jun 2015 16:43:09 -0400
Subject: [PATCH 7/7] Minor style fixes for LineHighlighter

---
 app/assets/javascripts/line_highlighter.js.coffee | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/app/assets/javascripts/line_highlighter.js.coffee b/app/assets/javascripts/line_highlighter.js.coffee
index d7de551642..a8b3c1fa33 100644
--- a/app/assets/javascripts/line_highlighter.js.coffee
+++ b/app/assets/javascripts/line_highlighter.js.coffee
@@ -73,7 +73,7 @@ class @LineHighlighter
     lineNumber = $(event.target).data('line-number')
     current = @hashToRange(@_hash)
 
-    unless current[0] and event.shiftKey
+    unless current[0] && event.shiftKey
       # If there's no current selection, or there is but Shift wasn't held,
       # treat this like a single-line selection.
       @setHash(lineNumber)
@@ -105,9 +105,9 @@ class @LineHighlighter
   hashToRange: (hash) ->
     matches = hash.match(/^#?L(\d+)(?:-(\d+))?$/)
 
-    if matches and matches.length
+    if matches && matches.length
       first = parseInt(matches[1])
-      last  = matches[2] and parseInt(matches[2]) or null
+      last  = if matches[2] then parseInt(matches[2]) else null
 
       [first, last]
     else