From 483864db77acb6db479ecdb8ce4dd121731a8330 Mon Sep 17 00:00:00 2001
From: Fatih Acet <acetfatih@gmail.com>
Date: Tue, 3 Jul 2018 23:18:27 +0000
Subject: [PATCH] Improve performance of toggling diff view type

---
 .../diffs/components/diff_content.vue         |   4 +-
 .../diffs/components/diff_table_cell.vue      |  16 ++-
 .../components/inline_diff_table_row.vue      | 104 ++++++++++++++++++
 .../diffs/components/inline_diff_view.vue     |  33 +++++-
 ...le_row.vue => parallel_diff_table_row.vue} |  69 +++---------
 .../diffs/components/parallel_diff_view.vue   |  40 ++++++-
 .../javascripts/diffs/mixins/diff_content.js  |  57 ----------
 .../javascripts/diffs/store/mutation_types.js |   1 -
 .../javascripts/diffs/store/mutations.js      |   6 -
 .../javascripts/diffs/store/mutations_spec.js |  15 ---
 10 files changed, 194 insertions(+), 151 deletions(-)
 create mode 100644 app/assets/javascripts/diffs/components/inline_diff_table_row.vue
 rename app/assets/javascripts/diffs/components/{diff_table_row.vue => parallel_diff_table_row.vue} (67%)
 delete mode 100644 app/assets/javascripts/diffs/mixins/diff_content.js

