Commenting on image diffs

9 files changed, 168 insertions, 100 deletions

diff --git a/app/views/discussions/_diff_discussion.html.haml b/app/views/discussions/_diff_discussion.html.haml
index e6d307e5568..52279d0a870 100644
--- a/app/views/discussions/_diff_discussion.html.haml
+++ b/app/views/discussions/_diff_discussion.html.haml
@@ -1,6 +1,10 @@
-- expanded = local_assigns.fetch(:expanded, true)
-%tr.notes_holder{ class: ('hide' unless expanded) }
-  %td.notes_line{ colspan: 2 }
-  %td.notes_content
-    .content{ class: ('hide' unless expanded) }
-      = render partial: "discussions/notes", collection: discussions, as: :discussion
+- if local_assigns[:on_image]
+  = render partial: "discussions/notes", collection: discussions, as: :discussion
+- else
+  -# Text diff discussions
+  - expanded = local_assigns.fetch(:expanded, true)
+  %tr.notes_holder{ class: ('hide' unless expanded) }
+    %td.notes_line{ colspan: 2 }
+    %td.notes_content
+      .content{ class: ('hide' unless expanded) }
+        = render partial: "discussions/notes", collection: discussions, as: :discussion
diff --git a/app/views/discussions/_diff_with_notes.html.haml b/app/views/discussions/_diff_with_notes.html.haml
index 4a41be972da..636d06cab53 100644
--- a/app/views/discussions/_diff_with_notes.html.haml
+++ b/app/views/discussions/_diff_with_notes.html.haml
@@ -1,18 +1,27 @@
 - diff_file = discussion.diff_file
 - blob = discussion.blob
+- discussions = { discussion.original_line_code => [discussion] }
+- diff_file_class = diff_file.text? ? 'text-file' : 'js-image-file'
 
-.diff-file.file-holder
+.diff-file.file-holder{ class: diff_file_class }
   .js-file-title.file-title.file-title-flex-parent
     .file-header-content
       = render "projects/diffs/file_header", diff_file: diff_file, url: discussion_path(discussion), show_toggle: false
 
-  .diff-content.code.js-syntax-highlight
-    %table
-      - discussions = { discussion.original_line_code => [discussion] }
-      = render partial: "projects/diffs/line",
-        collection: discussion.truncated_diff_lines,
-        as: :line,
-        locals: { diff_file: diff_file,
-          discussions: discussions,
-          discussion_expanded: true,
-          plain: true }
+  - if diff_file.text?
+    .diff-content.code.js-syntax-highlight
+      %table
+        = render partial: "projects/diffs/line",
+          collection: discussion.truncated_diff_lines,
+          as: :line,
+          locals: { diff_file: diff_file,
+            discussions: discussions,
+            discussion_expanded: true,
+            plain: true }
+  - else
+    - partial = (diff_file.new_file? || diff_file.deleted_file?) ? 'single_image_diff' : 'replaced_image_diff'
+
+    = render partial: "projects/diffs/#{partial}", locals: { diff_file: diff_file, position: discussion.position.to_json, click_to_comment: false }
+
+    .note-container
+      = render partial: "discussions/notes", locals: { discussion: discussion, show_toggle: false, show_image_comment_badge: true, disable_collapse: true }
diff --git a/app/views/discussions/_notes.html.haml b/app/views/discussions/_notes.html.haml
index db5ab939948..9efcfef690f 100644
--- a/app/views/discussions/_notes.html.haml
+++ b/app/views/discussions/_notes.html.haml
@@ -1,6 +1,19 @@
-.discussion-notes
-  %ul.notes{ data: { discussion_id: discussion.id } }
-    = render partial: "shared/notes/note", collection: discussion.notes, as: :note
+- disable_collapse = local_assigns.fetch(:disable_collapse, false)
+- collapsed_class = 'collapsed' if discussion.resolved? && !disable_collapse
+- badge_counter = discussion_counter + 1 if local_assigns[:discussion_counter]
+- show_toggle = local_assigns.fetch(:show_toggle, true)
+- show_image_comment_badge = local_assigns.fetch(:show_image_comment_badge, false)
+
+.discussion-notes{ class: collapsed_class }
+  -# Save the first note position data so that we have a reference and can go
+  -# to the first note position when we click on a badge diff discussion
+  %ul.notes{ id: "discussion_#{discussion.id}", data: { discussion_id: discussion.id, position: discussion.notes[0].position.to_json } }
+    - if discussion.try(:on_image?) && show_toggle
+      %button.diff-notes-collapse.js-diff-notes-toggle{ type: 'button' }
+        = sprite_icon('collapse', css_class: 'collapse-icon')
+      %button.btn-transparent.badge.js-diff-notes-toggle{ type: 'button' }
+        = badge_counter
+    = render partial: "shared/notes/note", collection: discussion.notes, as: :note, locals: { badge_counter: badge_counter, show_image_comment_badge: show_image_comment_badge }
 
   .flash-container
 
