Merge "Remove diffSide property from UIComment"

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

@@ -18,16 +18,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,
@@ -37,7 +31,6 @@ import {
   RevisionId,
 } from '../../../types/common';
 import {hasOwnProperty} from '../../../utils/common-util';
-import {CommentSide, Side} from '../../../constants/constants';
 import {
   Comment,
   CommentMap,
@@ -49,6 +42,7 @@ import {
   UIHuman,
   UIRobot,
   createCommentThreads,
+  isInPatchRange,
 } from '../../../utils/comment-util';
 import {PatchSetFile, PatchNumOnly, isPatchSetFile} from '../../../types/types';
 import {appContext} from '../../../services/app-context';
@@ -57,11 +51,6 @@ export type CommentIdToCommentThreadMap = {
   [urlEncodedCommentId: string]: CommentThread;
 };
 
-export interface TwoSidesComments {
-  left: UIComment[];
-  right: UIComment[];
-}
-
 export class ChangeComments {
   private readonly _comments: {[path: string]: UIHuman[]};
 
@@ -160,7 +149,7 @@ export class ChangeComments {
             if (!patchRange) {
               return true;
             }
-            return this._isInPatchRange(c, patchRange);
+            return isInPatchRange(c, patchRange);
           })
         ) {
           commentMap[path] = true;
@@ -300,25 +289,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))
-    );
   }
 
   /**
@@ -331,10 +306,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[] = [];
@@ -352,33 +324,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))
-    );
   }
 
   /**
@@ -393,19 +352,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;
   }
@@ -471,58 +421,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

@@ -92,7 +92,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';
@@ -973,7 +973,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

@@ -507,7 +507,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;
@@ -516,7 +515,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

@@ -50,7 +50,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,
@@ -217,9 +216,6 @@ export class GrComment extends KeyboardShortcutMixin(
   @property({type: String, observer: '_messageTextChanged'})
   _messageText = '';
 
-  @property({type: String})
-  diffSide?: Side;
-
   @property({type: String})
   side?: string;
 
@@ -449,7 +445,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)
+  );
+}