Commenting on image diffs

6 files changed, 236 insertions, 59 deletions

diff --git a/lib/gitlab/diff/file.rb b/lib/gitlab/diff/file.rb
index fcac85ff892..599c3c5deab 100644
--- a/lib/gitlab/diff/file.rb
+++ b/lib/gitlab/diff/file.rb
@@ -27,16 +27,23 @@ module Gitlab
         @fallback_diff_refs = fallback_diff_refs
       end
 
-      def position(line)
+      def position(position_marker, position_type: :text)
         return unless diff_refs
 
-        Position.new(
+        data = {
+          diff_refs: diff_refs,
+          position_type: position_type.to_s,
           old_path: old_path,
-          new_path: new_path,
-          old_line: line.old_line,
-          new_line: line.new_line,
-          diff_refs: diff_refs
-        )
+          new_path: new_path
+        }
+
+        if position_type == :text
+          data.merge!(text_position_properties(position_marker))
+        else
+          data.merge!(image_position_properties(position_marker))
+        end
+
+        Position.new(data)
       end
 
       def line_code(line)
@@ -228,6 +235,14 @@ module Gitlab
 
       private
 
+      def text_position_properties(line)
+        { old_line: line.old_line, new_line: line.new_line }
+      end
+
+      def image_position_properties(image_point)
+        image_point.to_h
+      end
+
       def blobs_changed?
         old_blob && new_blob && old_blob.id != new_blob.id
       end
diff --git a/lib/gitlab/diff/formatters/base_formatter.rb b/lib/gitlab/diff/formatters/base_formatter.rb
new file mode 100644
index 00000000000..5e923b9e602
--- /dev/null
+++ b/lib/gitlab/diff/formatters/base_formatter.rb
@@ -0,0 +1,61 @@
+module Gitlab
+  module Diff
+    module Formatters
+      class BaseFormatter
+        attr_reader :old_path
+        attr_reader :new_path
+        attr_reader :base_sha
+        attr_reader :start_sha
+        attr_reader :head_sha
+        attr_reader :position_type
+
+        def initialize(attrs)
+          if diff_file = attrs[:diff_file]
+            attrs[:diff_refs] = diff_file.diff_refs
+            attrs[:old_path] = diff_file.old_path
+            attrs[:new_path] = diff_file.new_path
+          end
+
+          if diff_refs = attrs[:diff_refs]
+            attrs[:base_sha] = diff_refs.base_sha
+            attrs[:start_sha] = diff_refs.start_sha
+            attrs[:head_sha]  = diff_refs.head_sha
+          end
+
+          @old_path = attrs[:old_path]
+          @new_path = attrs[:new_path]
+          @base_sha = attrs[:base_sha]
+          @start_sha = attrs[:start_sha]
+          @head_sha  = attrs[:head_sha]
+        end
+
+        def key
+          [base_sha, start_sha, head_sha, Digest::SHA1.hexdigest(old_path || ""), Digest::SHA1.hexdigest(new_path || "")]
+        end
+
+        def to_h
+          {
+            base_sha: base_sha,
+            start_sha: start_sha,
+            head_sha: head_sha,
+            old_path: old_path,
+            new_path: new_path,
+            position_type: position_type
+          }
+        end
+
+        def position_type
+          raise NotImplementedError
+        end
+
+        def ==(other)
+          raise NotImplementedError
+        end
+
+        def complete?
+          raise NotImplementedError
+        end
+      end
+    end
+  end
+end
diff --git a/lib/gitlab/diff/formatters/image_formatter.rb b/lib/gitlab/diff/formatters/image_formatter.rb
new file mode 100644
index 00000000000..ccd0d309972
--- /dev/null
+++ b/lib/gitlab/diff/formatters/image_formatter.rb
@@ -0,0 +1,43 @@
+module Gitlab
+  module Diff
+    module Formatters
+      class ImageFormatter < BaseFormatter
+        attr_reader :width
+        attr_reader :height
+        attr_reader :x
+        attr_reader :y
+
+        def initialize(attrs)
+          @x = attrs[:x]
+          @y = attrs[:y]
+          @width = attrs[:width]
+          @height = attrs[:height]
+
+          super(attrs)
+        end
+
+        def key
+          @key ||= super.push(x, y)
+        end
+
+        def complete?
+          x && y && width && height
+        end
+
+        def to_h
+          super.merge(width: width, height: height, x: x, y: y)
+        end
+
+        def position_type
+          "image"
+        end
+
+        def ==(other)
+          other.is_a?(self.class) &&
+            x == other.x &&
+            y == other.y
+        end
+      end
+    end
+  end
+end
diff --git a/lib/gitlab/diff/formatters/text_formatter.rb b/lib/gitlab/diff/formatters/text_formatter.rb
new file mode 100644
index 00000000000..01c7e9f51ab
--- /dev/null
+++ b/lib/gitlab/diff/formatters/text_formatter.rb
@@ -0,0 +1,49 @@
+module Gitlab
+  module Diff
+    module Formatters
+      class TextFormatter < BaseFormatter
+        attr_reader :old_line
+        attr_reader :new_line
+
+        def initialize(attrs)
+          @old_line = attrs[:old_line]
+          @new_line = attrs[:new_line]
+
+          super(attrs)
+        end
+
+        def key
+          @key ||= super.push(old_line, new_line)
+        end
+
+        def complete?
+          old_line || new_line
+        end
+
+        def to_h
+          super.merge(old_line: old_line, new_line: new_line)
+        end
+
+        def line_age
+          if old_line && new_line
+            nil
+          elsif new_line
+            'new'
+          else
+            'old'
+          end
+        end
+
+        def position_type
+          "text"
+        end
+
+        def ==(other)
+          other.is_a?(self.class) &&
+            new_line == other.new_line &&
+            old_line == other.old_line
+        end
+      end
+    end
+  end
+end
diff --git a/lib/gitlab/diff/image_point.rb b/lib/gitlab/diff/image_point.rb
new file mode 100644
index 00000000000..65332dfd239
--- /dev/null
+++ b/lib/gitlab/diff/image_point.rb
@@ -0,0 +1,23 @@
+module Gitlab
+  module Diff
+    class ImagePoint
+      attr_reader :width, :height, :x, :y
+
+      def initialize(width, height, x, y)
+        @width = width
+        @height = height
+        @x = x
+        @y = y
+      end
+
+      def to_h
+        {
+          width: width,
+          height: height,
+          x: x,
+          y: y
+        }
+      end
+    end
+  end
+end
diff --git a/lib/gitlab/diff/position.rb b/lib/gitlab/diff/position.rb
index b8db3adef0a..bd0a9502a5e 100644
--- a/lib/gitlab/diff/position.rb
+++ b/lib/gitlab/diff/position.rb
@@ -1,37 +1,25 @@
-# Defines a specific location, identified by paths and line numbers,
+# Defines a specific location, identified by paths line numbers and image coordinates,
 # within a specific diff, identified by start, head and base commit ids.
 module Gitlab
   module Diff
     class Position