diff --git a/app/views/projects/diffs/_image_diff_frame.html.haml b/app/views/projects/diffs/_image_diff_frame.html.haml
new file mode 100644
index 00000000000..dae73e10460
--- /dev/null
+++ b/app/views/projects/diffs/_image_diff_frame.html.haml
@@ -0,0 +1,5 @@
+- class_name = local_assigns.fetch(:class_name, '')
+- note_type = local_assigns.fetch(:note_type, '')
+
+.frame{ class: class_name, data: { position: position, note_type: note_type } }
+  = image_tag(image_path, alt: alt, draggable: false, lazy: false)
diff --git a/app/views/projects/diffs/_replaced_image_diff.html.haml b/app/views/projects/diffs/_replaced_image_diff.html.haml
new file mode 100644
index 00000000000..8fc232b464e
--- /dev/null
+++ b/app/views/projects/diffs/_replaced_image_diff.html.haml
@@ -0,0 +1,61 @@
+- blob = diff_file.blob
+- old_blob = diff_file.old_blob
+- blob_raw_path = diff_file_blob_raw_path(diff_file)
+- old_blob_raw_path = diff_file_old_blob_raw_path(diff_file)
+- click_to_comment = local_assigns.fetch(:click_to_comment, true)
+- diff_view_data = local_assigns.fetch(:diff_view_data, '')
+- class_name = ''
+
+- if click_to_comment
+  - class_name = 'js-add-image-diff-note-button click-to-comment'
+
+.image.js-replaced-image{ data: diff_view_data }
+  .two-up.view
+    .wrap
+      .frame.deleted
+        = image_tag(old_blob_raw_path, alt: diff_file.old_path, lazy: false)
+      %p.image-info.hide
+        %span.meta-filesize= number_to_human_size(old_blob.size)
+        |
+        %strong W:
+        %span.meta-width
+        |
+        %strong H:
+        %span.meta-height
+    .wrap
+      = render partial: "projects/diffs/image_diff_frame", locals: { class_name: "added js-image-frame #{class_name}", position: position, note_type: DiffNote.name, image_path: blob_raw_path, alt: diff_file.new_path }
+      %p.image-info.hide
+        %span.meta-filesize= number_to_human_size(blob.size)
+        |
+        %strong W:
+        %span.meta-width
+        |
+        %strong H:
+        %span.meta-height
+
+  .swipe.view.hide
+    .swipe-frame
+      .frame.deleted
+        = image_tag(old_blob_raw_path, alt: diff_file.old_path, lazy: false)
+      .swipe-wrap
+        = render partial: "projects/diffs/image_diff_frame", locals: { class_name: "added js-image-frame #{class_name}", position: position, note_type: DiffNote.name, image_path: blob_raw_path, alt: diff_file.new_path }
+      %span.swipe-bar
+        %span.top-handle
+        %span.bottom-handle
+
+  .onion-skin.view.hide
+    .onion-skin-frame
+      .frame.deleted
+        = image_tag(old_blob_raw_path, alt: diff_file.old_path, lazy: false)
+      = render partial: "projects/diffs/image_diff_frame", locals: { class_name: "added js-image-frame #{class_name}", position: position, note_type: DiffNote.name, image_path: blob_raw_path, alt: diff_file.new_path }
+      .controls
+        .transparent
+        .drag-track
+          .dragger{ :style => "left: 0px;" }
+        .opaque
+
+.view-modes.hide
+  %ul.view-modes-menu
+    %li.two-up{ data: { mode: 'two-up' } } 2-up
+    %li.swipe{ data: { mode: 'swipe' } } Swipe
+    %li.onion-skin{ data: { mode: 'onion-skin' } } Onion skin
diff --git a/app/views/projects/diffs/_single_image_diff.html.haml b/app/views/projects/diffs/_single_image_diff.html.haml
new file mode 100644
index 00000000000..6b0c6bbe48f
--- /dev/null
+++ b/app/views/projects/diffs/_single_image_diff.html.haml
@@ -0,0 +1,16 @@
+- blob = diff_file.blob
+- old_blob = diff_file.old_blob
+- blob_raw_path = diff_file_blob_raw_path(diff_file)
+- old_blob_raw_path = diff_file_old_blob_raw_path(diff_file)
+- click_to_comment = local_assigns.fetch(:click_to_comment, true)
+- diff_view_data = local_assigns.fetch(:diff_view_data, '')
+- class_name = ''
+
+- if click_to_comment
+  - class_name = 'js-add-image-diff-note-button click-to-comment'
+
+.image.js-single-image{ data: diff_view_data }
+  .wrap
+    - single_class_name = diff_file.deleted_file? ? 'deleted' : 'added'
+    = render partial: "projects/diffs/image_diff_frame", locals: { class_name: "#{single_class_name} #{class_name} js-image-frame", position: position, note_type: DiffNote.name, image_path: blob_raw_path, alt: diff_file.file_path }
+  %p.image-info= number_to_human_size(blob.size)
diff --git a/app/views/projects/diffs/viewers/_image.html.haml b/app/views/projects/diffs/viewers/_image.html.haml
index 6b5233833c6..f190073c2fc 100644
--- a/app/views/projects/diffs/viewers/_image.html.haml
+++ b/app/views/projects/diffs/viewers/_image.html.haml
@@ -1,67 +1,13 @@
 - diff_file = viewer.diff_file
