Merge "Update URL generation in gr-comment-list"

polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html

@@ -523,7 +523,7 @@ limitations under the License.
           messages="[[_change.messages]]"
           reviewer-updates="[[_change.reviewer_updates]]"
           comments="[[_comments]]"
-          project-config="[[_projectConfig]]"
+          project-name="[[_change.project]]"
           show-reply-buttons="[[_loggedIn]]"
           on-reply="_handleMessageReply"></gr-messages-list>
     </div>

polygerrit-ui/app/elements/change/gr-comment-list/gr-comment-list.html

@@ -18,6 +18,7 @@ limitations under the License.
 <link rel="import" href="../../../behaviors/gr-path-list-behavior/gr-path-list-behavior.html">
 <link rel="import" href="../../../behaviors/gr-url-encoding-behavior.html">
 <link rel="import" href="../../../bower_components/polymer/polymer.html">
+<link rel="import" href="../../core/gr-navigation/gr-navigation.html">
 <link rel="import" href="../../shared/gr-formatted-text/gr-formatted-text.html">
 <link rel="import" href="../../../styles/shared-styles.html">
 
@@ -76,7 +77,7 @@ limitations under the License.
               class="message"
               no-trailing-margin
               content="[[comment.message]]"
-              config="[[projectConfig.commentlinks]]"></gr-formatted-text>
+              config="[[commentLinks]]"></gr-formatted-text>
         </div>
       </template>
     </template>

polygerrit-ui/app/elements/change/gr-comment-list/gr-comment-list.js

@@ -30,7 +30,8 @@
       changeNum: Number,
       comments: Object,
       patchNum: Number,
-      projectConfig: Object,
+      commentLinks: Object,
+      projectName: String,
     },
 
     _computeFilesFromComments(comments) {
@@ -39,8 +40,8 @@
     },
 
     _computeFileDiffURL(file, changeNum, patchNum) {
-      return this.getBaseUrl() + '/c/' + changeNum + '/' + patchNum + '/' +
+      return Gerrit.Nav.getUrlForDiffById(this.changeNum, this.projectName,
-          this.encodeURL(file);
+          file, patchNum);
     },
 
     _computeFileDisplayName(path) {
@@ -57,13 +58,8 @@
     },
 
     _computeDiffLineURL(file, changeNum, patchNum, comment) {
-      let diffURL = this._computeFileDiffURL(file, changeNum, patchNum);
+      return Gerrit.Nav.getUrlForDiffById(this.changeNum, this.projectName,
-      if (comment.line) {
+          file, patchNum, null, comment.line, this._isOnParent(comment));
-        diffURL += '#';
-        if (this._isOnParent(comment)) { diffURL += 'b'; }
-        diffURL += comment.line;
-      }
-      return diffURL;
     },
 
     _computeCommentsForFile(comments, file) {

polygerrit-ui/app/elements/change/gr-comment-list/gr-comment-list_test.html

@@ -60,12 +60,6 @@ limitations under the License.
       assert.deepEqual(element._computeFilesFromComments(null), []);
     });
 
-    test('_computeFileDiffURL', () => {
-      const expected = '/c/change/patch/file';
-      const actual = element._computeFileDiffURL('file', 'change', 'patch');
-      assert.equal(actual, expected);
-    });
-
     test('_computeFileDisplayName', () => {
       assert.equal(element._computeFileDisplayName('/COMMIT_MSG'),
           'Commit message');
@@ -75,27 +69,6 @@ limitations under the License.
           '/foo/bar/baz');
     });
 
