Merge "Refactor file list header into change view"

This commit is contained in:
Becky Siegel
2017-09-08 22:25:40 +00:00
committed by Gerrit Code Review
6 changed files with 291 additions and 255 deletions

View File

@@ -173,7 +173,7 @@ limitations under the License.
display: initial;
}
.patchInfo-header,
gr-file-list {
.fileList {
padding: .5em calc(var(--default-horizontal-margin) / 2);
}
.patchInfo-header {
@@ -252,6 +252,27 @@ limitations under the License.
.editLoaded .showOnEdit {
display: initial;
}
.fileList-header {
display: flex;
font-weight: bold;
justify-content: space-between;
margin-bottom: .5em;
}
.rightControls {
display: flex;
flex-wrap: wrap;
font-weight: normal;
justify-content: flex-end;
}
.separator {
margin: 0 .25em;
}
.expandInline {
padding-right: .25em;
}
.patchSetSelect {
max-width: 8em;
}
@media screen and (min-width: 80em) {
.commitMessage {
max-width: var(--commit-message-max-width, 100ch);
@@ -527,20 +548,76 @@ limitations under the License.
</span>
</div>
</div>
<gr-file-list id="fileList"
diff-prefs="{{_diffPrefs}}"
change="[[_change]]"
change-num="[[_changeNum]]"
patch-range="{{_patchRange}}"
comments="[[_comments]]"
drafts="[[_diffDrafts]]"
revisions="[[_sortedRevisions]]"
project-config="[[_projectConfig]]"
selected-index="{{viewState.selectedFileIndex}}"
diff-view-mode="{{viewState.diffMode}}"
edit-loaded="[[_editLoaded]]"
num-files-shown="{{_numFilesShown}}"
file-list-increment="{{_numFilesShown}}"></gr-file-list>
<div class="fileList">
<div class="fileList-header">
<div>Files</div>
<div class="rightControls">
<template is="dom-if"
if="[[_fileListActionsVisible(_shownFileCount, _maxFilesForBulkActions)]]">
<gr-button
id="expandBtn"
link
on-tap="_expandAllDiffs">Show diffs</gr-button>
<span class="separator">/</span>
<gr-button
id="collapseBtn"
link
on-tap="_collapseAllDiffs">Hide diffs</gr-button>
</template>
<template is="dom-if"
if="[[!_fileListActionsVisible(_shownFileCount, _maxFilesForBulkActions)]]">
<div class="warning">
Bulk actions disabled because there are too many files.
</div>
</template>
<span class="separator">/</span>
<gr-select
id="modeSelect"
bind-value="{{viewState.diffMode}}">
<select>
<option value="SIDE_BY_SIDE">Side By Side</option>
<option value="UNIFIED_DIFF">Unified</option>
</select>
</gr-select>
<span class="separator">/</span>
<label>
Diff against
<gr-select id="patchChange" bind-value="{{_diffAgainst}}"
class="patchSetSelect" on-change="_handleBasePatchChange">
<select>
<option value="PARENT">Base</option>
<template
is="dom-repeat"
items="[[_allPatchSets]]"
as="patchNum">
<option
disabled$="[[_computeBasePatchDisabled(patchNum.num, _patchRange.patchNum, _sortedRevisions)]]"
value$="[[patchNum.num]]">
[[patchNum.num]]
[[patchNum.desc]]
</option>
</template>
</select>
</gr-select>
</label>
</div>
</div>
<gr-file-list id="fileList"
diff-prefs="{{_diffPrefs}}"
change="[[_change]]"
change-num="[[_changeNum]]"
patch-range="{{_patchRange}}"
comments="[[_comments]]"
drafts="[[_diffDrafts]]"
revisions="[[_sortedRevisions]]"
project-config="[[_projectConfig]]"
selected-index="{{viewState.selectedFileIndex}}"
diff-view-mode="[[viewState.diffMode]]"
edit-loaded="[[_editLoaded]]"
num-files-shown="{{_numFilesShown}}"
file-list-increment="{{_numFilesShown}}"
on-files-shown-changed="_setShownFiles"></gr-file-list>
</div>
</section>
<gr-messages-list id="messageList"
class="hideOnMobileOverlay"

View File

@@ -123,6 +123,7 @@
computed: '_computeHideEditCommitMessage(_loggedIn, ' +
'_editingCommitMessage, _change)',
},
_diffAgainst: String,
/** @type {?string} */
_latestCommitMessage: {
type: String,
@@ -133,6 +134,13 @@
type: String,
computed:
'_computeChangeIdCommitMessageError(_latestCommitMessage, _change)',
},
// Caps the number of files that can be shown and have the 'show diffs' /
// 'hide diffs' buttons still be functional.
_maxFilesForBulkActions: {
type: Number,
readOnly: true,
value: 225,
},
/** @type {?} */
_patchRange: {
@@ -162,6 +170,7 @@
computed: '_computeReplyButtonLabel(_diffDrafts.*, _canStartReview)',
},
_selectedPatchSet: String,
_shownFileCount: Number,
_initialLoadComplete: {
type: Boolean,
value: false,
@@ -241,6 +250,7 @@
this._account = acct;
});
}
this._setDiffViewMode();
});
this.addEventListener('comment-save', this._handleCommentSave.bind(this));
@@ -264,6 +274,20 @@
}
},
_setDiffViewMode() {
if (!this.viewState.diffViewMode) {
return this.$.restAPI.getPreferences().then( prefs => {
if (!this.viewState.diffMode) {
this.set('viewState.diffMode', prefs.default_diff_view);
}
}).then(() => {
if (!this.viewState.diffMode) {
this.set('viewState.diffMode', 'SIDE_BY_SIDE');
}
});
}
},
_updateSortedRevisions(revisionsRecord) {
const revisions = revisionsRecord.base;
this._sortedRevisions = this.sortRevisions(Object.values(revisions));
@@ -385,8 +409,12 @@
this._diffDrafts = diffDrafts;
},
_handleBasePatchChange(e) {
this._changePatchNum(this._selectedPatchSet, e.target.value, true);
},
_handlePatchChange(e) {
this._changePatchNum(e.target.value, true);
this._changePatchNum(e.target.value, this._diffAgainst, true);
},
_handleReplyTap(e) {
@@ -462,6 +490,22 @@
}, 150);
},
_setShownFiles(e) {
this._shownFileCount = e.detail.length;
},
_fileListActionsVisible(shownFileCount, maxFilesForBulkActions) {
return shownFileCount <= maxFilesForBulkActions;
},
_expandAllDiffs() {
this.$.fileList.expandAllDiffs();
},
_collapseAllDiffs() {
this.$.fileList.collapseAllDiffs();
},
_paramsChanged(value) {
if (value.view !== Gerrit.Nav.View.CHANGE) {
this._initialLoadComplete = false;
@@ -630,12 +674,13 @@
/**
* Change active patch to the provided patch num.
* @param {number|string} patchNum the patchn number to be viewed.
* @param {number|string} basePatchNum the base patch to be viewed.
* @param {number|string} patchNum the patch number to be viewed.
* @param {boolean} opt_forceParams When set to true, the resulting URL will
* always include the patch range, even if the requested patchNum is
* known to be the latest.
*/
_changePatchNum(patchNum, opt_forceParams) {
_changePatchNum(patchNum, basePatchNum, opt_forceParams) {
if (!opt_forceParams) {
let currentPatchNum;
if (this._change.current_revision) {
@@ -645,13 +690,13 @@
currentPatchNum = this.computeLatestPatchNum(this._allPatchSets);
}
if (this.patchNumEquals(patchNum, currentPatchNum) &&
this._patchRange.basePatchNum === 'PARENT') {
basePatchNum === 'PARENT') {
Gerrit.Nav.navigateToChange(this._change);
return;
}
}
Gerrit.Nav.navigateToChange(this._change, patchNum,
this._patchRange.basePatchNum);
basePatchNum);
},
_computeChangeUrl(change) {
@@ -734,6 +779,11 @@
this.findSortedIndex(basePatchNum, this._sortedRevisions);
},
_computeBasePatchDisabled(patchNum, currentPatchNum) {
return this.findSortedIndex(patchNum, this._sortedRevisions) >=
this.findSortedIndex(currentPatchNum, this._sortedRevisions);
},
_computeLabelNames(labels) {
return Object.keys(labels).sort();
},
@@ -1115,6 +1165,7 @@
_updateSelected() {
this._selectedPatchSet = this._patchRange.patchNum;
this._diffAgainst = this._patchRange.basePatchNum;
},
_computePatchSetDescription(change, patchNum) {

View File

@@ -33,6 +33,12 @@ limitations under the License.
</template>
</test-fixture>
<test-fixture id="blank">
<template>
<div></div>
</template>
</test-fixture>
<script>
suite('gr-change-view tests', () => {
let element;
@@ -46,6 +52,7 @@ limitations under the License.
stub('gr-rest-api-interface', {
getConfig() { return Promise.resolve({test: 'config'}); },
getAccount() { return Promise.resolve(null); },
_fetchSharedCacheURL() { return Promise.resolve({}); },
});
element = fixture('basic');
});
@@ -665,6 +672,132 @@ limitations under the License.
element.fire('change', {}, {node: selectEl.nativeSelect});
});
test('diffMode defaults to side by side without preferences', done => {
sandbox.stub(element.$.restAPI, 'getPreferences').returns(
Promise.resolve({}));
// No user prefs or diff view mode set.
element._setDiffViewMode().then(() => {
assert.equal(element.viewState.diffMode, 'SIDE_BY_SIDE');
done();
});
});
test('diffMode defaults to preference when not already set', done => {
sandbox.stub(element.$.restAPI, 'getPreferences').returns(
Promise.resolve({default_diff_view: 'UNIFIED'}));
element._setDiffViewMode().then(() => {
assert.equal(element.viewState.diffMode, 'UNIFIED');
done();
});
});
test('existing diffMode overrides preference', done => {
element.viewState.diffMode = 'SIDE_BY_SIDE';
sandbox.stub(element.$.restAPI, 'getPreferences').returns(
Promise.resolve({default_diff_view: 'UNIFIED'}));
element._setDiffViewMode().then(() => {
assert.equal(element.viewState.diffMode, 'SIDE_BY_SIDE');
done();
});
});
test('diff against dropdown', done => {
element._changeNum = '42';
element._patchRange = {
basePatchNum: 'PARENT',
patchNum: '3',
};
element._change = {
change_id: 'Iad9dc96274af6946f3632be53b106ef80f7ba6ca',
revisions: {
rev1: {_number: 1},
rev2: {_number: 2},
rev3: {_number: 'edit', basePatchNum: 2},
rev4: {_number: 3},
},
status: 'NEW',
labels: {},
};
flush(() => {
const selectEl = element.$.patchChange;
assert.equal(selectEl.nativeSelect.value, 'PARENT');
assert.isTrue(element.$$('#patchChange option[value="3"]')
.hasAttribute('disabled'));
selectEl.addEventListener('change', () => {
assert.equal(selectEl.nativeSelect.value, 'edit');
assert(navigateToChangeStub.lastCall.calledWithExactly(
element._change, '3', 'edit'),
'Should navigate to /c/42/edit..3');
done();
});
selectEl.nativeSelect.value = 'edit';
element.fire('change', {}, {node: selectEl.nativeSelect});
});
});
test('expandAllDiffs called when expand button clicked', () => {
element._shownFileCount = 1;
flushAsynchronousOperations();
sandbox.stub(element.$.fileList, 'expandAllDiffs');
MockInteractions.tap(Polymer.dom(element.root).querySelector(
'#expandBtn'));
assert.isTrue(element.$.fileList.expandAllDiffs.called);
});
test('collapseAllDiffs called when expand button clicked', () => {
element._shownFileCount = 1;
flushAsynchronousOperations();
sandbox.stub(element.$.fileList, 'collapseAllDiffs');
MockInteractions.tap(Polymer.dom(element.root).querySelector(
'#collapseBtn'));
assert.isTrue(element.$.fileList.collapseAllDiffs.called);
});
test('show/hide diffs disabled for large amounts of files', done => {
const computeSpy = sandbox.spy(element, '_fileListActionsVisible');
element._files = [];
element.changeNum = '42';
element.patchRange = {
basePatchNum: 'PARENT',
patchNum: '2',
};
element._shownFileCount = 1;
flush(() => {
assert.isTrue(computeSpy.lastCall.returnValue);
_.times(element._maxFilesForBulkActions + 1, () => {
element._shownFileCount = element._shownFileCount + 1;
});
assert.isFalse(computeSpy.lastCall.returnValue);
done();
});
});
test('diff mode selector initializes from preferences', () => {
let resolvePrefs;
const prefsPromise = new Promise(resolve => {
resolvePrefs = resolve;
});
sandbox.stub(element.$.restAPI, 'getPreferences').returns(prefsPromise);
// Attach a new gr-change-view so we can intercept the preferences fetch.
const view = document.createElement('gr-change-view');
const select = view.$.modeSelect;
fixture('blank').appendChild(view);
flushAsynchronousOperations();
// At this point the diff mode doesn't yet have the user's preference.
assert.equal(select.nativeSelect.value, 'SIDE_BY_SIDE');
// Receive the overriding preference.
resolvePrefs({default_diff_view: 'UNIFIED'});
flushAsynchronousOperations();
assert.equal(select.nativeSelect.value, 'SIDE_BY_SIDE');
document.getElementById('blank').restore();
});
test('dont reload entire page when patchRange changes', () => {
const reloadStub = sandbox.stub(element, '_reload',
() => { return Promise.resolve(); });
@@ -726,13 +859,13 @@ limitations under the License.
labels: {},
};
element._changePatchNum(13);
element._changePatchNum(13, 2);
assert.isTrue(navigateToChangeStub.lastCall.calledWithExactly(
element._change, 13, '2'));
element._change, 13, 2));
element._patchRange.basePatchNum = 'PARENT';
element._changePatchNum(3);
element._changePatchNum(3, 'PARENT');
assert.isTrue(navigateToChangeStub.lastCall.calledWithExactly(
element._change, 3, 'PARENT'));
});

