Add preference for line wrapping in diff preferences
Previously in Polygerrit, diff views were always displayed in the width
specified in diff preferences. This change gives the option to wrap
lines instead, which takes precedence over column width (the column
width option is hidden when line wrapping is selected), and fits the
diff view to screen.
The gerrit API already supports the 'lineWrapping' preference so this
change uses that already existing option.
Feature: Issue 4809
Change-Id: I0d9e292739b5910abfd04af63ec4c745bf06e446
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 d8d5724..2f211bb 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
@@ -546,6 +546,7 @@
assert.isTrue(scrollStub.called);
document.body.style.height =
originalHeight + 'px';
+ scrollStub.restore();
done();
});
document.body.style.height = '10000px';
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-side-by-side.js b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-side-by-side.js
index 126882c..23c5036 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-side-by-side.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-side-by-side.js
@@ -34,6 +34,29 @@
return sectionEl;
};
+ GrDiffBuilderSideBySide.prototype.addColumns = function(outputEl, fontSize) {
+ var width = fontSize * 4;
+ var colgroup = document.createElement('colgroup');
+
+ // Add left-side line number.
+ var col = document.createElement('col');
+ col.setAttribute('width', width);
+ colgroup.appendChild(col);
+
+ // Add left-side content.
+ colgroup.appendChild(document.createElement('col'));
+
+ // Add right-side line number.
+ col = document.createElement('col');
+ col.setAttribute('width', width);
+ colgroup.appendChild(col);
+
+ // Add right-side content.
+ colgroup.appendChild(document.createElement('col'));
+
+ outputEl.appendChild(colgroup);
+ };
+
GrDiffBuilderSideBySide.prototype._createRow = function(section, leftLine,
rightLine) {
var row = this._createElement('tr');
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-unified.js b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-unified.js
index e1f3ed1..d2c543d 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-unified.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-unified.js
@@ -33,6 +33,26 @@
return sectionEl;
};
+ GrDiffBuilderUnified.prototype.addColumns = function(outputEl, fontSize) {
+ var width = fontSize * 4;
+ var colgroup = document.createElement('colgroup');
+
+ // Add left-side line number.
+ var col = document.createElement('col');
+ col.setAttribute('width', width);
+ colgroup.appendChild(col);
+
+ // Add right-side line number.
+ col = document.createElement('col');
+ col.setAttribute('width', width);
+ colgroup.appendChild(col);
+
+ // Add the content.
+ colgroup.appendChild(document.createElement('col'));
+
+ outputEl.appendChild(colgroup);
+ };
+
GrDiffBuilderUnified.prototype._createRow = function(section, line) {
var row = this._createElement('tr', line.type);
var lineEl = this._createLineEl(line, line.beforeNumber,
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html
index 0a595c7..c944db4 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html
@@ -126,6 +126,7 @@
this.$.processor.keyLocations = this._getCommentLocations(comments);
this._clearDiffContent();
+ this._builder.addColumns(this.diffElement, prefs.font_size);
var reporting = this.$.reporting;
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 c524f7f..d987972 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
@@ -65,7 +65,20 @@
var PARTIAL_CONTEXT_AMOUNT = 10;
- GrDiffBuilder.prototype.buildSectionElement = function(group) {
+ /**
+ * Abstract method
+ * @param {string} outputEl
+ * @param {number} fontSize
+ */
+ GrDiffBuilder.prototype.addColumns = function() {
+ throw Error('Subclasses must implement addColumns');
+ };
+
+ /**
+ * Abstract method
+ * @param {Object} group
+ */
+ GrDiffBuilder.prototype.buildSectionElement = function() {
throw Error('Subclasses must implement buildGroupElement');
};
@@ -377,7 +390,8 @@
var html = util.escapeHTML(text);
html = this._addTabWrappers(html, this._prefs.tab_size);
- if (this._textLength(text, this._prefs.tab_size) >
+ if (!this._prefs.line_wrapping &&
+ this._textLength(text, this._prefs.tab_size) >
this._prefs.line_length) {
html = this._addNewlines(text, html);
}
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html
index 69fea31..ad5173d 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html
@@ -124,6 +124,36 @@
'6789');
});
+ test('_addNewlines not called if line_wrapping is true', function(done) {
+ builder._prefs = {line_wrapping: true, tab_size: 4, line_length: 50};
+ var text = (new Array(52)).join('a');
+
+ var line = {text: text, highlights: []};
+ var newLineStub = sinon.stub(builder, '_addNewlines');
+ builder._createTextEl(line);
+ flush(function() {
+ assert.isFalse(newLineStub.called);
+ newLineStub.restore();
+ done();
+ });
+ });
+
+ test('_addNewlines called if line_wrapping is true and meets other ' +
+ 'conditions', function(done) {
+ builder._prefs = {line_wrapping: false, tab_size: 4, line_length: 50};
+ var text = (new Array(52)).join('a');
+
+ var line = {text: text, highlights: []};
+ var newLineStub = sinon.stub(builder, '_addNewlines');
+ builder._createTextEl(line);
+
+ flush(function() {
+ assert.isTrue(newLineStub.called);
+ newLineStub.restore();
+ done();
+ });
+ });
+
test('text length with tabs and unicode', function() {
assert.equal(builder._textLength('12345', 4), 5);
assert.equal(builder._textLength('\t\t12', 4), 10);
@@ -531,8 +561,10 @@
suite('rendering', function() {
var content;
var outputEl;
+ var sandbox;
setup(function(done) {
+ sandbox = sinon.sandbox.create();
var prefs = {
line_length: 10,
show_tabs: true,
@@ -553,19 +585,15 @@
},
];
stub('gr-reporting', {
- time: sinon.stub(),
- timeEnd: sinon.stub(),
+ time: sandbox.stub(),
+ timeEnd: sandbox.stub(),
});
element = fixture('basic');
outputEl = element.queryEffectiveChildren('#diffTable');
- var renderHandler = function() {
- element.removeEventListener('render', renderHandler);
- done();
- };
- element.addEventListener('render', renderHandler);
- sinon.stub(element, '_getDiffBuilder', function() {
+ sandbox.stub(element, '_getDiffBuilder', function() {
var builder = new GrDiffBuilder(
{content: content}, {left: [], right: []}, prefs, outputEl);
+ sandbox.stub(builder, 'addColumns');
builder.buildSectionElement = function(group) {
var section = document.createElement('stub');
section.textContent = group.lines.reduce(function(acc, line) {
@@ -576,21 +604,23 @@
return builder;
});
element.diff = {content: content};
- element.render({left: [], right: []}, prefs);
+ element.render({left: [], right: []}, prefs).then(done);
+ });
+
+ teardown(function() {
+ sandbox.restore();
});
test('reporting', function(done) {
var timeStub = element.$.reporting.time;
var timeEndStub = element.$.reporting.timeEnd;
- flush(function() {
- assert.isTrue(timeStub.calledWithExactly('Diff Total Render'));
- assert.isTrue(timeStub.calledWithExactly('Diff Content Render'));
- assert.isTrue(timeStub.calledWithExactly('Diff Syntax Render'));
- assert.isTrue(timeEndStub.calledWithExactly('Diff Total Render'));
- assert.isTrue(timeEndStub.calledWithExactly('Diff Content Render'));
- assert.isTrue(timeEndStub.calledWithExactly('Diff Syntax Render'));
- done();
- });
+ assert.isTrue(timeStub.calledWithExactly('Diff Total Render'));
+ assert.isTrue(timeStub.calledWithExactly('Diff Content Render'));
+ assert.isTrue(timeStub.calledWithExactly('Diff Syntax Render'));
+ assert.isTrue(timeEndStub.calledWithExactly('Diff Total Render'));
+ assert.isTrue(timeEndStub.calledWithExactly('Diff Content Render'));
+ assert.isTrue(timeEndStub.calledWithExactly('Diff Syntax Render'));
+ done();
});
test('renderSection', function() {
@@ -602,6 +632,11 @@
assert.equal(section.innerHTML, prevInnerHTML);
});
+ test('addColumns is called', function(done) {
+ element.render({left: [], right: []}, {}).then(done);
+ assert.isTrue(element._builder.addColumns.called);
+ });
+
test('getSectionsByLineRange one line', function() {
var section = outputEl.querySelector('stub:nth-of-type(2)');
var sections = element._builder.getSectionsByLineRange(1, 1, 'left');
@@ -622,8 +657,7 @@
test('render-start and render are fired', function(done) {
var fireStub = sinon.stub(element, 'fire');
- element.render({left: [], right: []}, {});
- flush(function() {
+ element.render({left: [], right: []}, {}).then(function() {
assert.isTrue(fireStub.calledWithExactly('render-start'));
assert.isTrue(fireStub.calledWithExactly('render'));
done();
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-preferences/gr-diff-preferences.html b/polygerrit-ui/app/elements/diff/gr-diff-preferences/gr-diff-preferences.html
index 4e64dcd..1188dd3 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-preferences/gr-diff-preferences.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-preferences/gr-diff-preferences.html
@@ -87,7 +87,15 @@
</select>
</div>
<div class="pref">
- <label for="columnsInput">Columns</label>
+ <label for="lineWrappingInput">Fit to Screen</label>
+ <input
+ is="iron-input"
+ type="checkbox"
+ id="lineWrappingInput"
+ on-tap="_handlelineWrappingTap">
+ </div>
+ <div class="pref" id="columnsPref" hidden$="[[_newPrefs.line_wrapping]]">
+ <label for="columnsInput">Diff Width</label>
<input is="iron-input" type="number" id="columnsInput"
prevent-invalid-input
allowed-pattern="[0-9]"
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-preferences/gr-diff-preferences.js b/polygerrit-ui/app/elements/diff/gr-diff-preferences/gr-diff-preferences.js
index 9bccd23..2d6bd8c 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-preferences/gr-diff-preferences.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-preferences/gr-diff-preferences.js
@@ -72,6 +72,7 @@
this._newPrefs = Object.assign({}, prefs);
this.$.contextSelect.value = prefs.context;
this.$.showTabsInput.checked = prefs.show_tabs;
+ this.$.lineWrappingInput.checked = prefs.line_wrapping;
this.$.syntaxHighlightInput.checked = prefs.syntax_highlighting;
},
@@ -95,6 +96,10 @@
Polymer.dom(e).rootTarget.checked);
},
+ _handlelineWrappingTap: function(e) {
+ this.set('_newPrefs.line_wrapping', Polymer.dom(e).rootTarget.checked);
+ },
+
_handleSave: function() {
this.prefs = this._newPrefs;
this.localPrefs = this._newLocalPrefs;
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-preferences/gr-diff-preferences_test.html b/polygerrit-ui/app/elements/diff/gr-diff-preferences/gr-diff-preferences_test.html
index c595663..9ec8a02 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-preferences/gr-diff-preferences_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-preferences/gr-diff-preferences_test.html
@@ -56,15 +56,29 @@
element.$.tabSizeInput.bindValue = 4;
MockInteractions.tap(element.$.showTabsInput);
MockInteractions.tap(element.$.syntaxHighlightInput);
+ MockInteractions.tap(element.$.lineWrappingInput);
assert.equal(element._newPrefs.context, 50);
assert.equal(element._newPrefs.font_size, 10);
assert.equal(element._newPrefs.line_length, 80);
assert.equal(element._newPrefs.tab_size, 4);
assert.isFalse(element._newPrefs.show_tabs);
+ assert.isTrue(element._newPrefs.line_wrapping);
assert.isFalse(element._newPrefs.syntax_highlighting);
});
+ test('clicking fit to screen hides line length input', function() {
+ element.prefs = {line_wrapping: false};
+
+ assert.isFalse(element.$.columnsPref.hidden);
+
+ MockInteractions.tap(element.$.lineWrappingInput);
+ assert.isTrue(element.$.columnsPref.hidden);
+
+ MockInteractions.tap(element.$.lineWrappingInput);
+ assert.isFalse(element.$.columnsPref.hidden);
+ });
+
test('clicking save button calls _handleSave function', function() {
var savePrefs = sinon.stub(element, '_handleSave');
MockInteractions.tap(element.$.saveButton);
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
index 14c2bb5..0816501 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
@@ -87,8 +87,13 @@
.content {
background-color: #fff;
}
+ .full-width {
+ width: 100%;
+ }
.lineNum,
.content {
+ /* Set font size based the user's diff preference. */
+ font-size: var(--font-size, 12px);
vertical-align: top;
white-space: pre;
}
@@ -113,11 +118,7 @@
allows them to shrink. */
max-width: var(--content-width, 80ch);
min-width: var(--content-width, 80ch);
- }
- .content,
- .lineNum {
- /* Set font size based the user's diff preference. */
- font-size: var(--font-size, 12px);
+ width: var(--content-width, 80ch);
}
.content.add .intraline,
.content.add.darkHighlight {
@@ -137,6 +138,10 @@
background-color: #fef;
color: #849;
}
+ .contentText {
+ white-space: pre-wrap;
+ word-wrap: break-word;
+ }
.contextControl gr-button {
display: inline-block;
font-family: var(--monospace-font-family);
@@ -171,10 +176,11 @@
comments="[[_comments]]"
diff="[[_diff]]"
view-mode="[[viewMode]]"
+ line-wrapping="[[lineWrapping]]"
is-image-diff="[[isImageDiff]]"
base-image="[[_baseImage]]"
revision-image="[[_revisionImage]]">
- <table id="diffTable"></table>
+ <table id="diffTable" class$="[[_diffTableClass]]"></table>
</gr-diff-builder>
</gr-diff-highlight>
</gr-diff-selection>
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 d3d78c4..eb95a77 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
@@ -66,12 +66,21 @@
type: Boolean,
value: false,
},
+ lineWrapping: {
+ type: Boolean,
+ value: false,
+ observer: '_lineWrappingObserver',
+ },
viewMode: {
type: String,
value: DiffViewMode.SIDE_BY_SIDE,
observer: '_viewModeObserver',
},
_diff: Object,
+ _diffTableClass: {
+ type: String,
+ value: '',
+ },
_comments: Object,
_baseImage: Object,
_revisionImage: Object,
@@ -356,9 +365,21 @@
this._prefsChanged(this.prefs);
},
+ _lineWrappingObserver: function() {
+ this._prefsChanged(this.prefs);
+ },
+
_prefsChanged: function(prefs) {
if (!prefs) { return; }
- this.customStyle['--content-width'] = prefs.line_length + 'ch';
+ if (prefs.line_wrapping) {
+ this._diffTableClass = 'full-width';
+ if (this.viewMode === 'SIDE_BY_SIDE') {
+ this.customStyle['--content-width'] = 'none';
+ }
+ } else {
+ this._diffTableClass = '';
+ this.customStyle['--content-width'] = prefs.line_length + 'ch';
+ }
if (!!prefs.font_size) {
this.customStyle['--font-size'] = prefs.font_size + 'px';
diff --git a/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view.html b/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view.html
index 22afbc3..bc9630e 100644
--- a/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view.html
+++ b/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view.html
@@ -210,7 +210,17 @@
</span>
</section>
<section>
- <span class="title">Columns</span>
+ <span class="title">Fit to Screen</span>
+ <span class="value">
+ <input
+ id="lineWrapping"
+ type="checkbox"
+ checked$="[[_diffPrefs.line_wrapping]]"
+ on-change="_handleLineWrappingChanged">
+ </span>
+ </section>
+ <section id="columnsPref" hidden$="[[_diffPrefs.line_wrapping]]">
+ <span class="title">Diff Width</span>
<span class="value">
<input
is="iron-input"
diff --git a/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view.js b/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view.js
index 81a56a1..566b623 100644
--- a/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view.js
+++ b/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view.js
@@ -206,6 +206,10 @@
}.bind(this));
},
+ _handleLineWrappingChanged: function() {
+ this.set('_diffPrefs.line_wrapping', this.$.lineWrapping.checked);
+ },
+
_handleShowTabsChanged: function() {
this.set('_diffPrefs.show_tabs', this.$.showTabs.checked);
},
diff --git a/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view_test.html b/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view_test.html
index 2862226..cb9d2eb 100644
--- a/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view_test.html
+++ b/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view_test.html
@@ -95,6 +95,7 @@
font_size: 12,
line_length: 100,
cursor_blink_rate: 0,
+ line_wrapping: false,
intraline_difference: true,
show_line_endings: true,
show_tabs: true,
@@ -189,7 +190,7 @@
// Rendered with the expected preferences selected.
assert.equal(valueOf('Context', 'diffPreferences')
.firstElementChild.bindValue, diffPreferences.context);
- assert.equal(valueOf('Columns', 'diffPreferences')
+ assert.equal(valueOf('Diff Width', 'diffPreferences')
.firstElementChild.bindValue, diffPreferences.line_length);
assert.equal(valueOf('Tab Width', 'diffPreferences')
.firstElementChild.bindValue, diffPreferences.tab_size);
@@ -197,6 +198,8 @@
.firstElementChild.bindValue, diffPreferences.font_size);
assert.equal(valueOf('Show Tabs', 'diffPreferences')
.firstElementChild.checked, diffPreferences.show_tabs);
+ assert.equal(valueOf('Fit to Screen', 'diffPreferences')
+ .firstElementChild.checked, diffPreferences.line_wrapping);
assert.isFalse(element._diffPrefsChanged);
@@ -221,6 +224,16 @@
});
});
+ test('columns input is hidden with fit to scsreen is selected', function() {
+ assert.isFalse(element.$.columnsPref.hidden);
+
+ MockInteractions.tap(element.$.lineWrapping);
+ assert.isTrue(element.$.columnsPref.hidden);
+
+ MockInteractions.tap(element.$.lineWrapping);
+ assert.isFalse(element.$.columnsPref.hidden);
+ });
+
test('menu', function(done) {
assert.isFalse(element._menuChanged);
assert.isFalse(element._prefsChanged);
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 91bde6b..672afa8 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
@@ -195,6 +195,7 @@
ignore_whitespace: 'IGNORE_NONE',
intraline_difference: true,
line_length: 100,
+ line_wrapping: false,
show_line_endings: true,
show_tabs: true,
show_whitespace_errors: true,