-- blob = diff_file.blob
-- old_blob = diff_file.old_blob
-- blob_raw_path = diff_file_blob_raw_path(diff_file)
-- old_blob_raw_path = diff_file_old_blob_raw_path(diff_file)
+- image_point = Gitlab::Diff::ImagePoint.new(nil, nil, nil, nil)
+- discussions = @grouped_diff_discussions[diff_file.new_path] if @grouped_diff_discussions
+
+- locals = { diff_file: diff_file, position: diff_file.position(image_point, position_type: :image).to_json, click_to_comment: true, diff_view_data: diff_view_data }
 
 - if diff_file.new_file? || diff_file.deleted_file?
-  .image
-    %span.wrap
-      .frame{ class: (diff_file.deleted_file? ? 'deleted' : 'added') }
-        = image_tag(blob_raw_path, alt: diff_file.file_path)
-      %p.image-info= number_to_human_size(blob.size)
+  = render partial: "projects/diffs/single_image_diff", locals: locals
 - else
-  .image
-    .two-up.view
-      %span.wrap
-        .frame.deleted
-          = image_tag(old_blob_raw_path, alt: diff_file.old_path)
-        %p.image-info.hide
-          %span.meta-filesize= number_to_human_size(old_blob.size)
-          |
-          %b W:
-          %span.meta-width
-          |
-          %b H:
-          %span.meta-height
-      %span.wrap
-        .frame.added
-          = image_tag(blob_raw_path, alt: diff_file.new_path)
-        %p.image-info.hide
-          %span.meta-filesize= number_to_human_size(blob.size)
-          |
-          %b W:
-          %span.meta-width
-          |
-          %b H:
-          %span.meta-height
-
-    .swipe.view.hide
-      .swipe-frame
-        .frame.deleted
-          = image_tag(old_blob_raw_path, alt: diff_file.old_path, lazy: false)
-        .swipe-wrap
-          .frame.added
-            = image_tag(blob_raw_path, alt: diff_file.new_path, lazy: false)
-        %span.swipe-bar
-          %span.top-handle
-          %span.bottom-handle
-
-    .onion-skin.view.hide
-      .onion-skin-frame
-        .frame.deleted
-          = image_tag(old_blob_raw_path, alt: diff_file.old_path, lazy: false)
-        .frame.added
-          = image_tag(blob_raw_path, alt: diff_file.new_path, lazy: false)
-        .controls
-          .transparent
-          .drag-track
-            .dragger{ :style => "left: 0px;" }
-          .opaque
-
+  = render partial: "projects/diffs/replaced_image_diff", locals: locals
 