-    test('_computeDiffLineURL', () => {
-      const comment = {line: 123, side: 'REVISION', patch_set: 10};
-      let expected = '/c/change/patch/file#123';
-      let actual = element._computeDiffLineURL('file', 'change', 'patch',
-          comment);
-      assert.equal(actual, expected);
-
-      comment.line = 321;
-      comment.side = 'PARENT';
-
-      expected = '/c/change/patch/file#b321';
-      actual = element._computeDiffLineURL('file', 'change', 'patch', comment);
-    });
-
-    test('_computeDiffLineURL encoding', () => {
-      const comment = {line: 123, side: 'REVISION', patch_set: 10};
-      const expected = '/c/123/2/x%252By.md#123';
-      const actual = element._computeDiffLineURL('x+y.md', '123', '2', comment);
-      assert.equal(actual, expected);
-    });
-
     test('_computePatchDisplayName', () => {
       const comment = {line: 123, side: 'REVISION', patch_set: 10};
 

polygerrit-ui/app/elements/change/gr-message/gr-message.html

@@ -152,12 +152,13 @@ limitations under the License.
             <gr-formatted-text
                 class="message hideOnCollapsed"
                 content="[[message.message]]"
-                config="[[projectConfig.commentlinks]]"></gr-formatted-text>
+                config="[[_commentLinks]]"></gr-formatted-text>
             <gr-comment-list
                 comments="[[comments]]"
                 change-num="[[changeNum]]"
                 patch-num="[[message._revision_number]]"
-                project-config="[[projectConfig]]"></gr-comment-list>
+                project-name="[[projectName]]"
+                comment-links="[[_commentLinks]]"></gr-comment-list>
           </div>
         </template>
         <template is="dom-if" if="[[_computeIsReviewerUpdate(message)]]">

polygerrit-ui/app/elements/change/gr-message/gr-message.js

@@ -74,7 +74,11 @@
         type: Boolean,
         computed: '_computeShowReplyButton(message, _loggedIn)',
       },
-      projectConfig: Object,
+      projectName: {
+        type: String,
+        observer: '_projectNameChanged',
+      },
+      _commentLinks: Object,
       // Computed property needed to trigger Polymer value observing.
       _expanded: {
         type: Object,
@@ -240,5 +244,11 @@
 
       return this.getAnonymousName(this.config);
     },
+
+    _projectNameChanged(name) {
+      this.$.restAPI.getProjectConfig(name).then(config => {
+        this._commentLinks = config.commentlinks;
+      });
+    },
   });
 })();

polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list.html

@@ -95,7 +95,7 @@ limitations under the License.
           message="[[message]]"
           comments="[[_computeCommentsForMessage(comments, message)]]"
           hide-automated="[[_hideAutomated]]"
-          project-config="[[projectConfig]]"
+          project-name="[[projectName]]"
           show-reply-button="[[showReplyButtons]]"
           on-scroll-to="_handleScrollTo"
           data-message-id$="[[message.id]]"></gr-message>

polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list.js

@@ -36,7 +36,7 @@
         value() { return []; },
       },
       comments: Object,