diff --git a/app/assets/javascripts/diffs/components/diff_content.vue b/app/assets/javascripts/diffs/components/diff_content.vue
index 48ba967285f..b6af49c7e2e 100644
--- a/app/assets/javascripts/diffs/components/diff_content.vue
+++ b/app/assets/javascripts/diffs/components/diff_content.vue
@@ -39,12 +39,12 @@ export default {
     <div class="diff-viewer">
       <template v-if="isTextFile">
         <inline-diff-view
-          v-if="isInlineView"
+          v-show="isInlineView"
           :diff-file="diffFile"
           :diff-lines="diffFile.highlightedDiffLines || []"
         />
         <parallel-diff-view
-          v-else-if="isParallelView"
+          v-show="isParallelView"
           :diff-file="diffFile"
           :diff-lines="diffFile.parallelDiffLines || []"
         />
diff --git a/app/assets/javascripts/diffs/components/diff_table_cell.vue b/app/assets/javascripts/diffs/components/diff_table_cell.vue
index 68fe6787f9b..fdefd63ced2 100644
--- a/app/assets/javascripts/diffs/components/diff_table_cell.vue
+++ b/app/assets/javascripts/diffs/components/diff_table_cell.vue
@@ -10,6 +10,7 @@ import {
   NEW_NO_NEW_LINE_TYPE,
   LINE_HOVER_CLASS_NAME,
   LINE_UNFOLD_CLASS_NAME,
+  INLINE_DIFF_VIEW_TYPE,
 } from '../constants';
 
 export default {
@@ -25,6 +26,11 @@ export default {
       type: Object,
       required: true,
     },
+    diffViewType: {
+      type: String,
+      required: false,
+      default: INLINE_DIFF_VIEW_TYPE,
+    },
     showCommentButton: {
       type: Boolean,
       required: false,
@@ -57,9 +63,9 @@ export default {
     },
   },
   computed: {
-    ...mapGetters(['isLoggedIn', 'isInlineView']),
+    ...mapGetters(['isLoggedIn']),
     normalizedLine() {
-      if (this.isInlineView) {
+      if (this.diffViewType === INLINE_DIFF_VIEW_TYPE) {
         return this.line;
       }
 
@@ -72,10 +78,10 @@ export default {
       return this.normalizedLine.type === CONTEXT_LINE_TYPE;
     },
     isMetaLine() {
+      const { type } = this.normalizedLine;
+
       return (
-        this.normalizedLine.type === OLD_NO_NEW_LINE_TYPE ||
-        this.normalizedLine.type === NEW_NO_NEW_LINE_TYPE ||
-        this.normalizedLine.type === EMPTY_CELL_TYPE
+        type === OLD_NO_NEW_LINE_TYPE || type === NEW_NO_NEW_LINE_TYPE || type === EMPTY_CELL_TYPE
       );
     },
     classNameMap() {
diff --git a/app/assets/javascripts/diffs/components/inline_diff_table_row.vue b/app/assets/javascripts/diffs/components/inline_diff_table_row.vue
new file mode 100644
index 00000000000..a2470843ca6
--- /dev/null
+++ b/app/assets/javascripts/diffs/components/inline_diff_table_row.vue
@@ -0,0 +1,104 @@
+<script>
+import { mapGetters } from 'vuex';
+import DiffTableCell from './diff_table_cell.vue';
+import {
+  NEW_LINE_TYPE,
+  OLD_LINE_TYPE,
+  CONTEXT_LINE_TYPE,
+  CONTEXT_LINE_CLASS_NAME,
+  PARALLEL_DIFF_VIEW_TYPE,
+  LINE_POSITION_LEFT,
+  LINE_POSITION_RIGHT,
+} from '../constants';
+
+export default {
+  components: {
+    DiffTableCell,
+  },
+  props: {
+    diffFile: {
+      type: Object,
+      required: true,
+    },
+    line: {
+      type: Object,
+      required: true,
+    },
+    isBottom: {
+      type: Boolean,
+      required: false,
+      default: false,
+    },
+  },
+  data() {
+    return {
+      isHover: false,
+    };
+  },
+  computed: {
+    ...mapGetters(['isInlineView']),
+    isContextLine() {
+      return this.line.type === CONTEXT_LINE_TYPE;
+    },
+    classNameMap() {
+      return {
+        [this.line.type]: this.line.type,
+        [CONTEXT_LINE_CLASS_NAME]: this.isContextLine,
+        [PARALLEL_DIFF_VIEW_TYPE]: this.isParallelView,
+      };
+    },
+    inlineRowId() {
+      const { lineCode, oldLine, newLine } = this.line;
+
+      return lineCode || `${this.diffFile.fileHash}_${oldLine}_${newLine}`;
+    },
+  },
+  created() {
+    this.newLineType = NEW_LINE_TYPE;
+    this.oldLineType = OLD_LINE_TYPE;
+    this.linePositionLeft = LINE_POSITION_LEFT;
+    this.linePositionRight = LINE_POSITION_RIGHT;
+  },
+  methods: {
+    handleMouseMove(e) {
+      // To show the comment icon on the gutter we need to know if we hover the line.
+      // Current table structure doesn't allow us to do this with CSS in both of the diff view types
+      this.isHover = e.type === 'mouseover';
+    },
+  },
+};
+</script>
+
+<template>
+  <tr
+    :id="inlineRowId"
+    :class="classNameMap"
+    class="line_holder"
+    @mouseover="handleMouseMove"
+    @mouseout="handleMouseMove"
+  >
+    <diff-table-cell
+      :diff-file="diffFile"
+      :line="line"
+      :line-type="oldLineType"
+      :is-bottom="isBottom"
+      :is-hover="isHover"
+      :show-comment-button="true"
+      class="diff-line-num old_line"
+    />
+    <diff-table-cell
+      :diff-file="diffFile"
+      :line="line"
+      :line-type="newLineType"
+      :is-bottom="isBottom"
+      :is-hover="isHover"
+      class="diff-line-num new_line"
+    />
+    <diff-table-cell
+      :class="line.type"
+      :diff-file="diffFile"
+      :line="line"
+      :is-content-line="true"
+    />
+  </tr>
+</template>
diff --git a/app/assets/javascripts/diffs/components/inline_diff_view.vue b/app/assets/javascripts/diffs/components/inline_diff_view.vue
index e72f85df77a..b884230fb63 100644
--- a/app/assets/javascripts/diffs/components/inline_diff_view.vue
+++ b/app/assets/javascripts/diffs/components/inline_diff_view.vue
@@ -1,12 +1,39 @@
 <script>
-import diffContentMixin from '../mixins/diff_content';
+import { mapGetters } from 'vuex';
+import inlineDiffTableRow from './inline_diff_table_row.vue';
 import inlineDiffCommentRow from './inline_diff_comment_row.vue';
+import { trimFirstCharOfLineContent } from '../store/utils';
 
 export default {
   components: {
     inlineDiffCommentRow,
+    inlineDiffTableRow,
+  },
+  props: {
+    diffFile: {
+      type: Object,
+      required: true,
+    },
+    diffLines: {
+      type: Array,
+      required: true,
+    },
+  },
+  computed: {
+    ...mapGetters(['commit']),
+    normalizedDiffLines() {
+      return this.diffLines.map(line => (line.richText ? trimFirstCharOfLineContent(line) : line));
+    },
+    diffLinesLength() {
+      return this.normalizedDiffLines.length;
+    },
+    commitId() {
+      return this.commit && this.commit.id;
+    },
+    userColorScheme() {
+      return window.gon.user_color_scheme;
+    },
   },
-  mixins: [diffContentMixin],
 };
 </script>
 
@@ -19,7 +46,7 @@ export default {
       <template
         v-for="(line, index) in normalizedDiffLines"
       >
-        <diff-table-row
+        <inline-diff-table-row
           :diff-file="diffFile"
           :line="line"
           :is-bottom="index + 1 === diffLinesLength"
diff --git a/app/assets/javascripts/diffs/components/diff_table_row.vue b/app/assets/javascripts/diffs/components/parallel_diff_table_row.vue
similarity index 67%
rename from app/assets/javascripts/diffs/components/diff_table_row.vue
rename to app/assets/javascripts/diffs/components/parallel_diff_table_row.vue
index 8716fdaf44d..9cb68971c49 100644
--- a/app/assets/javascripts/diffs/components/diff_table_row.vue
+++ b/app/assets/javascripts/diffs/components/parallel_diff_table_row.vue
@@ -35,30 +35,21 @@ export default {
   },
   data() {
     return {
-      isHover: false,
       isLeftHover: false,
       isRightHover: false,
     };
   },
   computed: {
-    ...mapGetters(['isInlineView', 'isParallelView']),
+    ...mapGetters(['isParallelView']),
     isContextLine() {
-      return this.line.left
-        ? this.line.left.type === CONTEXT_LINE_TYPE
-        : this.line.type === CONTEXT_LINE_TYPE;
+      return this.line.left.type === CONTEXT_LINE_TYPE;
     },
     classNameMap() {
       return {
-        [this.line.type]: this.line.type,
         [CONTEXT_LINE_CLASS_NAME]: this.isContextLine,
         [PARALLEL_DIFF_VIEW_TYPE]: this.isParallelView,
       };
     },
-    inlineRowId() {
-      const { lineCode, oldLine, newLine } = this.line;
-
-      return lineCode || `${this.diffFile.fileHash}_${oldLine}_${newLine}`;
-    },
     parallelViewLeftLineType() {
       if (this.line.right.type === NEW_NO_NEW_LINE_TYPE) {
         return OLD_NO_NEW_LINE_TYPE;
@@ -72,23 +63,19 @@ export default {
     this.oldLineType = OLD_LINE_TYPE;
     this.linePositionLeft = LINE_POSITION_LEFT;
     this.linePositionRight = LINE_POSITION_RIGHT;
+    this.parallelDiffViewType = PARALLEL_DIFF_VIEW_TYPE;
   },
   methods: {
     handleMouseMove(e) {
       const isHover = e.type === 'mouseover';
+      const hoveringCell = e.target.closest('td');
+      const allCellsInHoveringRow = Array.from(e.currentTarget.children);
+      const hoverIndex = allCellsInHoveringRow.indexOf(hoveringCell);
 
-      if (this.isInlineView) {
-        this.isHover = isHover;
+      if (hoverIndex >= 2) {
+        this.isRightHover = isHover;
       } else {
-        const hoveringCell = e.target.closest('td');
-        const allCellsInHoveringRow = Array.from(e.currentTarget.children);
-        const hoverIndex = allCellsInHoveringRow.indexOf(hoveringCell);
-
-        if (hoverIndex >= 2) {
-          this.isRightHover = isHover;
-        } else {
-          this.isLeftHover = isHover;
-        }
+        this.isLeftHover = isHover;
       }
     },
     // Prevent text selecting on both sides of parallel diff view
@@ -110,40 +97,6 @@ export default {
 
 <template>
   <tr
-    v-if="isInlineView"
-    :id="inlineRowId"
-    :class="classNameMap"
-    class="line_holder"
-    @mouseover="handleMouseMove"
-    @mouseout="handleMouseMove"
-  >
-    <diff-table-cell
-      :diff-file="diffFile"
-      :line="line"
-      :line-type="oldLineType"
-      :is-bottom="isBottom"
-      :is-hover="isHover"
-      :show-comment-button="true"
-      class="diff-line-num old_line"
-    />
-    <diff-table-cell
-      :diff-file="diffFile"
-      :line="line"
-      :line-type="newLineType"
-      :is-bottom="isBottom"
-      :is-hover="isHover"
-      class="diff-line-num new_line"
-    />
-    <diff-table-cell
-      :class="line.type"
-      :diff-file="diffFile"
-      :line="line"
-      :is-content-line="true"
-    />
-  </tr>
-
-  <tr
-    v-else
     :class="classNameMap"
     class="line_holder"
     @mouseover="handleMouseMove"
@@ -157,6 +110,7 @@ export default {
       :is-bottom="isBottom"
       :is-hover="isLeftHover"
       :show-comment-button="true"
+      :diff-view-type="parallelDiffViewType"
       class="diff-line-num old_line"
     />
     <diff-table-cell
@@ -165,6 +119,7 @@ export default {
       :line="line"
       :is-content-line="true"
       :line-type="parallelViewLeftLineType"
+      :diff-view-type="parallelDiffViewType"
       class="line_content parallel left-side"
       @mousedown.native="handleParallelLineMouseDown"
     />
@@ -176,6 +131,7 @@ export default {
       :is-bottom="isBottom"
       :is-hover="isRightHover"
       :show-comment-button="true"
+      :diff-view-type="parallelDiffViewType"
       class="diff-line-num new_line"
     />
     <diff-table-cell
@@ -184,6 +140,7 @@ export default {
       :line="line"
       :is-content-line="true"
       :line-type="line.right.type"
+      :diff-view-type="parallelDiffViewType"
       class="line_content parallel right-side"
       @mousedown.native="handleParallelLineMouseDown"
     />
diff --git a/app/assets/javascripts/diffs/components/parallel_diff_view.vue b/app/assets/javascripts/diffs/components/parallel_diff_view.vue
index ed92b4ee249..d7280338b68 100644
--- a/app/assets/javascripts/diffs/components/parallel_diff_view.vue
+++ b/app/assets/javascripts/diffs/components/parallel_diff_view.vue
@@ -1,25 +1,53 @@
 <script>
-import diffContentMixin from '../mixins/diff_content';
+import { mapGetters } from 'vuex';
+import parallelDiffTableRow from './parallel_diff_table_row.vue';
 import parallelDiffCommentRow from './parallel_diff_comment_row.vue';
 import { EMPTY_CELL_TYPE } from '../constants';
+import { trimFirstCharOfLineContent } from '../store/utils';
 
 export default {
   components: {
+    parallelDiffTableRow,
     parallelDiffCommentRow,
   },
-  mixins: [diffContentMixin],
+  props: {
+    diffFile: {
+      type: Object,
+      required: true,
+    },
+    diffLines: {
+      type: Array,
+      required: true,
+    },
+  },
   computed: {
+    ...mapGetters(['commit']),
     parallelDiffLines() {
-      return this.normalizedDiffLines.map(line => {
-        if (!line.left) {
+      return this.diffLines.map(line => {
+        if (line.left) {
+          Object.assign(line, { left: trimFirstCharOfLineContent(line.left) });
+        } else {
           Object.assign(line, { left: { type: EMPTY_CELL_TYPE } });
-        } else if (!line.right) {
+        }
+
+        if (line.right) {
+          Object.assign(line, { right: trimFirstCharOfLineContent(line.right) });
+        } else {
           Object.assign(line, { right: { type: EMPTY_CELL_TYPE } });
         }
 
         return line;
       });
     },
+    diffLinesLength() {
+      return this.parallelDiffLines.length;
+    },
+    commitId() {
+      return this.commit && this.commit.id;
+    },
+    userColorScheme() {
+      return window.gon.user_color_scheme;
+    },
   },
 };
 </script>
@@ -35,7 +63,7 @@ export default {
         <template
           v-for="(line, index) in parallelDiffLines"
         >
-          <diff-table-row
+          <parallel-diff-table-row
             :diff-file="diffFile"
             :line="line"
             :is-bottom="index + 1 === diffLinesLength"
diff --git a/app/assets/javascripts/diffs/mixins/diff_content.js b/app/assets/javascripts/diffs/mixins/diff_content.js
deleted file mode 100644
index ebb511d3a7e..00000000000
--- a/app/assets/javascripts/diffs/mixins/diff_content.js
+++ /dev/null
@@ -1,57 +0,0 @@
-import { mapGetters } from 'vuex';
-import diffDiscussions from '../components/diff_discussions.vue';
-import diffLineGutterContent from '../components/diff_line_gutter_content.vue';
-import diffLineNoteForm from '../components/diff_line_note_form.vue';
-import diffTableRow from '../components/diff_table_row.vue';
-import { trimFirstCharOfLineContent } from '../store/utils';
-
-export default {
-  props: {
-    diffFile: {
-      type: Object,
-      required: true,
-    },
-    diffLines: {
-      type: Array,
-      required: true,
-    },
-  },
-  components: {
-    diffDiscussions,
-    diffTableRow,
-    diffLineNoteForm,
-    diffLineGutterContent,
-  },
-  computed: {
-    ...mapGetters(['commit']),
-    commitId() {
-      return this.commit && this.commit.id;
-    },
-    userColorScheme() {
-      return window.gon.user_color_scheme;
-    },
-    normalizedDiffLines() {
-      return this.diffLines.map(line => {
-        if (line.richText) {
-          return trimFirstCharOfLineContent(line);
-        }
-
-        if (line.left) {
-          Object.assign(line, { left: trimFirstCharOfLineContent(line.left) });
-        }
-
-        if (line.right) {
-          Object.assign(line, { right: trimFirstCharOfLineContent(line.right) });
-        }
-
-        return line;
-      });
-    },
-    diffLinesLength() {
-      return this.normalizedDiffLines.length;
-    },
-    fileHash() {
-      return this.diffFile.fileHash;
-    },
-  },
-};
diff --git a/app/assets/javascripts/diffs/store/mutation_types.js b/app/assets/javascripts/diffs/store/mutation_types.js
index 63e9239dce4..2c8e1a1466f 100644
--- a/app/assets/javascripts/diffs/store/mutation_types.js
+++ b/app/assets/javascripts/diffs/store/mutation_types.js
@@ -1,7 +1,6 @@
 export const SET_BASE_CONFIG = 'SET_BASE_CONFIG';
 export const SET_LOADING = 'SET_LOADING';
 export const SET_DIFF_DATA = 'SET_DIFF_DATA';
-export const SET_DIFF_FILES = 'SET_DIFF_FILES';
 export const SET_DIFF_VIEW_TYPE = 'SET_DIFF_VIEW_TYPE';
 export const SET_MERGE_REQUEST_DIFFS = 'SET_MERGE_REQUEST_DIFFS';
 export const ADD_COMMENT_FORM_LINE = 'ADD_COMMENT_FORM_LINE';
diff --git a/app/assets/javascripts/diffs/store/mutations.js b/app/assets/javascripts/diffs/store/mutations.js
index 339a33f8b71..8aa8a114c6f 100644
--- a/app/assets/javascripts/diffs/store/mutations.js
+++ b/app/assets/javascripts/diffs/store/mutations.js
@@ -20,12 +20,6 @@ export default {
     });
   },
 
-  [types.SET_DIFF_FILES](state, diffFiles) {
-    Object.assign(state, {
-      diffFiles: convertObjectPropsToCamelCase(diffFiles, { deep: true }),
-    });
-  },
-
   [types.SET_MERGE_REQUEST_DIFFS](state, mergeRequestDiffs) {
     Object.assign(state, {
       mergeRequestDiffs: convertObjectPropsToCamelCase(mergeRequestDiffs, { deep: true }),
diff --git a/spec/javascripts/diffs/store/mutations_spec.js b/spec/javascripts/diffs/store/mutations_spec.js
index 02836fcaeea..1af49f4985c 100644
--- a/spec/javascripts/diffs/store/mutations_spec.js
+++ b/spec/javascripts/diffs/store/mutations_spec.js
@@ -24,21 +24,6 @@ describe('DiffsStoreMutations', () => {
     });
   });
 
-  describe('SET_DIFF_FILES', () => {
-    it('should set diff files to state', () => {
-      const filePath = '/first-diff-file-path';
-      const state = {};
-      const diffFiles = {
-        a_mode: 1,
-        highlighted_diff_lines: [{ file_path: filePath }],
-      };
-
-      mutations[types.SET_DIFF_FILES](state, diffFiles);
-      expect(state.diffFiles.aMode).toEqual(1);
-      expect(state.diffFiles.highlightedDiffLines[0].filePath).toEqual(filePath);
-    });
-  });
-
   describe('SET_DIFF_VIEW_TYPE', () => {
     it('should set diff view type properly', () => {
       const state = {};