View File

@@ -44,21 +44,6 @@ limitations under the License.
:host(.loading) .row {
opacity: .5;
};
header {
display: flex;
font-weight: bold;
justify-content: space-between;
margin-bottom: .5em;
}
.rightControls {
display: flex;
flex-wrap: wrap;
font-weight: normal;
justify-content: flex-end;
}
.separator {
margin: 0 .25em;
}
.reviewed,
.status {
align-items: center;
@@ -132,9 +117,6 @@ limitations under the License.
padding-right: 2.6em;
text-align: right;
}
.expandInline {
padding-right: .25em;
}
.warning {
color: #666;
}
@@ -155,9 +137,6 @@ limitations under the License.
margin: .25em 0 1em;
overflow-x: auto;
}
.patchSetSelect {
max-width: 8em;
}
.truncatedFileName {
display: none;
}
@@ -201,53 +180,6 @@ limitations under the License.
}
}
</style>
<header>
<div>Files</div>
<div class="rightControls">
<template is="dom-if"
if="[[_fileListActionsVisible(_shownFiles.*, _maxFilesForBulkActions)]]">
<gr-button link on-tap="_expandAllDiffs">Show diffs</gr-button>
<span class="separator">/</span>
<gr-button link on-tap="collapseAllDiffs">Hide diffs</gr-button>
</template>
<template is="dom-if"
if="[[!_fileListActionsVisible(_shownFiles.*, _maxFilesForBulkActions)]]">
<div class="warning">
Bulk actions disabled because there are too many files.
</div>
</template>
<span class="separator">/</span>
<gr-select
id="modeSelect"
bind-value="{{diffViewMode}}">
<select>
<option value="SIDE_BY_SIDE">Side By Side</option>
<option value="UNIFIED_DIFF">Unified</option>
</select>
</gr-select>
<span class="separator">/</span>
<label>
Diff against
<gr-select id="patchChange" bind-value="{{_diffAgainst}}"
class="patchSetSelect" on-change="_handlePatchChange">
<select>
<option value="PARENT">Base</option>
<template
is="dom-repeat"
items="[[computeAllPatchSets(change)]]"
as="patchNum">
<option
disabled$="[[_computePatchSetDisabled(patchNum.num, patchRange.patchNum, revisions)]]"
value$="[[patchNum.num]]">
[[patchNum.num]]
[[patchNum.desc]]
</option>
</template>
</select>
</gr-select>
</label>
</div>
</header>
<div
id="container"
class$="[[_computeContainerClass(editLoaded)]]"
@@ -351,7 +283,7 @@ limitations under the License.
project-config="[[projectConfig]]"
on-line-selected="_onLineSelected"
no-render-on-prefs-change
view-mode="[[_getDiffViewMode(diffViewMode, _userPrefs)]]"></gr-diff>
view-mode="[[diffViewMode]]"></gr-diff>
</template>
</template>
</div>

