Support “diff against” in the file list Bug: Issue 3936 Change-Id: I07f242a8cb7be063a2d56dbe3f4b6a28143588c0
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html index 8c6c216..1f5d9e1 100644 --- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html +++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html
@@ -273,7 +273,7 @@ </section> <gr-file-list id="fileList" change-num="[[_changeNum]]" - patch-num="{{_patchRange.patchNum}}" + patch-range="[[_patchRange]]" comments="[[_comments]]" drafts="[[_diffDrafts]]" revisions="[[_change.revisions]]"
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js index a8012a0..a3d2146 100644 --- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js +++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
@@ -179,7 +179,14 @@ basePatchNum: value.basePatchNum || 'PARENT', }; - this._resetStateIfNecessary(); + // If the change number or patch range is different, then reset the + // selected file index. + var patchRangeState = this.viewState.patchRange; + if (this.viewState.changeNum !== this._changeNum || + patchRangeState.basePatchNum !== this._patchRange.basePatchNum || + patchRangeState.patchNum !== this._patchRange.patchNum) { + this._resetFileListViewState(); + } if (!this._changeNum) { return; @@ -212,17 +219,10 @@ }.bind(this)); }, - _resetStateIfNecessary: function() { - // If the change number or patch range is different, then reset the - // selected file index. - if (this.viewState.changeNum !== this._changeNum || - this.viewState.patchRange.basePatchNum !== - this._patchRange.basePatchNum || - this.viewState.patchRange.patchNum !== this._patchRange.patchNum) { - this.set('viewState.selectedFileIndex', 0); - this.set('viewState.changeNum', this._changeNum); - this.set('viewState.patchRange', this._patchRange); - } + _resetFileListViewState: function() { + this.set('viewState.selectedFileIndex', 0); + this.set('viewState.changeNum', this._changeNum); + this.set('viewState.patchRange', this._patchRange); }, _changeChanged: function(change) {
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html index 104b27f2..7ce687c 100644 --- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html +++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html
@@ -34,7 +34,7 @@ justify-content: space-between; margin-bottom: .5em; } - header label { + .diffAgainst { font-weight: normal; } .positionIndicator, @@ -125,12 +125,22 @@ </style> <header> <div>Files</div> - <label hidden> - Diff against - <select></select> - </label> + <div class="diffAgainst"> + <label> + Diff against + <select on-change="_handlePatchChange"> + <option value="PARENT">Base</option> + <template is="dom-repeat" items="[[_computePatchSets(revisions, patchRange.*)]]" as="patchNum"> + <option + value$="[[patchNum]]" + selected$="[[_computePatchSetSelected(patchNum, patchRange.basePatchNum)]]" + disabled$="[[_computePatchSetDisabled(patchNum, patchRange.patchNum)]]">[[patchNum]]</option> + </template> + </select> + </label> + </div> </header> - <template is="dom-repeat" items="{{_files}}" as="file"> + <template is="dom-repeat" items="[[_files]]" as="file"> <div class="row" selected$="[[_computeFileSelected(index, selectedIndex)]]"> <div class="positionIndicator">▶</div> <div class="reviewed" hidden$="[[!_loggedIn]]" hidden> @@ -140,7 +150,7 @@ <div class$="[[_computeClass('status', file.__path)]]"> [[_computeFileStatus(file.status)]] </div> - <a class="path" href$="[[_computeDiffURL(changeNum, patchNum, file.__path)]]"> + <a class="path" href$="[[_computeDiffURL(changeNum, patchRange, file.__path)]]"> <div title$="[[_computeFileDisplayName(file.__path)]]"> [[_computeFileDisplayName(file.__path)]] </div> @@ -150,8 +160,8 @@ </div> </a> <div class="comments"> - <span class="drafts">[[_computeDraftsString(drafts, patchNum, file.__path)]]</span> - [[_computeCommentsString(comments, patchNum, file.__path)]] + <span class="drafts">[[_computeDraftsString(drafts, patchRange.patchNum, file.__path)]]</span> + [[_computeCommentsString(comments, patchRange.patchNum, file.__path)]] </div> <div class$="[[_computeClass('stats', file.__path)]]"> <span class="added">+[[file.lines_inserted]]</span>
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js index 47b6abd..a9ce44e 100644 --- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js +++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js
@@ -20,6 +20,7 @@ is: 'gr-file-list', properties: { + patchRange: Object, patchNum: String, changeNum: String, comments: Object, @@ -50,7 +51,7 @@ ], reload: function() { - if (!this.changeNum || !this.patchNum) { + if (!this.changeNum || !this.patchRange.patchNum) { return Promise.resolve(); } @@ -73,6 +74,28 @@ return Promise.all(promises); }, + _computePatchSets: function(revisions) { + var patchNums = []; + for (var commit in revisions) { + patchNums.push(revisions[commit]._number); + } + return patchNums.sort(function(a, b) { return a - b; }); + }, + + _computePatchSetDisabled: function(patchNum, currentPatchNum) { + return parseInt(patchNum, 10) >= parseInt(currentPatchNum, 10); + }, + + _computePatchSetSelected: function(patchNum, basePatchNum) { + return parseInt(patchNum, 10) === parseInt(basePatchNum, 10); + }, + + _handlePatchChange: function(e) { + this.set('patchRange.basePatchNum', Polymer.dom(e).rootTarget.value); + page.show('/c/' + encodeURIComponent(this.changeNum) + '/' + + encodeURIComponent(this._patchRangeStr(this.patchRange))); + }, + _computeCommentsString: function(comments, patchNum, path) { return this._computeCountString(comments, patchNum, path, 'comment'); }, @@ -114,8 +137,8 @@ }, _saveReviewedState: function(path, reviewed) { - return this.$.restAPI.saveFileReviewed(this.changeNum, this.patchNum, - path, reviewed); + return this.$.restAPI.saveFileReviewed(this.changeNum, + this.patchRange.patchNum, path, reviewed); }, _getLoggedIn: function() { @@ -123,12 +146,13 @@ }, _getReviewedFiles: function() { - return this.$.restAPI.getReviewedFiles(this.changeNum, this.patchNum); + return this.$.restAPI.getReviewedFiles(this.changeNum, + this.patchRange.patchNum); }, _getFiles: function() { return this.$.restAPI.getChangeFilesAsSpeciallySortedArray( - this.changeNum, this.patchNum); + this.changeNum, this.patchRange); }, _handleKey: function(e) { @@ -164,7 +188,7 @@ if (opt_index != null) { this.selectedIndex = opt_index; } - page.show(this._computeDiffURL(this.changeNum, this.patchNum, + page.show(this._computeDiffURL(this.changeNum, this.patchRange, this._files[this.selectedIndex].__path)); }, @@ -176,8 +200,19 @@ return status || 'M'; }, - _computeDiffURL: function(changeNum, patchNum, path) { - return '/c/' + changeNum + '/' + patchNum + '/' + path; + _computeDiffURL: function(changeNum, patchRange, path) { + return '/c/' + + encodeURIComponent(changeNum) + + '/' + + encodeURIComponent(this._patchRangeStr(patchRange)) + + '/' + + path; + }, + + _patchRangeStr: function(patchRange) { + return patchRange.basePatchNum !== 'PARENT' ? + patchRange.basePatchNum + '..' + patchRange.patchNum : + patchRange.patchNum + ''; }, _computeFileDisplayName: function(path) {
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html index 64ef91f..f80d211 100644 --- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html +++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html
@@ -89,7 +89,10 @@ {__path: 'myfile.txt'}, ]; element.changeNum = '42'; - element.patchNum = '2'; + element.patchRange = { + basePatchNum: 'PARENT', + patchNum: '2', + }; element.selectedIndex = 0; flushAsynchronousOperations(); @@ -181,7 +184,10 @@ ]; element._reviewed = ['/COMMIT_MSG', 'myfile.txt']; element.changeNum = '42'; - element.patchNum = '2'; + element.patchRange = { + basePatchNum: 'PARENT', + patchNum: '2', + }; element.selectedIndex = 0; flushAsynchronousOperations(); @@ -205,5 +211,52 @@ saveStub.restore(); }); + + test('patch set from revisions', function() { + var patchNums = element._computePatchSets({ + rev3: {_number: 3}, + rev1: {_number: 1}, + rev4: {_number: 4}, + rev2: {_number: 2}, + }); + assert.deepEqual(patchNums, [1, 2, 3, 4]); + }); + + test('patch range string', function() { + assert.equal( + element._patchRangeStr({basePatchNum: 'PARENT', patchNum: '1'}), + '1'); + assert.equal( + element._patchRangeStr({basePatchNum: '1', patchNum: '3'}), + '1..3'); + }); + + test('diff against dropdown', function(done) { + var showStub = sinon.stub(page, 'show'); + element.changeNum = '42'; + element.patchRange = { + basePatchNum: 'PARENT', + patchNum: '3', + }; + element.revisions = { + rev1: {_number: 1}, + rev2: {_number: 2}, + rev3: {_number: 3}, + }; + flush(function() { + var selectEl = element.$$('select'); + assert.equal(selectEl.value, 'PARENT'); + assert.isTrue(element.$$('option[value="3"]').hasAttribute('disabled')); + selectEl.addEventListener('change', function() { + assert.equal(selectEl.value, '2'); + assert(showStub.lastCall.calledWithExactly('/c/42/2..3'), + 'Should navigate to /c/42/2..3'); + showStub.restore(); + done(); + }); + selectEl.value = '2'; + element.fire('change', {}, {node: selectEl}); + }); + }); }); </script>
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 41bf824..9145ba1 100644 --- a/polygerrit-ui/app/elements/core/gr-router/gr-router.js +++ b/polygerrit-ui/app/elements/core/gr-router/gr-router.js
@@ -74,7 +74,6 @@ // Matches /c/<changeNum>/[<basePatchNum>..][<patchNum>]. page(/^\/c\/(\d+)\/?(((\d+)(\.\.(\d+))?))?$/, function(ctx) { // Parameter order is based on the regex group number matched. - console.info(ctx) var params = { changeNum: ctx.params[0], basePatchNum: ctx.params[3],
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html index 4480319..b916571 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html +++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html
@@ -123,7 +123,7 @@ } </style> <h3> - <a href$="[[_computeChangePath(_changeNum, _patchRange.patchNum, _change.revisions)]]"> + <a href$="[[_computeChangePath(_changeNum, _patchRange.*, _change.revisions)]]"> [[_changeNum]]</a><span>:</span> <span>[[_change.subject]]</span> <span class="dash">—</span> @@ -140,7 +140,7 @@ <iron-dropdown id="dropdown" vertical-align="top" vertical-offset="25"> <div class="dropdown-content"> <template is="dom-repeat" items="[[_fileList]]" as="path"> - <a href$="[[_computeDiffURL(_changeNum, _patchRange, path)]]" + <a href$="[[_computeDiffURL(_changeNum, _patchRange.*, path)]]" selected$="[[_computeFileSelected(path, _path)]]" data-key-nav$="[[_computeKeyNav(path, _path, _fileList)]]" on-tap="_handleFileTap">
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 6932d10..4b0ecdd 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
@@ -83,7 +83,7 @@ observers: [ '_getChangeDetail(_changeNum)', '_getProjectConfig(_change.project)', - '_getFiles(_changeNum, _patchRange.patchNum)', + '_getFiles(_changeNum, _patchRange.*)', '_updateModeSelect(_diffMode)', ], @@ -127,9 +127,10 @@ }.bind(this)); }, - _getFiles: function(changeNum, patchNum) { + _getFiles: function(changeNum, patchRangeRecord) { + var patchRange = patchRangeRecord.base; return this.$.restAPI.getChangeFilePathsAsSpeciallySortedArray( - changeNum, patchNum).then(function(files) { + changeNum, patchRange).then(function(files) { this._fileList = files; }.bind(this)); }, @@ -196,9 +197,9 @@ case 85: // 'u' if (this._changeNum && this._patchRange.patchNum) { e.preventDefault(); - page.show(this._computeChangePath( + page.show(this._getChangePath( this._changeNum, - this._patchRange.patchNum, + this._patchRange, this._change && this._change.revisions)); } break; @@ -221,15 +222,15 @@ var idx = fileList.indexOf(this._path) + direction; if (idx < 0 || idx > fileList.length - 1) { - page.show(this._computeChangePath( + page.show(this._getChangePath( this._changeNum, - this._patchRange.patchNum, + this._patchRange, this._change && this._change.revisions)); return; } - page.show(this._computeDiffURL(this._changeNum, - this._patchRange, - fileList[idx])); + page.show(this._getDiffURL(this._changeNum, + this._patchRange, + fileList[idx])); }, _paramsChanged: function(value) { @@ -282,13 +283,22 @@ } }, - _computeDiffURL: function(changeNum, patchRange, path) { + _getDiffURL: function(changeNum, patchRange, path) { + return '/c/' + changeNum + '/' + this._patchRangeStr(patchRange) + '/' + + path; + }, + + _computeDiffURL: function(changeNum, patchRangeRecord, path) { + return this._getDiffURL(changeNum, patchRangeRecord.base, path); + }, + + _patchRangeStr: function(patchRange) { var patchStr = patchRange.patchNum; if (patchRange.basePatchNum != null && patchRange.basePatchNum != 'PARENT') { patchStr = patchRange.basePatchNum + '..' + patchRange.patchNum; } - return '/c/' + changeNum + '/' + patchStr + '/' + path; + return patchStr; }, _computeAvailablePatches: function(revisions) { @@ -299,25 +309,30 @@ return patchNums.sort(function(a, b) { return a - b; }); }, - _computeChangePath: function(changeNum, patchNum, revisions) { + _getChangePath: function(changeNum, patchRange, revisions) { var base = '/c/' + changeNum + '/'; // The change may not have loaded yet, making revisions unavailable. if (!revisions) { - return base + patchNum; + return base + this._patchRangeStr(patchRange); } var latestPatchNum = -1; for (var rev in revisions) { latestPatchNum = Math.max(latestPatchNum, revisions[rev]._number); } - if (parseInt(patchNum, 10) != latestPatchNum) { - return base + patchNum; + if (patchRange.basePatchNum !== 'PARENT' || + parseInt(patchRange.patchNum, 10) !== latestPatchNum) { + return base + this._patchRangeStr(patchRange); } return base; }, + _computeChangePath: function(changeNum, patchRangeRecord, revisions) { + return this._getChangePath(changeNum, patchRangeRecord.base, revisions); + }, + _computeFileDisplayName: function(path) { return path == COMMIT_MESSAGE_PATH ? 'Commit message' : path; }, @@ -347,8 +362,7 @@ _handleMobileSelectChange: function(e) { var path = Polymer.dom(e).rootTarget.value; - page.show( - this._computeDiffURL(this._changeNum, this._patchRange, path)); + page.show(this._getDiffURL(this._changeNum, this._patchRange, path)); }, _showDropdownTapHandler: function(e) {
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 4167424..e0d0735 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
@@ -50,6 +50,7 @@ test('keyboard shortcuts', function() { element._changeNum = '42'; element._patchRange = { + basePatchNum: 'PARENT', patchNum: '10', }; element._change = { @@ -143,12 +144,12 @@ MockInteractions.pressAndReleaseKeyOn(element, 65); // 'a' assert.isTrue(element.changeViewState.showReplyDialog); - assert(showStub.lastCall.calledWithExactly('/c/42/'), - 'Should navigate to /c/42/'); + assert(showStub.lastCall.calledWithExactly('/c/42/5..10'), + 'Should navigate to /c/42/5..10'); MockInteractions.pressAndReleaseKeyOn(element, 85); // 'u' - assert(showStub.lastCall.calledWithExactly('/c/42/'), - 'Should navigate to /c/42/'); + assert(showStub.lastCall.calledWithExactly('/c/42/5..10'), + 'Should navigate to /c/42/5..10'); MockInteractions.pressAndReleaseKeyOn(element, 221); // ']' assert(showStub.lastCall.calledWithExactly('/c/42/5..10/wheatley.md'), @@ -166,8 +167,8 @@ element._path = 'chell.go'; MockInteractions.pressAndReleaseKeyOn(element, 219); // '[' - assert(showStub.lastCall.calledWithExactly('/c/42/'), - 'Should navigate to /c/42/'); + assert(showStub.lastCall.calledWithExactly('/c/42/5..10'), + 'Should navigate to /c/42/5..10'); showStub.restore(); }); @@ -175,6 +176,7 @@ test('keyboard shortcuts with old patch number', function() { element._changeNum = '42'; element._patchRange = { + basePatchNum: 'PARENT', patchNum: '1', }; element._change = { @@ -230,6 +232,7 @@ test('go up to change via kb without change loaded', function() { element._changeNum = '42'; element._patchRange = { + basePatchNum: 'PARENT', patchNum: '1', }; @@ -280,6 +283,7 @@ test('jump to file dropdown', function() { element._changeNum = '42'; element._patchRange = { + basePatchNum: 'PARENT', patchNum: '10', }; element._fileList = ['chell.go', 'glados.txt', 'wheatley.md'];
diff --git a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js index a2fa039..3cb37b6 100644 --- a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js +++ b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js
@@ -315,18 +315,22 @@ this.getChangeActionURL(changeNum, patchNum, '/commit?links')); }, - getChangeFiles: function(changeNum, patchNum) { + getChangeFiles: function(changeNum, patchRange) { + var endpoint = '/files'; + if (patchRange.basePatchNum !== 'PARENT') { + endpoint += '?base=' + encodeURIComponent(patchRange.basePatchNum); + } return this.fetchJSON( - this.getChangeActionURL(changeNum, patchNum, '/files')); + this.getChangeActionURL(changeNum, patchRange.patchNum, endpoint)); }, - getChangeFilesAsSpeciallySortedArray: function(changeNum, patchNum) { - return this.getChangeFiles(changeNum, patchNum).then( + getChangeFilesAsSpeciallySortedArray: function(changeNum, patchRange) { + return this.getChangeFiles(changeNum, patchRange).then( this._normalizeChangeFilesResponse.bind(this)); }, - getChangeFilePathsAsSpeciallySortedArray: function(changeNum, patchNum) { - return this.getChangeFiles(changeNum, patchNum).then(function(files) { + getChangeFilePathsAsSpeciallySortedArray: function(changeNum, patchRange) { + return this.getChangeFiles(changeNum, patchRange).then(function(files) { return Object.keys(files).sort(this._specialFilePathCompare.bind(this)); }.bind(this)); },