-      projectConfig: Object,
+      projectName: String,
       showReplyButtons: {
         type: Boolean,
         value: false,

polygerrit-ui/app/elements/core/gr-navigation/gr-navigation.html

@@ -43,6 +43,10 @@ limitations under the License.
     //    - `basePatchNum`, optional, Number, the patch for the left-hand-side
     //        of the diff. If `basePatchNum` is provided, then `patchNum` must
     //        also be provided.
+    //    - `lineNum`, optional, Number, the line number to be selected on load.
+    //    - `leftSide`, optional, Boolean, if a `lineNum` is provided, a value
+    //        of true selects the line from base of the patch range. False by
+    //        default.
 
     window.Gerrit = window.Gerrit || {};
 
@@ -166,9 +170,9 @@ limitations under the License.
 
       /**
        * @param {!Object} change The change object.
-       * @param {number} opt_patchNum
+       * @param {number=} opt_patchNum
-       * @param {number|string} opt_basePatchNum The string 'PARENT' can be used
+       * @param {number|string=} opt_basePatchNum The string 'PARENT' can be
-       *     for none.
+       *     used for none.
        * @return {string}
        */
       getUrlForChange(change, opt_patchNum, opt_basePatchNum) {
@@ -187,9 +191,9 @@ limitations under the License.
 
       /**
        * @param {!Object} change The change object.
-       * @param {number} opt_patchNum
+       * @param {number=} opt_patchNum
-       * @param {number|string} opt_basePatchNum The string 'PARENT' can be used
+       * @param {number|string=} opt_basePatchNum The string 'PARENT' can be
-       *     for none.
+       *     used for none.
        * @return {string}
        */
       navigateToChange(change, opt_patchNum, opt_basePatchNum) {
@@ -199,13 +203,30 @@ limitations under the License.
 
       /**
        * @param {!Object} change The change object.
-       * @param {!String} path The file path.
+       * @param {!string} path The file path.
-       * @param {number} opt_patchNum
+       * @param {number=} opt_patchNum
-       * @param {number|string} opt_basePatchNum The string 'PARENT' can be used
+       * @param {number|string=} opt_basePatchNum The string 'PARENT' can be
-       *     for none.
+       *     used for none.
        * @return {string}
        */
       getUrlForDiff(change, path, opt_patchNum, opt_basePatchNum) {
+        return this.getUrlForDiffById(change._number, change.project, path,
+            opt_patchNum, opt_basePatchNum);
+      },
+
+      /**
+       * @param {!number} change The change object.
+       * @param {!string} projectName The name of the project.
+       * @param {!string} path The file path.
+       * @param {number=} opt_patchNum
+       * @param {number|string=} opt_basePatchNum The string 'PARENT' can be
+       *     used for none.
+       * @param {number=} opt_lineNum
+       * @param {boolean=} opt_leftSide
+       * @return {string}
+       */
+      getUrlForDiffById(changeNum, projectName, path, opt_patchNum,
+          opt_basePatchNum, opt_lineNum, opt_leftSide) {
         if (opt_basePatchNum === PARENT_PATCHNUM) {
           opt_basePatchNum = undefined;
         }
@@ -213,19 +234,22 @@ limitations under the License.
         this._checkPatchRange(opt_patchNum, opt_basePatchNum);
         return this._getUrlFor({
           view: Gerrit.Nav.View.DIFF,
-          changeNum: change._number,
+          changeNum,
+          projectName,
           path,
           patchNum: opt_patchNum,
           basePatchNum: opt_basePatchNum,
+          lineNum: opt_lineNum,
+          leftSide: opt_leftSide,
         });
       },
 
       /**
        * @param {!Object} change The change object.
-       * @param {!String} path The file path.
+       * @param {!string} path The file path.
-       * @param {number} opt_patchNum
+       * @param {number=} opt_patchNum
-       * @param {number|string} opt_basePatchNum The string 'PARENT' can be used
+       * @param {number|string=} opt_basePatchNum The string 'PARENT' can be
-       *     for none.
+       *     used for none.
        */
       navigateToDiff(change, path, opt_patchNum, opt_basePatchNum) {
         this._navigate(this.getUrlForDiff(change, path, opt_patchNum,

polygerrit-ui/app/elements/core/gr-router/gr-router.js

@@ -580,6 +580,12 @@
         if (range.length) { range = '/' + range; }
 
         url = `/c/${params.changeNum}${range}/${encode(params.path, true)}`;
+
+        if (params.lineNum) {
+          url += '#';
+          if (params.leftSide) { url += 'b'; }
+          url += params.lineNum;
+        }
       } else {
         throw new Error('Can\'t generate');
       }

polygerrit-ui/app/elements/core/gr-router/gr-router_test.html

@@ -93,6 +93,13 @@ limitations under the License.
         delete params.basePatchNum;
         assert.equal(element._generateUrl(params),
             '/c/42/2/foo+bar/my%252Bfile.txt%2525');
+
+        params.path = 'file.cpp';
+        params.lineNum = 123;
+        assert.equal(element._generateUrl(params), '/c/42/2/file.cpp#123');
+
+        params.leftSide = true;
+        assert.equal(element._generateUrl(params), '/c/42/2/file.cpp#b123');
       });
     });
   });