Remove diffSide property from UIComment

diffSide property determines on which side of the diff, the thread
will be rendered, left or right.
The property naturally associates with threads and not comments, so
move the determination logic of diffSide to thread-level instead of
comment-level.

Change-Id: Ifd33918fe24cbc5bb5b2daf25b161cd53cb8b75f

polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.js

@@ -29,6 +29,7 @@ import {html} from '@polymer/polymer/lib/utils/html-tag.js';
 import {TestKeyboardShortcutBinder} from '../../../test/test-utils.js';
 import {Shortcut} from '../../../mixins/keyboard-shortcut-mixin/keyboard-shortcut-mixin.js';
 import {ChangeComments} from '../../diff/gr-comment-api/gr-comment-api.js';
+import {createCommentThreads} from '../../../utils/comment-util.js';
 
 const commentApiMock = createCommentApiMockWithTemplateElement(
     'gr-file-list-comment-api-mock', html`
@@ -106,8 +107,6 @@ suite('gr-file-list tests', () => {
       // been initialized.
       commentApiWrapper.loadComments().then(() => {
         sinon.stub(element.changeComments, 'getPaths').returns({});
-        sinon.stub(element.changeComments, 'getCommentsBySideForPath')
-            .returns({meta: {}, left: [], right: []});
         done();
       });
       element._loading = false;
@@ -1527,17 +1526,8 @@ suite('gr-file-list tests', () => {
     ];
 
     async function setupDiff(diff) {
-      diff.comments = {
+      diff.threads = diff.path === '/COMMIT_MSG' ?
-        left: diff.path === '/COMMIT_MSG' ? commitMsgComments : [],
+        createCommentThreads(commitMsgComments) : [];
-        right: [],
-        meta: {
-          changeNum: 1,
-          patchRange: {
-            basePatchNum: 'PARENT',
-            patchNum: 2,
-          },
-        },
-      };
       diff.prefs = {
         context: 10,
         tab_size: 8,
@@ -1556,7 +1546,7 @@ suite('gr-file-list tests', () => {
       };
       diff.diff = getMockDiffResponse();
       commentApiWrapper.loadComments().then(() => {
-        sinon.stub(element.changeComments, 'getCommentsBySideForPath')
+        sinon.stub(element.changeComments, 'getCommentsForPath')
             .withArgs('/COMMIT_MSG', {
               basePatchNum: 'PARENT',
               patchNum: 2,
@@ -1608,8 +1598,6 @@ suite('gr-file-list tests', () => {
       // been initialized.
       commentApiWrapper.loadComments().then(() => {
         sinon.stub(element.changeComments, 'getPaths').returns({});
-        sinon.stub(element.changeComments, 'getCommentsBySideForPath')
-            .returns({meta: {}, left: [], right: []});
         done();
       });
       element._loading = false;

polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api.ts

@@ -19,16 +19,10 @@ import {GestureEventListeners} from '@polymer/polymer/lib/mixins/gesture-event-l
 import {LegacyElementMixin} from '@polymer/polymer/lib/legacy/legacy-element-mixin';
 import {PolymerElement} from '@polymer/polymer/polymer-element';
 import {htmlTemplate} from './gr-comment-api_html';
-import {
+import {patchNumEquals, CURRENT} from '../../../utils/patch-set-util';
-  getParentIndex,
-  isMergeParent,
-  patchNumEquals,
-  CURRENT,
-} from '../../../utils/patch-set-util';
 import {customElement, property} from '@polymer/decorators';
 import {
   CommentBasics,
-  ParentPatchSetNum,
   PatchRange,
   PatchSetNum,
   PathToRobotCommentsInfoMap,
@@ -38,7 +32,6 @@ import {
   RevisionId,
 } from '../../../types/common';
 import {hasOwnProperty} from '../../../utils/common-util';
-import {CommentSide, Side} from '../../../constants/constants';
 import {
   Comment,
   CommentMap,
@@ -50,6 +43,7 @@ import {
   UIHuman,
   UIRobot,
   createCommentThreads,
+  isInPatchRange,
 } from '../../../utils/comment-util';
 import {PatchSetFile, PatchNumOnly, isPatchSetFile} from '../../../types/types';
 import {appContext} from '../../../services/app-context';
@@ -58,11 +52,6 @@ export type CommentIdToCommentThreadMap = {
   [urlEncodedCommentId: string]: CommentThread;
 };
 
-export interface TwoSidesComments {
-  left: UIComment[];
-  right: UIComment[];
-}
-
 export class ChangeComments {
   private readonly _comments: {[path: string]: UIHuman[]};
 
@@ -161,7 +150,7 @@ export class ChangeComments {
             if (!patchRange) {
               return true;
             }
-            return this._isInPatchRange(c, patchRange);
+            return isInPatchRange(c, patchRange);
           })
         ) {
           commentMap[path] = true;
@@ -301,25 +290,11 @@ export class ChangeComments {
     return allDrafts;
   }
 
-  _addCommentSide(comments: TwoSidesComments) {
-    const allComments = [];
-    for (const side of [Side.LEFT, Side.RIGHT]) {
-      // This is needed by the threading.
-      for (const comment of comments[side]) {
-        comment.diffSide = side;
-      }
-      allComments.push(...comments[side]);
-    }
-    return allComments;
-  }
-
   getThreadsBySideForPath(
     path: string,
     patchRange: PatchRange
   ): CommentThread[] {
-    return createCommentThreads(
+    return createCommentThreads(this.getCommentsForPath(path, patchRange));
-      this._addCommentSide(this.getCommentsBySideForPath(path, patchRange))
-    );
   }
 
   /**
@@ -332,10 +307,7 @@ export class ChangeComments {
    * @param projectConfig Optional project config object to
    * include in the meta sub-object.
    */
-  getCommentsBySideForPath(
+  getCommentsForPath(path: string, patchRange: PatchRange): Comment[] {
-    path: string,
-    patchRange: PatchRange
-  ): TwoSidesComments {
     let comments: Comment[] = [];
     let drafts: DraftInfo[] = [];
     let robotComments: RobotCommentInfo[] = [];
@@ -353,33 +325,20 @@ export class ChangeComments {
       d.__draft = true;
     });
 
-    const all: Comment[] = comments
+    return comments
       .concat(drafts)
       .concat(robotComments)
+      .filter(c => isInPatchRange(c, patchRange))
       .map(c => {
         return {...c};
       });
-
-    const baseComments = all.filter(c =>
-      this._isInBaseOfPatchRange(c, patchRange)
-    );
-    const revisionComments = all.filter(c =>
-      this._isInRevisionOfPatchRange(c, patchRange)
-    );
-
-    return {
-      left: baseComments,
-      right: revisionComments,
-    };
   }
 
   getThreadsBySideForFile(
     file: PatchSetFile,
     patchRange: PatchRange
   ): CommentThread[] {
-    return createCommentThreads(
+    return createCommentThreads(this.getCommentsForFile(file, patchRange));
-      this._addCommentSide(this.getCommentsBySideForFile(file, patchRange))
-    );
   }
 
   /**
@@ -394,19 +353,10 @@ export class ChangeComments {
    * @param projectConfig Optional project config object to
    * include in the meta sub-object.
    */
-  getCommentsBySideForFile(
+  getCommentsForFile(file: PatchSetFile, patchRange: PatchRange): Comment[] {
-    file: PatchSetFile,
+    const comments = this.getCommentsForPath(file.path, patchRange);
-    patchRange: PatchRange
-  ): TwoSidesComments {
-    const comments = this.getCommentsBySideForPath(file.path, patchRange);
     if (file.basePath) {
-      const commentsForBasePath = this.getCommentsBySideForPath(
+      comments.push(...this.getCommentsForPath(file.basePath, patchRange));
-        file.basePath,
-        patchRange
-      );
-      // merge in the left and right
-      comments.left = comments.left.concat(commentsForBasePath.left);
-      comments.right = comments.right.concat(commentsForBasePath.right);
     }
     return comments;
   }
@@ -472,58 +422,6 @@ export class ChangeComments {
     const comments = this._commentObjToArray(this.getAllComments(true));
     return createCommentThreads(comments);
   }
-
-  /**
-   * Whether the given comment should be included in the base side of the
-   * given patch range.
-   */
-  _isInBaseOfPatchRange(comment: CommentBasics, range: PatchRange) {
-    // If the base of the patch range is a parent of a merge, and the comment
-    // appears on a specific parent then only show the comment if the parent
-    // index of the comment matches that of the range.
-    if (comment.parent && comment.side === CommentSide.PARENT) {
-      return (
-        isMergeParent(range.basePatchNum) &&
-        comment.parent === getParentIndex(range.basePatchNum)
-      );
-    }
-
-    // If the base of the range is the parent of the patch:
-    if (
-      range.basePatchNum === ParentPatchSetNum &&
-      comment.side === CommentSide.PARENT &&
-      patchNumEquals(comment.patch_set, range.patchNum)
-    ) {
-      return true;
-    }
-    // If the base of the range is not the parent of the patch:
-    return (
-      range.basePatchNum !== ParentPatchSetNum &&
-      comment.side !== CommentSide.PARENT &&
-      patchNumEquals(comment.patch_set, range.basePatchNum)
-    );
-  }
-
-  /**
-   * Whether the given comment should be included in the revision side of the
-   * given patch range.
-   */
-  _isInRevisionOfPatchRange(comment: CommentBasics, range: PatchRange) {
-    return (
-      comment.side !== CommentSide.PARENT &&
-      patchNumEquals(comment.patch_set, range.patchNum)
-    );
-  }
-
-  /**
-   * Whether the given comment should be included in the given patch range.
-   */
-  _isInPatchRange(comment: CommentBasics, range: PatchRange): boolean {
-    return (
-      this._isInBaseOfPatchRange(comment, range) ||
-      this._isInRevisionOfPatchRange(comment, range)
-    );
-  }
 }
 
 // TODO(TS): move findCommentById out of class

polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api_test.js

@@ -19,6 +19,7 @@ import '../../../test/common-test-setup-karma.js';
 import './gr-comment-api.js';
 import {ChangeComments} from './gr-comment-api.js';
 import {CommentSide} from '../../../constants/constants.js';
+import {isInRevisionOfPatchRange, isInBaseOfPatchRange} from '../../../utils/comment-util.js';
 
 const basicFixture = fixtureFromElement('gr-comment-api');
 
@@ -146,74 +147,50 @@ suite('gr-comment-api tests', () => {
       });
     });
 
-    test('_isInBaseOfPatchRange', () => {
+    test('isInBaseOfPatchRange', () => {
       const comment = {patch_set: 1};
       const patchRange = {basePatchNum: 1, patchNum: 2};
-      assert.isTrue(element._changeComments._isInBaseOfPatchRange(comment,
+      assert.isTrue(isInBaseOfPatchRange(comment,
           patchRange));
 
       patchRange.basePatchNum = PARENT;
-      assert.isFalse(element._changeComments._isInBaseOfPatchRange(comment,
+      assert.isFalse(isInBaseOfPatchRange(comment,
           patchRange));
 
       comment.side = PARENT;
-      assert.isFalse(element._changeComments._isInBaseOfPatchRange(comment,
+      assert.isFalse(isInBaseOfPatchRange(comment,
           patchRange));
 
       comment.patch_set = 2;
-      assert.isTrue(element._changeComments._isInBaseOfPatchRange(comment,
+      assert.isTrue(isInBaseOfPatchRange(comment,
           patchRange));
 
       patchRange.basePatchNum = -2;
       comment.side = PARENT;
       comment.parent = 1;
-      assert.isFalse(element._changeComments._isInBaseOfPatchRange(comment,
+      assert.isFalse(isInBaseOfPatchRange(comment,
           patchRange));
 
       comment.parent = 2;
-      assert.isTrue(element._changeComments._isInBaseOfPatchRange(comment,
+      assert.isTrue(isInBaseOfPatchRange(comment,
           patchRange));
     });
 
-    test('_isInRevisionOfPatchRange', () => {
+    test('isInRevisionOfPatchRange', () => {
       const comment = {patch_set: 123};
       const patchRange = {basePatchNum: 122, patchNum: 124};
-      assert.isFalse(element._changeComments._isInRevisionOfPatchRange(
+      assert.isFalse(isInRevisionOfPatchRange(
           comment, patchRange));
 
       patchRange.patchNum = 123;
-      assert.isTrue(element._changeComments._isInRevisionOfPatchRange(
+      assert.isTrue(isInRevisionOfPatchRange(
           comment, patchRange));
 
       comment.side = PARENT;
-      assert.isFalse(element._changeComments._isInRevisionOfPatchRange(
+      assert.isFalse(isInRevisionOfPatchRange(
           comment, patchRange));
     });
 
-    test('_isInPatchRange', () => {
-      const patchRange1 = {basePatchNum: 122, patchNum: 124};
-      const patchRange2 = {basePatchNum: 123, patchNum: 125};
-      const patchRange3 = {basePatchNum: 124, patchNum: 125};
-
-      const isInBasePatchStub = sinon.stub(element._changeComments,
-          '_isInBaseOfPatchRange');
-      const isInRevisionPatchStub = sinon.stub(element._changeComments,
-          '_isInRevisionOfPatchRange');
-
-      isInBasePatchStub.withArgs({}, patchRange1).returns(true);
-      isInBasePatchStub.withArgs({}, patchRange2).returns(false);
-      isInBasePatchStub.withArgs({}, patchRange3).returns(false);
-
-      isInRevisionPatchStub.withArgs({}, patchRange1).returns(false);
-      isInRevisionPatchStub.withArgs({}, patchRange2).returns(true);
-      isInRevisionPatchStub.withArgs({}, patchRange3).returns(false);
-
-      assert.isTrue(element._changeComments._isInPatchRange({}, patchRange1));
-      assert.isTrue(element._changeComments._isInPatchRange({}, patchRange2));
-      assert.isFalse(element._changeComments._isInPatchRange({},
-          patchRange3));
-    });
-
     suite('comment ranges and paths', () => {
       function makeTime(mins) {
         return `2013-02-26 15:0${mins}:43.986000000`;
@@ -349,32 +326,40 @@ suite('gr-comment-api tests', () => {
         assert.property(paths, 'file/four');
       });
 
-      test('getCommentsBySideForPath', () => {
+      test('getCommentsForPath', () => {
         const patchRange = {basePatchNum: 1, patchNum: 3};
         let path = 'file/one';
-        let comments = element._changeComments.getCommentsBySideForPath(path,
+        let comments = element._changeComments.getCommentsForPath(path,
             patchRange);
-        assert.equal(comments.left.length, 0);
+        assert.equal(comments.filter(c => isInBaseOfPatchRange(c, patchRange))
-        assert.equal(comments.right.length, 0);
+            .length, 0);
+        assert.equal(comments.filter(c => isInRevisionOfPatchRange(c,
+            patchRange)).length, 0);
 
         path = 'file/two';
-        comments = element._changeComments.getCommentsBySideForPath(path,
+        comments = element._changeComments.getCommentsForPath(path,
             patchRange);
-        assert.equal(comments.left.length, 0);
+        assert.equal(comments.filter(c => isInBaseOfPatchRange(c, patchRange))
-        assert.equal(comments.right.length, 2);
+            .length, 0);
+        assert.equal(comments.filter(c => isInRevisionOfPatchRange(c,
+            patchRange)).length, 2);
 
         patchRange.basePatchNum = 2;
-        comments = element._changeComments.getCommentsBySideForPath(path,
+        comments = element._changeComments.getCommentsForPath(path,
             patchRange);
-        assert.equal(comments.left.length, 1);
+        assert.equal(comments.filter(c => isInBaseOfPatchRange(c,
-        assert.equal(comments.right.length, 2);
+            patchRange)).length, 1);
+        assert.equal(comments.filter(c => isInRevisionOfPatchRange(c,
+            patchRange)).length, 2);
 
         patchRange.basePatchNum = PARENT;
         path = 'file/three';
-        comments = element._changeComments.getCommentsBySideForPath(path,
+        comments = element._changeComments.getCommentsForPath(path,
             patchRange);
-        assert.equal(comments.left.length, 0);
+        assert.equal(comments.filter(c => isInBaseOfPatchRange(c, patchRange))
-        assert.equal(comments.right.length, 1);
+            .length, 0);
+        assert.equal(comments.filter(c => isInRevisionOfPatchRange(c,
+            patchRange)).length, 1);
       });
 
       test('getAllCommentsForPath', () => {
@@ -539,7 +524,6 @@ suite('gr-comment-api tests', () => {
                 path: 'file/one',
               },
             ],
-            diffSide: undefined,
             commentSide: 'PARENT',
             patchNum: 2,
             path: 'file/one',
@@ -566,7 +550,6 @@ suite('gr-comment-api tests', () => {
             path: 'file/one',
             line: 2,
             range: undefined,
-            diffSide: undefined,
             commentSide: CommentSide.PARENT,
             rootId: '03',
           }, {
@@ -602,7 +585,6 @@ suite('gr-comment-api tests', () => {
             line: 1,
             rootId: '04',
             range: undefined,
-            diffSide: undefined,
             commentSide: CommentSide.REVISION,
           }, {
             comments: [
@@ -619,7 +601,6 @@ suite('gr-comment-api tests', () => {
             line: 2,
             rootId: '05',
             range: undefined,
-            diffSide: undefined,
             commentSide: CommentSide.REVISION,
           }, {
             comments: [
@@ -636,7 +617,6 @@ suite('gr-comment-api tests', () => {
             line: 2,
             rootId: '06',
             range: undefined,
-            diffSide: undefined,
             commentSide: CommentSide.REVISION,
           }, {
             comments: [
@@ -666,7 +646,6 @@ suite('gr-comment-api tests', () => {
             line: 1,
             rootId: '07',
             range: undefined,
-            diffSide: undefined,
           }, {
             comments: [
               {
@@ -682,7 +661,6 @@ suite('gr-comment-api tests', () => {
             line: 1,
             rootId: '09',
             range: undefined,
-            diffSide: undefined,
             commentSide: CommentSide.REVISION,
           }, {
             comments: [
@@ -701,7 +679,6 @@ suite('gr-comment-api tests', () => {
             line: 1,
             rootId: '10',
             range: undefined,
-            diffSide: undefined,
           }, {
             comments: [
               {
@@ -717,7 +694,6 @@ suite('gr-comment-api tests', () => {
             path: 'file/four',
             line: 1,
             range: undefined,
-            diffSide: undefined,
             commentSide: CommentSide.REVISION,
           }, {
             comments: [
@@ -735,7 +711,6 @@ suite('gr-comment-api tests', () => {
             path: 'file/two',
             line: 1,
             range: undefined,
-            diffSide: undefined,
             commentSide: CommentSide.REVISION,
           }, {
             comments: [
@@ -755,7 +730,6 @@ suite('gr-comment-api tests', () => {
             path: 'file/one',
             line: 1,
             range: undefined,
-            diffSide: undefined,
           },
         ];
         const threads = element._changeComments.getAllThreadsForChange();

polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.js

@@ -973,14 +973,12 @@ suite('gr-diff-host tests', () => {
         message: 'i like you, jack',
         updated: '2015-12-23 15:00:20.396000000',
         line: 1,
-        diffSide: Side.LEFT,
         patch_set: 1,
         path: 'some/path',
       }, {
         id: 'jacks_reply',
         message: 'i like you, too',
         updated: '2015-12-24 15:01:20.396000000',
-        diffSide: Side.LEFT,
         line: 1,
         in_reply_to: 'sallys_confession',
         patch_set: 1,
@@ -989,7 +987,6 @@ suite('gr-diff-host tests', () => {
       {
         id: 'new_draft',
         message: 'i do not like either of you',
-        diffSide: Side.LEFT,
         __draft: true,
         updated: '2015-12-20 15:01:20.396000000',
         patch_set: 1,
@@ -997,7 +994,8 @@ suite('gr-diff-host tests', () => {
       },
     ];
 
-    const actualThreads = createCommentThreads(comments);
+    const actualThreads = createCommentThreads(comments,
+        {basePatchNum: 1, patchNum: 4});
 
     assert.equal(actualThreads.length, 2);
 
@@ -1015,7 +1013,7 @@ suite('gr-diff-host tests', () => {
     assert.equal(actualThreads[1].line, FILE);
   });
 
-  test('_createThreads inherits patchNum and range', () => {
+  test('_createThreads derives patchNum and range', () => {
     const comments = [{
       id: 'betsys_confession',
       message: 'i like you, jack',
@@ -1028,7 +1026,6 @@ suite('gr-diff-host tests', () => {
       },
       patch_set: 5,
       path: '/p',
-      diffSide: Side.LEFT,
       line: 1,
     }];
 
@@ -1050,7 +1047,6 @@ suite('gr-diff-host tests', () => {
             end_character: 2,
           },
           patch_set: 5,
-          diffSide: Side.LEFT,
           line: 1,
         }],
         patchNum: 5,
@@ -1065,7 +1061,7 @@ suite('gr-diff-host tests', () => {
     ];
 
     assert.deepEqual(
-        createCommentThreads(comments),
+        createCommentThreads(comments, {basePatchNum: 5, patchNum: 10}),
         expectedThreads);
   });
 

polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts

@@ -93,7 +93,7 @@ import {hasOwnProperty} from '../../../utils/common-util';
 import {GrApplyFixDialog} from '../gr-apply-fix-dialog/gr-apply-fix-dialog';
 import {LineOfInterest} from '../gr-diff/gr-diff';
 import {RevisionInfo as RevisionInfoObj} from '../../shared/revision-info/revision-info';
-import {CommentMap} from '../../../utils/comment-util';
+import {CommentMap, isInBaseOfPatchRange} from '../../../utils/comment-util';
 import {AppElementParams} from '../../gr-app-types';
 import {CustomKeyboardEvent, OpenFixPreviewEvent} from '../../../types/events';
 import {PORTING_COMMENTS_DIFF_LATENCY_LABEL} from '../../../services/gr-reporting/gr-reporting';
@@ -974,7 +974,7 @@ export class GrDiffView extends KeyboardShortcutMixin(
           patchNum: latestPatchNum,
           basePatchNum: ParentPatchSetNum,
         };
-        leftSide = comment.diffSide === Side.LEFT;
+        leftSide = isInBaseOfPatchRange(comment, this._patchRange);
       } else {
         this._patchRange = {
           patchNum: latestPatchNum,

polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts

@@ -508,7 +508,6 @@ export class GrCommentThread extends KeyboardShortcutMixin(
       if (rootComment.patch_set !== undefined)
         d.patch_set = rootComment.patch_set;
       if (rootComment.side !== undefined) d.side = rootComment.side;
-      if (rootComment.diffSide !== undefined) d.diffSide = rootComment.diffSide;
       if (rootComment.line !== undefined) d.line = rootComment.line;
       if (rootComment.range !== undefined) d.range = rootComment.range;
       if (rootComment.parent !== undefined) d.parent = rootComment.parent;
@@ -517,7 +516,6 @@ export class GrCommentThread extends KeyboardShortcutMixin(
       d.path = this.path;
       d.patch_set = this.patchNum;
       d.side = this._getSide(this.isOnParent);
-      d.diffSide = this.diffSide;
 
       if (lineNum && lineNum !== FILE) {
         d.line = lineNum;

polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread_html.ts

@@ -117,7 +117,6 @@ export const htmlTemplate = html`
         draft="[[_isDraft(comment)]]"
         show-actions="[[_showActions]]"
         show-patchset="[[showPatchset]]"
-        diff-side="[[comment.diffSide]]"
         side="[[comment.side]]"
         project-config="[[_projectConfig]]"
         on-create-fix-comment="_handleCommentFix"

polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread_test.ts

@@ -348,7 +348,6 @@ suite('comment action tests with unresolved thread', () => {
         path: '/path/to/file.txt',
         unresolved: true,
         patch_set: 3 as PatchSetNum,
-        diffSide: Side.LEFT,
       },
     ];
     flush();
@@ -833,7 +832,6 @@ suite('comment action tests with unresolved thread', () => {
 
   test('_newDraft with root', () => {
     const draft = element._newDraft();
-    assert.equal(draft.diffSide, Side.LEFT);
     assert.equal(draft.patch_set, 3 as PatchSetNum);
   });
 
@@ -842,7 +840,6 @@ suite('comment action tests with unresolved thread', () => {
     element.diffSide = Side.RIGHT;
     element.patchNum = 2 as PatchSetNum;
     const draft = element._newDraft();
-    assert.equal(draft.diffSide, Side.RIGHT);
     assert.equal(draft.patch_set, 2 as PatchSetNum);
   });
 

polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts

@@ -51,7 +51,6 @@ import {
 import {GrButton} from '../gr-button/gr-button';
 import {GrConfirmDeleteCommentDialog} from '../gr-confirm-delete-comment-dialog/gr-confirm-delete-comment-dialog';
 import {GrDialog} from '../gr-dialog/gr-dialog';
-import {Side} from '../../../constants/constants';
 import {
   isDraft,
   UIComment,
@@ -218,9 +217,6 @@ export class GrComment extends KeyboardShortcutMixin(
   @property({type: String, observer: '_messageTextChanged'})
   _messageText = '';
 
-  @property({type: String})
-  diffSide?: Side;
-
   @property({type: String})
   side?: string;
 
@@ -450,7 +446,6 @@ export class GrComment extends KeyboardShortcutMixin(
           if (this.comment?.__draftID) {
             resComment.__draftID = this.comment.__draftID;
           }
-          resComment.diffSide = this.diffSide;
           this.comment = resComment;
           this._fireSave();
           return obj;

polygerrit-ui/app/elements/shared/gr-comment/gr-comment_test.js

@@ -910,7 +910,6 @@ suite('gr-comment tests', () => {
 
         assert.deepEqual(dispatchEventStub.lastCall.args[0].detail, {
           comment: {
-            diffSide: Side.RIGHT,
             __draft: true,
             __draftID: 'temp_draft_id',
             id: 'baf0414d_40572e03',

polygerrit-ui/app/utils/comment-util.ts

@@ -22,11 +22,14 @@ import {
   Timestamp,
   UrlEncodedCommentId,
   CommentRange,
+  PatchRange,
+  ParentPatchSetNum,
 } from '../types/common';
 import {CommentSide, Side} from '../constants/constants';
 import {parseDate} from './date-util';
 import {LineNumber} from '../elements/diff/gr-diff/gr-diff-line';
 import {CommentIdToCommentThreadMap} from '../elements/diff/gr-comment-api/gr-comment-api';
+import {isMergeParent, getParentIndex, patchNumEquals} from './patch-set-util';
 
 export interface DraftCommentProps {
   __draft?: boolean;
@@ -42,9 +45,6 @@ export type DraftInfo = CommentBasics & DraftCommentProps;
 export type Comment = DraftInfo | CommentInfo | RobotCommentInfo;
 
 export interface UIStateCommentProps {
-  // diffSide is used by gr-diff to decide which side(left/right) to show
-  // the comment
-  diffSide?: Side;
   collapsed?: boolean;
   // TODO(TS): Consider allowing this only for drafts.
   __editing?: boolean;
@@ -97,7 +97,10 @@ export function sortComments<T extends SortableComment>(comments: T[]): T[] {
   });
 }
 
-export function createCommentThreads(comments: UIComment[]) {
+export function createCommentThreads(
+  comments: UIComment[],
+  patchRange?: PatchRange
+) {
   const sortedComments = sortComments(comments);
   const threads: CommentThread[] = [];
   const idThreadMap: CommentIdToCommentThreadMap = {};
@@ -126,8 +129,14 @@ export function createCommentThreads(comments: UIComment[]) {
       line: comment.line,
       range: comment.range,
       rootId: comment.id,
-      diffSide: comment.diffSide,
     };
+    if (patchRange) {
+      if (isInBaseOfPatchRange(comment, patchRange))
+        newThread.diffSide = Side.LEFT;
+      else if (isInRevisionOfPatchRange(comment, patchRange))
+        newThread.diffSide = Side.RIGHT;
+      else throw new Error('comment does not belong in given patchrange');
+    }
     if (!comment.line && !comment.range) {
       newThread.line = 'FILE';
     }
@@ -162,3 +171,64 @@ export function isUnresolved(thread?: CommentThread): boolean {
 export function isDraftThread(thread?: CommentThread): boolean {
   return isDraft(getLastComment(thread));
 }
+
+/**
+ * Whether the given comment should be included in the base side of the
+ * given patch range.
+ */
+export function isInBaseOfPatchRange(
+  comment: CommentBasics,
+  range: PatchRange
+) {
+  // If the base of the patch range is a parent of a merge, and the comment
+  // appears on a specific parent then only show the comment if the parent
+  // index of the comment matches that of the range.
+  if (comment.parent && comment.side === CommentSide.PARENT) {
+    return (
+      isMergeParent(range.basePatchNum) &&
+      comment.parent === getParentIndex(range.basePatchNum)
+    );
+  }
+
+  // If the base of the range is the parent of the patch:
+  if (
+    range.basePatchNum === ParentPatchSetNum &&
+    comment.side === CommentSide.PARENT &&
+    patchNumEquals(comment.patch_set, range.patchNum)
+  ) {
+    return true;
+  }
+  // If the base of the range is not the parent of the patch:
+  return (
+    range.basePatchNum !== ParentPatchSetNum &&
+    comment.side !== CommentSide.PARENT &&
+    patchNumEquals(comment.patch_set, range.basePatchNum)
+  );
+}
+
+/**
+ * Whether the given comment should be included in the revision side of the
+ * given patch range.
+ */
+export function isInRevisionOfPatchRange(
+  comment: CommentBasics,
+  range: PatchRange
+) {
+  return (
+    comment.side !== CommentSide.PARENT &&
+    patchNumEquals(comment.patch_set, range.patchNum)
+  );
+}
+
+/**
+ * Whether the given comment should be included in the given patch range.
+ */
+export function isInPatchRange(
+  comment: CommentBasics,
+  range: PatchRange
+): boolean {
+  return (
+    isInBaseOfPatchRange(comment, range) ||
+    isInRevisionOfPatchRange(comment, range)
+  );
+}