From f0b33aa489772b4cd3bbd5a73fbecec2711089c0 Mon Sep 17 00:00:00 2001
From: Ole Rehmsen <oler@google.com>
Date: Mon, 24 Sep 2018 14:54:27 +0200
Subject: [PATCH] Extract helpers for isOnParent and patchNum

Splitting is a prerequisite for first threading comments, then grouping
them by line.

Change-Id: I6b58d8bac9ba21244152b4e3596732362233b832
---
 .../diff/gr-diff-builder/gr-diff-builder.js   | 59 +++++++++++++++----
 1 file changed, 46 insertions(+), 13 deletions(-)

diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js
index 36fc6833b1..4b85c227a2 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js
@@ -398,6 +398,46 @@
     return threads;
   };
 
+  /**
+   * Returns the patch number that new comment threads should be attached to.
+   *
+   * @param {GrDiffLine} line The line new thread will be attached to.
+   * @param {string=} opt_side Set to LEFT to force adding it to the LEFT side -
+   *     will be ignored if the left is a parent or a merge parent
+   * @return {number} Patch set to attach the new thread to
+   */
+  GrDiffBuilder.prototype._determinePatchNumForNewThreads = function(
+      patchRange, line, opt_side) {
+    if ((line.type === GrDiffLine.Type.REMOVE ||
+         opt_side === GrDiffBuilder.Side.LEFT) &&
+        patchRange.basePatchNum !== 'PARENT' &&
+        !Gerrit.PatchSetBehavior.isMergeParent(patchRange.basePatchNum)) {
+      return patchRange.basePatchNum;
+    } else {
+      return patchRange.patchNum;
+    }
+  };
+
+  /**
+   * Returns whether the comments on the given line are on a (merge) parent.
+   *
+   * @param {string} firstCommentSide
+   * @param {{basePatchNum: number, patchNum: number}} patchRange
+   * @param {GrDiffLine} line The line the comments are on.
+   * @param {string=} opt_side
+   * @return {boolean} True iff the comments on the given line are on a (merge)
+   *    parent.
+   */
+  GrDiffBuilder.prototype._determineIsOnParent = function(
+      firstCommentSide, patchRange, line, opt_side) {
+    return ((line.type === GrDiffLine.Type.REMOVE ||
+             opt_side === GrDiffBuilder.Side.LEFT) &&
+            (patchRange.basePatchNum === 'PARENT' ||
+             Gerrit.PatchSetBehavior.isMergeParent(
+                 patchRange.basePatchNum))) ||
+          firstCommentSide === 'PARENT';
+  };
+
   /**
    * @param {GrDiffLine} line
    * @param {string=} opt_side
@@ -406,23 +446,16 @@
   GrDiffBuilder.prototype._commentThreadGroupForLine = function(
       line, opt_side) {
     const comments =
-        this._getCommentsForLine(this._comments, line, opt_side);
+    this._getCommentsForLine(this._comments, line, opt_side);
     if (!comments || comments.length === 0) {
       return null;
     }
 
-    let patchNum = this._comments.meta.patchRange.patchNum;
-    let isOnParent = comments[0].side === 'PARENT' || false;
-    if (line.type === GrDiffLine.Type.REMOVE ||
-        opt_side === GrDiffBuilder.Side.LEFT) {
-      if (this._comments.meta.patchRange.basePatchNum === 'PARENT' ||
-          Gerrit.PatchSetBehavior.isMergeParent(
-              this._comments.meta.patchRange.basePatchNum)) {
-        isOnParent = true;
-      } else {
-        patchNum = this._comments.meta.patchRange.basePatchNum;
-      }
-    }
+    const patchNum = this._determinePatchNumForNewThreads(
+        this._comments.meta.patchRange, line, opt_side);
+    const isOnParent = this._determineIsOnParent(
+        comments[0].side, this._comments.meta.patchRange, line, opt_side);
+
     const threadGroupEl = this._createThreadGroupFn(patchNum, isOnParent,
         opt_side);
     threadGroupEl.threads = this._getThreads(comments, patchNum);