diff --git a/polygerrit-ui/app/elements/core/gr-router/gr-router.js b/polygerrit-ui/app/elements/core/gr-router/gr-router.js index f550af4acf..38997225e0 100644 --- a/polygerrit-ui/app/elements/core/gr-router/gr-router.js +++ b/polygerrit-ui/app/elements/core/gr-router/gr-router.js @@ -91,6 +91,15 @@ SETTINGS_LEGACY: /^\/settings\/VE\/(\S+)/, }; + /** + * Pattern to recognize and parse the diff line locations as they appear in + * the hash of diff URLs. In this format, a number on its own indicates that + * line number in the revision of the diff. A number prefixed by either an 'a' + * or a 'b' indicates that line number of the base of the diff. + * @type {RegExp} + */ + const LINE_ADDRESS_PATTERN = /^([ab]?)(\d+)$/; + // Polymer makes `app` intrinsically defined on the window by virtue of the // custom element having the id "app", but it is made explicit here. const app = document.querySelector('#app'); @@ -300,6 +309,15 @@ return canonicalPath.split('#').slice(1).join('#'); }, + _parseLineAddress(hash) { + const match = hash.match(LINE_ADDRESS_PATTERN); + if (!match) { return null; } + return { + leftSide: !!match[1], + lineNum: parseInt(match[2], 10), + }; + }, + /** * Check to see if the user is logged in and return a promise that only * resolves if the user is logged in. If the user us not logged in, the @@ -745,6 +763,8 @@ }, _handleChangeOrDiffRoute(ctx) { + const isDiffView = ctx.params[8]; + // Parameter order is based on the regex group number matched. const params = { project: ctx.params[0], @@ -752,17 +772,23 @@ basePatchNum: ctx.params[4], patchNum: ctx.params[6], path: ctx.params[8], - view: ctx.params[8] ? - Gerrit.Nav.View.DIFF : Gerrit.Nav.View.CHANGE, - hash: ctx.hash, + view: isDiffView ? Gerrit.Nav.View.DIFF : Gerrit.Nav.View.CHANGE, }; + + if (isDiffView) { + const address = this._parseLineAddress(ctx.hash); + if (address) { + params.leftSide = address.leftSide; + params.lineNum = address.lineNum; + } + } + const needsRedirect = this._normalizePatchRangeParams(params); if (needsRedirect) { this._redirect(this._generateUrl(params)); } else { this._setParams(params); - this._restAPI.setInProjectLookup(params.changeNum, - params.project); + this._restAPI.setInProjectLookup(params.changeNum, params.project); } }, @@ -793,10 +819,15 @@ basePatchNum: ctx.params[2], patchNum: ctx.params[4], path: ctx.params[5], - hash: ctx.hash, view: Gerrit.Nav.View.DIFF, }; + const address = this._parseLineAddress(ctx.hash); + if (address) { + params.leftSide = address.leftSide; + params.lineNum = address.lineNum; + } + this._normalizeLegacyRouteParams(params); }, diff --git a/polygerrit-ui/app/elements/core/gr-router/gr-router_test.html b/polygerrit-ui/app/elements/core/gr-router/gr-router_test.html index fe0c812f5d..3864a7d682 100644 --- a/polygerrit-ui/app/elements/core/gr-router/gr-router_test.html +++ b/polygerrit-ui/app/elements/core/gr-router/gr-router_test.html @@ -65,6 +65,39 @@ limitations under the License. assert.equal(hash, 'foo#bar#baz'); }); + suite('_parseLineAddress', () => { + test('returns null for empty and invalid hashes', () => { + let actual = element._parseLineAddress(''); + assert.isNull(actual); + + actual = element._parseLineAddress('foobar'); + assert.isNull(actual); + + actual = element._parseLineAddress('foo123'); + assert.isNull(actual); + + actual = element._parseLineAddress('123bar'); + assert.isNull(actual); + }); + + test('parses correctly', () => { + let actual = element._parseLineAddress('1234'); + assert.isOk(actual); + assert.equal(actual.lineNum, 1234); + assert.isFalse(actual.leftSide); + + actual = element._parseLineAddress('a4'); + assert.isOk(actual); + assert.equal(actual.lineNum, 4); + assert.isTrue(actual.leftSide); + + actual = element._parseLineAddress('b77'); + assert.isOk(actual); + assert.equal(actual.lineNum, 77); + assert.isTrue(actual.leftSide); + }); + }); + test('_startRouter requires auth for the right handlers', () => { // This test encodes the lists of route handler methods that gr-router // automatically checks for authentication before triggering. @@ -903,8 +936,9 @@ limitations under the License. basePatchNum: 3, patchNum: 8, view: Gerrit.Nav.View.DIFF, - hash: 'b123', path: 'foo/bar', + lineNum: 123, + leftSide: true, }); }); @@ -967,7 +1001,6 @@ limitations under the License. basePatchNum: 4, patchNum: 7, path: null, - hash: '', }); assert.isFalse(redirectStub.called); assert.isTrue(normalizeRangeStub.called); @@ -984,7 +1017,8 @@ limitations under the License. basePatchNum: 4, patchNum: 7, path: 'foo/bar/baz', - hash: 'b44', + leftSide: true, + lineNum: 44, }); assert.isFalse(redirectStub.called); assert.isTrue(normalizeRangeStub.called); diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js index 409fd09bb3..56be27def4 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js +++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js @@ -26,8 +26,6 @@ RIGHT: 'right', }; - const HASH_PATTERN = /^[ab]?\d+$/; - Polymer({ is: 'gr-diff-view', @@ -467,7 +465,7 @@ _paramsChanged(value) { if (value.view !== Gerrit.Nav.View.DIFF) { return; } - this._loadHash(this.params.hash); + this._initCursor(this.params); this._changeNum = value.changeNum; this._patchRange = { @@ -539,18 +537,16 @@ }, /** - * If the URL hash is a diff address then configure the diff cursor. + * If the params specify a diff address then configure the diff cursor. */ - _loadHash(hash) { - hash = (hash || '').replace(/^#/, ''); - if (!HASH_PATTERN.test(hash)) { return; } - if (hash[0] === 'a' || hash[0] === 'b') { + _initCursor(params) { + if (params.lineNum === undefined) { return; } + if (params.leftSide) { this.$.cursor.side = DiffSides.LEFT; - hash = hash.substring(1); } else { this.$.cursor.side = DiffSides.RIGHT; } - this.$.cursor.initialLineNumber = parseInt(hash, 10); + this.$.cursor.initialLineNumber = params.lineNum; }, _pathChanged(path) { diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.html b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.html index 250e0f509b..e239bf0457 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.html +++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.html @@ -509,7 +509,7 @@ limitations under the License. getDiffComments() { return Promise.resolve({}); }, }); sandbox.stub(element.$.diff, 'reload'); - sandbox.stub(element, '_loadHash'); + sandbox.stub(element, '_initCursor'); element._loggedIn = true; element.params = { @@ -522,7 +522,7 @@ limitations under the License. }; flush(() => { - assert.isTrue(element._loadHash.lastCall.calledWithExactly(10)); + assert.isTrue(element._initCursor.calledOnce); done(); }); }); @@ -573,31 +573,31 @@ limitations under the License. assert.equal(select.nativeSelect.value, 'SIDE_BY_SIDE'); }); - test('_loadHash', () => { + test('_initCursor', () => { assert.isNotOk(element.$.cursor.initialLineNumber); - // Ignores invalid hashes: - element._loadHash('not valid'); + // Does nothing when params specify no cursor address: + element._initCursor({}); assert.isNotOk(element.$.cursor.initialLineNumber); - // Ignores null hash: - element._loadHash(null); + // Does nothing when params specify side but no number: + element._initCursor({leftSide: true}); assert.isNotOk(element.$.cursor.initialLineNumber); - // Revision hash: - element._loadHash('234'); + // Revision hash: specifies lineNum but not side. + element._initCursor({lineNum: 234}); assert.equal(element.$.cursor.initialLineNumber, 234); assert.equal(element.$.cursor.side, 'right'); - // Base hash: - element._loadHash('b345'); + // Base hash: specifies lineNum and side. + element._initCursor({leftSide: true, lineNum: 345}); assert.equal(element.$.cursor.initialLineNumber, 345); assert.equal(element.$.cursor.side, 'left'); - // GWT-style base hash: - element._loadHash('a123'); + // Specifies right side: + element._initCursor({leftSide: false, lineNum: 123}); assert.equal(element.$.cursor.initialLineNumber, 123); - assert.equal(element.$.cursor.side, 'left'); + assert.equal(element.$.cursor.side, 'right'); }); test('_shortenPath with long path should add ellipsis', () => {