-  .view-modes.hide
-    %ul.view-modes-menu
-      %li.two-up{ data: { mode: 'two-up' } } 2-up
-      %li.swipe{ data: { mode: 'swipe' } } Swipe
-      %li.onion-skin{ data: { mode: 'onion-skin' } } Onion skin
+.note-container
+  = render partial: "discussions/notes", collection: discussions, as: :discussion
diff --git a/app/views/shared/notes/_form.html.haml b/app/views/shared/notes/_form.html.haml
index 725bf916592..71c0d740bc8 100644
--- a/app/views/shared/notes/_form.html.haml
+++ b/app/views/shared/notes/_form.html.haml
@@ -24,20 +24,21 @@
   -# DiffNote
   = f.hidden_field :position
 
-  = render layout: 'projects/md_preview', locals: { url: preview_url, referenced_users: true } do
-    = render 'projects/zen', f: f,
-      attr: :note,
-      classes: 'note-textarea js-note-text',
-      placeholder: "Write a comment or drag your files here...",
-      supports_quick_actions: supports_quick_actions,
-      supports_autocomplete: supports_autocomplete
-    = render 'shared/notes/hints', supports_quick_actions: supports_quick_actions
-    .error-alert
-
-  .note-form-actions.clearfix
-    = render partial: 'shared/notes/comment_button'
-
-    = yield(:note_actions)
-
-    %a.btn.btn-cancel.js-note-discard{ role: "button", data: {cancel_text: "Cancel" } }
-      Discard draft
+  .discussion-form-container
+    = render layout: 'projects/md_preview', locals: { url: preview_url, referenced_users: true } do
+      = render 'projects/zen', f: f,
+        attr: :note,
+        classes: 'note-textarea js-note-text',
+        placeholder: "Write a comment or drag your files here...",
+        supports_quick_actions: supports_quick_actions,
+        supports_autocomplete: supports_autocomplete
+      = render 'shared/notes/hints', supports_quick_actions: supports_quick_actions
+      .error-alert
+
+    .note-form-actions.clearfix
+      = render partial: 'shared/notes/comment_button'
+
+      = yield(:note_actions)
+
+      %a.btn.btn-cancel.js-note-discard{ role: "button", data: {cancel_text: "Cancel" } }
+        Discard draft
diff --git a/app/views/shared/notes/_note.html.haml b/app/views/shared/notes/_note.html.haml
index 4f00a9f2759..b6085fd3af0 100644
--- a/app/views/shared/notes/_note.html.haml
+++ b/app/views/shared/notes/_note.html.haml
@@ -1,7 +1,10 @@
 - return unless note.author
 - return if note.cross_reference_not_visible_for?(current_user)
 
+- show_image_comment_badge = local_assigns.fetch(:show_image_comment_badge, false)
 - note_editable = note_editable?(note)
+- note_counter = local_assigns.fetch(:note_counter, 0)
+
 %li.timeline-entry{ id: dom_id(note),
   class: ["note", "note-row-#{note.id}", ('system-note' if note.system)],
   data: { author_id: note.author.id,
@@ -12,8 +15,18 @@
       - if note.system
         = icon_for_system_note(note)
       - else
-        %a{ href: user_path(note.author) }
+        %a.image-diff-avatar-link{ href: user_path(note.author) }
           = image_tag avatar_icon(note.author), alt: '', class: 'avatar s40'
+          - if note.is_a?(DiffNote) && note.on_image?
+            - if show_image_comment_badge && note_counter == 0
+              -# Only show this for the first comment in the discussion
+              %span.image-comment-badge.inverted
+                = icon('comment-o')
+            - elsif note_counter == 0
+              - counter = badge_counter if local_assigns[:badge_counter]
+              - badge_class = "hidden" if @fresh_discussion || counter.nil?
+              %span.badge{ class: badge_class }
+                = counter
     .timeline-content
       .note-header
         .note-header-info