-      attr_reader :old_path
-      attr_reader :new_path
-      attr_reader :old_line
-      attr_reader :new_line
-      attr_reader :base_sha
-      attr_reader :start_sha
-      attr_reader :head_sha
-
+      attr_accessor :formatter
+
+      delegate :old_path,
+               :new_path,
+               :base_sha,
+               :start_sha,
+               :head_sha,
+               :old_line,
+               :new_line,
+               :position_type, to: :formatter
+
+      # A position can belong to a text line or to an image coordinate
+      # it depends of the position_type argument.
+      # Text position will have: new_line and old_line
+      # Image position will have: width, height, x, y
       def initialize(attrs = {})
-        if diff_file = attrs[:diff_file]
-          attrs[:diff_refs] = diff_file.diff_refs
-          attrs[:old_path] = diff_file.old_path
-          attrs[:new_path] = diff_file.new_path
-        end
-
-        if diff_refs = attrs[:diff_refs]
-          attrs[:base_sha]  = diff_refs.base_sha
-          attrs[:start_sha] = diff_refs.start_sha
-          attrs[:head_sha]  = diff_refs.head_sha
-        end
-
-        @old_path = attrs[:old_path]
-        @new_path = attrs[:new_path]
-        @base_sha  = attrs[:base_sha]
-        @start_sha = attrs[:start_sha]
-        @head_sha  = attrs[:head_sha]
-
-        @old_line = attrs[:old_line]
-        @new_line = attrs[:new_line]
+        @formatter = get_formatter_class(attrs[:position_type]).new(attrs)
       end
 
       # `Gitlab::Diff::Position` objects are stored as serialized attributes in
@@ -46,7 +34,11 @@ module Gitlab
       end
 
       def encode_with(coder)
-        coder['attributes'] = self.to_h
+        coder['attributes'] = formatter.to_h
+      end
+
+      def key
+        formatter.key
       end
 
       def ==(other)
@@ -54,20 +46,11 @@ module Gitlab
           other.diff_refs == diff_refs &&
           other.old_path == old_path &&
           other.new_path == new_path &&
-          other.old_line == old_line &&
-          other.new_line == new_line
+          other.formatter == formatter
       end
 
       def to_h
-        {
-          old_path: old_path,
-          new_path: new_path,
-          old_line: old_line,
-          new_line: new_line,
-          base_sha:  base_sha,
-          start_sha: start_sha,
-          head_sha:  head_sha
-        }
+        formatter.to_h
       end
 
       def inspect
@@ -75,23 +58,15 @@ module Gitlab
       end
 
       def complete?
-        file_path.present? &&
-          (old_line || new_line) &&
-          diff_refs.complete?
+        file_path.present? && formatter.complete? && diff_refs.complete?
       end
 
       def to_json(opts = nil)
-        JSON.generate(self.to_h, opts)
+        JSON.generate(formatter.to_h, opts)
       end
 
       def type
-        if old_line && new_line
-          nil
-        elsif new_line
-          'new'
-        else
-          'old'
-        end
+        formatter.line_age
       end
 
       def unchanged?
@@ -150,6 +125,17 @@ module Gitlab
 
         diff_refs.compare_in(repository.project).diffs(paths: paths, expanded: true).diff_files.first
       end
+
+      def get_formatter_class(type)
+        type ||= "text"
+
+        case type
+        when 'image'
+          Gitlab::Diff::Formatters::ImageFormatter
+        else
+          Gitlab::Diff::Formatters::TextFormatter
+        end
+      end
     end
   end
 end