View File

@@ -36,10 +36,7 @@
properties: {
/** @type {?} */
patchRange: {
type: Object,
observer: '_updateSelected',
},
patchRange: Object,
patchNum: String,
changeNum: String,
comments: Object,
@@ -76,7 +73,6 @@
type: Array,
value() { return []; },
},
_diffAgainst: String,
diffPrefs: {
type: Object,
notify: true,
@@ -108,13 +104,6 @@
type: Array,
computed: '_computeFilesShown(numFilesShown, _files.*)',
},
// Caps the number of files that can be shown and have the 'show diffs' /
// 'hide diffs' buttons still be functional.
_maxFilesForBulkActions: {
type: Number,
readOnly: true,
value: 225,
},
_expandedFilePaths: {
type: Array,
value() { return []; },
@@ -188,9 +177,6 @@
promises.push(this._getPreferences().then(prefs => {
this._userPrefs = prefs;
if (!this.diffViewMode) {
this.set('diffViewMode', prefs.default_diff_view);
}
}));
return Promise.all(promises).then(() => {
@@ -239,11 +225,6 @@
return this.$.restAPI.getPreferences();
},
_computePatchSetDisabled(patchNum, currentPatchNum) {
return this.findSortedIndex(patchNum, this.revisions) >=
this.findSortedIndex(currentPatchNum, this.revisions);
},
_togglePathExpanded(path) {
// Is the path in the list of expanded diffs? IF so remove it, otherwise
// add it to the list.
@@ -259,14 +240,6 @@
this._togglePathExpanded(this._files[index].__path);
},
_handlePatchChange(e) {
const patchRange = Object.assign({}, this.patchRange);
patchRange.basePatchNum = Polymer.dom(e).rootTarget.value;
Gerrit.Nav.navigateToChange(this.change, patchRange.patchNum,
patchRange.basePatchNum);
},
_updateDiffPreferences() {
if (!this.diffs.length) { return; }
// Re-render all expanded diffs sequentially.
@@ -287,7 +260,7 @@
}
},
_expandAllDiffs() {
expandAllDiffs() {
this._showInlineDiffs = true;
// Find the list of paths that are in the file list, but not in the
@@ -642,7 +615,7 @@
if (this._showInlineDiffs) {
this.collapseAllDiffs();
} else {
this._expandAllDiffs();
this.expandAllDiffs();
}
},
@@ -751,7 +724,9 @@
},
_computeFilesShown(numFilesShown, files) {
return files.base.slice(0, numFilesShown);
const filesShown = files.base.slice(0, numFilesShown);
this.fire('files-shown-changed', {length: filesShown.length});
return filesShown;
},
_setReviewedFiles(shownFiles, files, reviewedRecord, loggedIn) {
@@ -816,35 +791,6 @@
this.numFilesShown = this._files.length;
},
_updateSelected(patchRange) {
this._diffAgainst = patchRange.basePatchNum;
},
/**
* _getDiffViewMode: Get the diff view (side-by-side or unified) based on
* the current state.
*
* The expected behavior is to use the mode specified in the user's
* preferences unless they have manually chosen the alternative view.
*
* Use side-by-side if there is no view mode or preferences.
*
* @return {string}
*/
_getDiffViewMode(diffViewMode, userPrefs) {
if (diffViewMode) {
return diffViewMode;
} else if (userPrefs) {
return this.diffViewMode = userPrefs.default_diff_view;
}
return 'SIDE_BY_SIDE';
},
_fileListActionsVisible(shownFilesRecord,
maxFilesForBulkActions) {
return shownFilesRecord.base.length <= maxFilesForBulkActions;
},
_computePatchSetDescription(revisions, patchNum) {
const rev = this.getRevisionByPatchNum(revisions, patchNum);
return (rev && rev.description) ?

View File

@@ -35,12 +35,6 @@ limitations under the License.
</template>
</test-fixture>
<test-fixture id="blank">
<template>
<div></div>
</template>
</test-fixture>
<script>
suite('gr-file-list tests', () => {
let element;
@@ -653,44 +647,6 @@ limitations under the License.
}
});
test('diff against dropdown', done => {
const navStub = sandbox.stub(Gerrit.Nav, 'navigateToChange');
element.changeNum = '42';
element.patchRange = {
basePatchNum: 'PARENT',
patchNum: '3',
};
element.change = {
revisions: {
rev1: {_number: 1},
rev2: {_number: 2},
rev3: {_number: 'edit', basePatchNum: 2},
rev4: {_number: 3},
},
};
element.revisions = [
{_number: 1},
{_number: 2},
{_number: 'edit', basePatchNum: 2},
{_number: 3},
];
flush(() => {
const selectEl = element.$.patchChange;
assert.equal(selectEl.nativeSelect.value, 'PARENT');
assert.isTrue(element.$$('option[value="3"]').hasAttribute('disabled'));
selectEl.addEventListener('change', () => {
assert.equal(selectEl.nativeSelect.value, 'edit');
assert(navStub.lastCall.calledWithExactly(element.change, '3', 'edit'),
'Should navigate to /c/42/edit..3');
navStub.restore();
done();
});
selectEl.nativeSelect.value = 'edit';
element.fire('change', {}, {node: selectEl.nativeSelect});
});
});
test('checkbox shows/hides diff inline', () => {
element._files = [
{__path: 'myfile.txt'},
@@ -737,57 +693,11 @@ limitations under the License.
flushAsynchronousOperations();
const diffDisplay = element.diffs[0];
element._userPrefs = {default_diff_view: 'SIDE_BY_SIDE'};
assert.equal(element.diffViewMode, 'SIDE_BY_SIDE');
assert.equal(diffDisplay.viewMode, 'SIDE_BY_SIDE');
element.set('diffViewMode', 'UNIFIED_DIFF');
assert.equal(diffDisplay.viewMode, 'UNIFIED_DIFF');
assert.isTrue(element._updateDiffPreferences.called);
});
test('diff mode selector initializes from preferences', () => {
let resolvePrefs;
const prefsPromise = new Promise(resolve => {
resolvePrefs = resolve;
});
sandbox.stub(element, '_getPreferences').returns(prefsPromise);
// Attach a new gr-file-list so we can intercept the preferences fetch.
const view = document.createElement('gr-file-list');
const select = view.$.modeSelect;
fixture('blank').appendChild(view);
flushAsynchronousOperations();
// At this point the diff mode doesn't yet have the user's preference.
assert.equal(select.nativeSelect.value, 'SIDE_BY_SIDE');
// Receive the overriding preference.
resolvePrefs({default_diff_view: 'UNIFIED'});
flushAsynchronousOperations();
assert.equal(select.nativeSelect.value, 'SIDE_BY_SIDE');
document.getElementById('blank').restore();
});
test('show/hide diffs disabled for large amounts of files', done => {
const computeSpy = sandbox.spy(element, '_fileListActionsVisible');
element._files = [];
element.changeNum = '42';
element.patchRange = {
basePatchNum: 'PARENT',
patchNum: '2',
};
element.$.fileCursor.setCursorAtIndex(0);
flush(() => {
assert.isTrue(computeSpy.lastCall.returnValue);
const arr = [];
_.times(element._maxFilesForBulkActions + 1, () => {
arr.push({__path: 'myfile.txt'});
});
element._files = arr;
element.numFilesShown = arr.length;
assert.isFalse(computeSpy.lastCall.returnValue);
done();
});
});
test('expanded attribute not set on path when not expanded', () => {
element._files = [
@@ -796,19 +706,6 @@ limitations under the License.
assert.isNotOk(element.$$('.expanded'));
});
test('_getDiffViewMode', () => {
// No user prefs or diff view mode set.
assert.equal(element._getDiffViewMode(), 'SIDE_BY_SIDE');
// User prefs but no diff view mode set.
element.diffViewMode = null;
element._userPrefs = {default_diff_view: 'UNIFIED_DIFF'};
assert.equal(
element._getDiffViewMode(null, element._userPrefs), 'UNIFIED_DIFF');
// User prefs and diff view mode set.
element.diffViewMode = 'SIDE_BY_SIDE';
assert.equal(element._getDiffViewMode(
element.diffViewMode, element._userPrefs), 'SIDE_BY_SIDE');
});
test('expand_inline_diffs user preference', () => {
element._files = [
{__path: '/COMMIT_MSG'},