Add diff preferences to change view
Noteworthy decisions:
- Preferences are hidden when diff prefs are not loaded or the user is not
logged in.
- Preferences are hidden on small screens
- The trigger button is in gr-change-view but the gr-diff-preferences
is part of gr-file-list. This is because gr-file-list because diff
preferences and local preferences are more closely tied with that
than the change view. In order to put it in the change view, local
prefs would also have to be two-way bound back.
Also fixes computePrefsButtonHidden in gr-diff-view as well. The
function did not work as intended before. If preferences didn't exist,
the function would not get called, and the container would not be
hidden.
Bug: Issue 5426
Change-Id: I361cdf132c6e15b5ae2f15e62af318cfa05161ce
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 776ace2..2d57392 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
@@ -19,6 +19,7 @@
<link rel="import" href="../../../behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior.html">
<link rel="import" href="../../../behaviors/rest-client-behavior/rest-client-behavior.html">
<link rel="import" href="../../../bower_components/polymer/polymer.html">
+<link rel="import" href="../../diff/gr-diff-preferences/gr-diff-preferences.html">
<link rel="import" href="../../shared/gr-account-link/gr-account-link.html">
<link rel="import" href="../../shared/gr-select/gr-select.html">
<link rel="import" href="../../shared/gr-button/gr-button.html">
@@ -78,6 +79,9 @@
font-size: 1.2em;
font-weight: bold;
}
+ .prefsButton {
+ float: right;
+ }
gr-change-star {
margin-right: .25em;
vertical-align: -.425em;
@@ -193,6 +197,9 @@
height: 0;
margin-bottom: 1em;
}
+ #diffPrefsContainer {
+ margin: auto 0 auto auto;
+ }
.patchInfo-header-wrapper {
width: 100%;
}
@@ -461,9 +468,17 @@
read-only="[[_descriptionReadOnly]]"
on-changed="_handleDescriptionChanged"></gr-editable-label>
</span>
+ <span id="diffPrefsContainer"
+ hidden$="[[_computePrefsButtonHidden(_diffPrefs, _loggedIn)]]"
+ hidden>
+ <gr-button link
+ class="prefsButton desktop"
+ on-tap="_handlePrefsTap">Diff Preferences</gr-button>
+ </span>
</div>
</div>
<gr-file-list id="fileList"
+ diff-prefs="{{_diffPrefs}}"
change="[[_change]]"
change-num="[[_changeNum]]"
patch-range="{{_patchRange}}"
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 112b2a7..c9d6b57 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
@@ -73,6 +73,7 @@
type: Object,
value: function() { return document.body; },
},
+ _diffPrefs: Object,
_numFilesShown: {
type: Number,
observer: '_numFilesShownChanged',
@@ -190,6 +191,7 @@
'u': '_handleUKey',
'x': '_handleXKey',
'z': '_handleZKey',
+ ',': '_handleCommaKey',
},
attached: function() {
@@ -225,6 +227,10 @@
}
},
+ _computePrefsButtonHidden: function(prefs, loggedIn) {
+ return !loggedIn || !prefs;
+ },
+
_handleEditCommitMessage: function(e) {
this._editingCommitMessage = true;
this.$.commitMessageEditor.focusTextarea();
@@ -273,6 +279,11 @@
return false;
},
+ _handlePrefsTap: function(e) {
+ e.preventDefault();
+ this.$.fileList.openDiffPrefs();
+ },
+
_handleCommentSave: function(e) {
if (!e.target.comment.__draft) { return; }
@@ -800,6 +811,14 @@
this.$.messageList.handleExpandCollapse(false);
},
+ _handleCommaKey: function(e) {
+ if (this.shouldSuppressKeyboardShortcut(e) ||
+ this.modifierPressed(e)) { return; }
+
+ e.preventDefault();
+ this.$.fileList.openDiffPrefs();
+ },
+
_determinePageBack: function() {
// Default backPage to '/' if user came to change view page
// via an email link, etc.
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.html b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.html
index b7c848c..2b707cf 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.html
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.html
@@ -157,6 +157,45 @@
MockInteractions.pressAndReleaseKeyOn(element, 68, null, 'd');
assert.isTrue(stub.called);
});
+
+ test(', should open diff preferences', function() {
+ var stub = sandbox.stub(element.$.fileList.$.diffPreferences, 'open');
+ MockInteractions.pressAndReleaseKeyOn(element, 188, null, ',');
+ assert.isTrue(stub.called);
+ });
+ });
+
+ test('Diff preferences hidden when no prefs or logged out',
+ function() {
+ element._loggedIn = false;
+ flushAsynchronousOperations();
+ assert.isTrue(element.$.diffPrefsContainer.hidden);
+
+ element._loggedIn = true;
+ flushAsynchronousOperations();
+ assert.isTrue(element.$.diffPrefsContainer.hidden);
+
+ element._loggedIn = false;
+ element._diffPrefs = {'font_size': '12'};
+ flushAsynchronousOperations();
+ assert.isTrue(element.$.diffPrefsContainer.hidden);
+
+ element._loggedIn = true;
+ flushAsynchronousOperations();
+ assert.isFalse(element.$.diffPrefsContainer.hidden);
+ });
+
+ test('prefsButton opens gr-diff-preferences', function() {
+ var handlePrefsTapSpy = sandbox.spy(element, '_handlePrefsTap');
+ var overlayOpenStub = sandbox.stub(element.$.fileList,
+ 'openDiffPrefs');
+ var prefsButton = Polymer.dom(element.root).querySelectorAll(
+ '.prefsButton')[0];
+
+ MockInteractions.tap(prefsButton);
+
+ assert.isTrue(handlePrefsTapSpy.called);
+ assert.isTrue(overlayOpenStub.called);
});
test('_computeDescriptionReadOnly', function() {
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 6f8568c..f3e1092 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
@@ -328,9 +328,10 @@
change-num="[[changeNum]]"
patch-range="[[patchRange]]"
path="[[file.__path]]"
- prefs="[[_diffPrefs]]"
+ prefs="[[diffPrefs]]"
project-config="[[projectConfig]]"
on-line-selected="_onLineSelected"
+ no-render-on-prefs-change
view-mode="[[_getDiffViewMode(diffViewMode, _userPrefs)]]"></gr-diff>
</template>
</template>
@@ -384,6 +385,10 @@
[[_computeShowAllText(_files)]]
</gr-button><!--
--></gr-tooltip-content>
+ <gr-diff-preferences
+ id="diffPreferences"
+ prefs="{{diffPrefs}}"
+ local-prefs="{{_localPrefs}}"></gr-diff-preferences>
<gr-rest-api-interface id="restAPI"></gr-rest-api-interface>
<gr-storage id="storage"></gr-storage>
<gr-diff-cursor id="diffCursor"></gr-diff-cursor>
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 705a8f1..30ccb76 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
@@ -54,6 +54,7 @@
diffViewMode: {
type: String,
notify: true,
+ observer: '_updateDiffPreferences',
},
_files: {
type: Array,
@@ -69,7 +70,11 @@
value: function() { return []; },
},
_diffAgainst: String,
- _diffPrefs: Object,
+ diffPrefs: {
+ type: Object,
+ notify: true,
+ observer: '_updateDiffPreferences',
+ },
_userPrefs: Object,
_localPrefs: Object,
_showInlineDiffs: Boolean,
@@ -158,7 +163,7 @@
this._localPrefs = this.$.storage.getPreferences();
promises.push(this._getDiffPreferences().then(function(prefs) {
- this._diffPrefs = prefs;
+ this.diffPrefs = prefs;
}.bind(this)));
promises.push(this._getPreferences().then(function(prefs) {
@@ -173,6 +178,10 @@
return Polymer.dom(this.root).querySelectorAll('gr-diff');
},
+ openDiffPrefs: function() {
+ this.$.diffPreferences.open();
+ },
+
_calculatePatchChange: function(files) {
var filesNoCommitMsg = files.filter(function(files) {
return files.__path !== '/COMMIT_MSG';
@@ -232,6 +241,19 @@
this._patchRangeStr(patchRange), true));
},
+ _updateDiffPreferences: function() {
+ if (!this.diffs.length) { return; }
+ // Re-render all expanded diffs sequentially.
+ var timerName = 'Update ' + this._expandedFilePaths.length +
+ ' diffs with new prefs';
+ this._renderInOrder(this._expandedFilePaths, this.diffs,
+ this._expandedFilePaths.length)
+ .then(function() {
+ this.$.reporting.timeEnd(timerName);
+ this.$.diffCursor.handleDiffUpdate();
+ }.bind(this));
+ },
+
_forEachDiff: function(fn) {
var diffs = this.diffs;
for (var i = 0; i < diffs.length; i++) {
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 a34463b..d07f364 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
@@ -699,6 +699,7 @@
basePatchNum: 'PARENT',
patchNum: '2',
};
+ sandbox.spy(element, '_updateDiffPreferences');
element.$.fileCursor.setCursorAtIndex(0);
flushAsynchronousOperations();
@@ -714,6 +715,7 @@
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', function() {
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js
index 214454a..c407829 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js
@@ -405,7 +405,6 @@
td.classList.add(line.type);
var html = this._escapeHTML(text);
html = this._addTabWrappers(html, this._prefs.tab_size);
-
if (!this._prefs.line_wrapping &&
this._textLength(text, this._prefs.tab_size) >
this._prefs.line_length) {
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 a3ff3d1..93d939d 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
@@ -264,7 +264,8 @@
<option value="SIDE_BY_SIDE">Side By Side</option>
<option value="UNIFIED_DIFF">Unified</option>
</select>
- <span hidden$="[[_computePrefsButtonHidden(_prefs, _loggedIn)]]">
+ <span id="diffPrefsContainer"
+ hidden$="[[_computePrefsButtonHidden(_prefs, _loggedIn)]]" hidden>
<span class="preferences desktop">
<span
hidden$="[[_computeModeSelectHidden(_isImageDiff)]]">/</span>
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 166a3d3..663cd3b 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
@@ -252,6 +252,38 @@
'Should navigate to /c/42/1');
});
+ test('Diff preferences hidden when no prefs or logged out',
+ function() {
+ element._loggedIn = false;
+ flushAsynchronousOperations();
+ assert.isTrue(element.$.diffPrefsContainer.hidden);
+
+ element._loggedIn = true;
+ flushAsynchronousOperations();
+ assert.isTrue(element.$.diffPrefsContainer.hidden);
+
+ element._loggedIn = false;
+ element._prefs = {'font_size': '12'};
+ flushAsynchronousOperations();
+ assert.isTrue(element.$.diffPrefsContainer.hidden);
+
+ element._loggedIn = true;
+ flushAsynchronousOperations();
+ assert.isFalse(element.$.diffPrefsContainer.hidden);
+ });
+
+ test('prefsButton opens gr-diff-preferences', function() {
+ var handlePrefsTapSpy = sandbox.spy(element, '_handlePrefsTap');
+ var overlayOpenStub = sandbox.stub(element.$.diffPreferences,
+ 'open');
+ var prefsButton = Polymer.dom(element.root).querySelector('.prefsButton');
+
+ MockInteractions.tap(prefsButton);
+
+ assert.isTrue(handlePrefsTapSpy.called);
+ assert.isTrue(overlayOpenStub.called);
+ });
+
test('go up to change via kb without change loaded', function() {
element._changeNum = '42';
element._patchRange = {
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
index cd1931a..1d766b86 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
@@ -74,6 +74,7 @@
type: Boolean,
reflectToAttribute: true,
},
+ noRenderOnPrefsChange: Boolean,
_loggedIn: {
type: Boolean,
value: false,
@@ -438,7 +439,7 @@
this.updateStyles();
- if (this._diff && this._comments) {
+ if (this._diff && this._comments && !this.noRenderOnPrefsChange) {
this._renderDiffTable();
}
},
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html
index 5868f77..2a150ff 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html
@@ -840,6 +840,59 @@
});
});
+ suite('change in preferences', function() {
+ setup(function() {
+ element._diff = {
+ meta_a: {name: 'carrot.jpg', content_type: 'image/jpeg', lines: 66},
+ meta_b: {name: 'carrot.jpg', content_type: 'image/jpeg',
+ lines: 560},
+ diff_header: [],
+ intraline_status: 'OK',
+ change_type: 'MODIFIED',
+ content: [{skip: 66}],
+ };
+ element._comments = {
+ meta: {
+ changeNum: '42',
+ patchRange: {
+ basePatchNum: 'PARENT',
+ patchNum: 3,
+ },
+ path: '/path/to/foo',
+ projectConfig: {foo: 'bar'},
+ },
+ left: [
+ {id: 'bc1', side: 'PARENT', __commentSide: 'left'},
+ {id: 'bc2', side: 'PARENT', __commentSide: 'left'},
+ {id: 'bd1', __draft: true, side: 'PARENT', __commentSide: 'left'},
+ {id: 'bd2', __draft: true, side: 'PARENT', __commentSide: 'left'},
+ ],
+ right: [
+ {id: 'c1', __commentSide: 'right'},
+ {id: 'c2', __commentSide: 'right'},
+ {id: 'd1', __draft: true, __commentSide: 'right'},
+ {id: 'd2', __draft: true, __commentSide: 'right'},
+ ],
+ };
+ });
+
+ test('change in preferences re-renders diff', function() {
+ sandbox.stub(element, '_renderDiffTable');
+ element.prefs = {};
+ element.prefs = {time_format: 'HHMM_12'};
+ assert.isTrue(element._renderDiffTable.called);
+ });
+
+ test('change in preferences does not re-renders diff with ' +
+ 'noRenderOnPrefsChange', function() {
+ sandbox.stub(element, '_renderDiffTable');
+ element.noRenderOnPrefsChange = true;
+ element.prefs = {};
+ element.prefs = {time_format: 'HHMM_12'};
+ assert.isFalse(element._renderDiffTable.called);
+ });
+ });
+
suite('handle comment-update', function() {
setup(function() {
