opendev/gerrit
Code Issues Proposed changes

Annotate trailing whitespace per user preference

Browse Source 
Add a simple annotation layer that marks trailing whitespace in diffs
(guarded by the `show_whitespace_errors` diff preference). The newly
supported diff preference is added to both diff preference controls. The
requirement that all annotation layers must implement `addListener` is
relaxed as the trailing whitespace layer is the third layer that doesn't
use it.

Adds tests for the layer and the diff preference.

Feature: Issue 4836
Change-Id: Ifba05216bf0bc3c0a8a094f5ef392b983091d59f
This commit is contained in:
Wyatt Allen
2016-11-17 12:03:51 -08:00
parent 7959720c79
commit d970500d62
11 changed files with 165 additions and 6 deletions
Download Patch File Download Diff File Expand all files Collapse all files

32
polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html
View File

@@ -62,6 +62,8 @@ limitations under the License.
// syntax highlighting for the entire file.
var SYNTAX_MAX_LINE_LENGTH = 500;
var TRAILING_WHITESPACE_PATTERN = /\s+$/;
Polymer({
is: 'gr-diff-builder',
@@ -101,6 +103,7 @@ limitations under the License.
attached: function() {
// Setup annotation layers.
this._layers = [
this._createTrailingWhitespaceLayer(),
this.$.syntaxLayer,
this._createIntralineLayer(),
this._createTabIndicatorLayer(),
@@ -115,6 +118,7 @@ limitations under the License.
render: function(comments, prefs) {
this.$.syntaxLayer.enabled = prefs.syntax_highlighting;
this._showTabs = !!prefs.show_tabs;
this._showTrailingWhitespace = !!prefs.show_whitespace_errors;
// Stop the processor (if it's running).
this.$.processor.cancel();
@@ -323,8 +327,6 @@ limitations under the License.
_createIntralineLayer: function() {
return {
addListener: function() {},
// Take a DIV.contentText element and a line object with intraline
// differences to highlight and apply them to the element as
// annotations.
@@ -351,9 +353,8 @@ limitations under the License.
},
_createTabIndicatorLayer: function() {
var show = (function() { return this._showTabs; }).bind(this);
var show = function() { return this._showTabs; }.bind(this);
return {
addListener: function() {},
annotate: function(el, line) {
// If visible tabs are disabled, do nothing.
if (!show()) { return; }
@@ -375,6 +376,29 @@ limitations under the License.
};
},
_createTrailingWhitespaceLayer: function() {
var show = function() {
return this._showTrailingWhitespace;
}.bind(this);
return {
annotate: function(el, line) {
if (!show()) { return; }
var match = line.text.match(TRAILING_WHITESPACE_PATTERN);
if (match) {
// Normalize string positions in case there is unicode before or
// within the match.
var index = GrAnnotation.getStringLength(
line.text.substr(0, match.index));
var length = GrAnnotation.getStringLength(match[0]);
GrAnnotation.annotateElement(el, index, length,
'style-scope gr-diff trailing-whitespace');
}
},
};
},
/**
* In pages with large diffs, creating the first comment thread can be
* slow because nested Polymer elements (particularly

4
polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js
View File

@@ -29,7 +29,9 @@
this.layers = layers || [];
this.layers.forEach(function(layer) {
layer.addListener(this._handleLayerUpdate.bind(this));
if (layer.addListener) {
layer.addListener(this._handleLayerUpdate.bind(this));
}
}.bind(this));
}

94
polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html
View File

@@ -558,6 +558,100 @@ limitations under the License.
});
});
suite('trailing whitespace', function() {
var sandbox;
var element;
var layer;
setup(function() {
sandbox = sinon.sandbox.create();
element = fixture('basic');
element._showTrailingWhitespace = true;
layer = element._createTrailingWhitespaceLayer();
});
teardown(function() {
sandbox.restore();
});
test('does nothing with empty line', function() {
var line = {text: ''};
var el = document.createElement('div');
var annotateElementStub = sandbox.stub(GrAnnotation, 'annotateElement');
layer.annotate(el, line);
assert.isFalse(annotateElementStub.called);
});
test('does nothing with no trailing whitespace', function() {
var str = 'lorem ipsum blah blah';
var line = {text: str};
var el = document.createElement('div');
el.textContent = str;
var annotateElementStub = sandbox.stub(GrAnnotation, 'annotateElement');
layer.annotate(el, line);
assert.isFalse(annotateElementStub.called);
});
test('annotates trailing spaces', function() {
var str = 'lorem ipsum ';
var line = {text: str};
var el = document.createElement('div');
el.textContent = str;
var annotateElementStub = sandbox.stub(GrAnnotation, 'annotateElement');
layer.annotate(el, line);
assert.isTrue(annotateElementStub.called);
assert.equal(annotateElementStub.lastCall.args[1], 11);
assert.equal(annotateElementStub.lastCall.args[2], 3);
});
test('annotates trailing tabs', function() {
var str = 'lorem ipsum\t\t\t';
var line = {text: str};
var el = document.createElement('div');
el.textContent = str;
var annotateElementStub = sandbox.stub(GrAnnotation, 'annotateElement');
layer.annotate(el, line);
assert.isTrue(annotateElementStub.called);
assert.equal(annotateElementStub.lastCall.args[1], 11);
assert.equal(annotateElementStub.lastCall.args[2], 3);
});
test('annotates mixed trailing whitespace', function() {
var str = 'lorem ipsum\t \t';
var line = {text: str};
var el = document.createElement('div');
el.textContent = str;
var annotateElementStub = sandbox.stub(GrAnnotation, 'annotateElement');
layer.annotate(el, line);
assert.isTrue(annotateElementStub.called);
assert.equal(annotateElementStub.lastCall.args[1], 11);
assert.equal(annotateElementStub.lastCall.args[2], 3);
});
test('unicode preceding trailing whitespace', function() {
var str = '💢\t';
var line = {text: str};
var el = document.createElement('div');
el.textContent = str;
var annotateElementStub = sandbox.stub(GrAnnotation, 'annotateElement');
layer.annotate(el, line);
assert.isTrue(annotateElementStub.called);
assert.equal(annotateElementStub.lastCall.args[1], 1);
assert.equal(annotateElementStub.lastCall.args[2], 1);
});
test('does not annotate when disabled', function() {
element._showTrailingWhitespace = false;
var str = 'lorem upsum\t \t ';
var line = {text: str};
var el = document.createElement('div');
el.textContent = str;
var annotateElementStub = sandbox.stub(GrAnnotation, 'annotateElement');
layer.annotate(el, line);
assert.isFalse(annotateElementStub.called);
});
});
suite('rendering', function() {
var content;
var outputEl;

6
polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-annotation.js
View File

@@ -32,7 +32,11 @@
* @return {Number} The length of the text.
*/
getLength: function(node) {
return node.textContent.replace(REGEX_ASTRAL_SYMBOL, '_').length;
return this.getStringLength(node.textContent);
},
getStringLength: function(str) {
return str.replace(REGEX_ASTRAL_SYMBOL, '_').length;
},
/**

5
polygerrit-ui/app/elements/diff/gr-diff-preferences/gr-diff-preferences.html
View File

@@ -120,6 +120,11 @@ limitations under the License.
<input is="iron-input" type="checkbox" id="showTabsInput"
on-tap="_handleShowTabsTap">
</div>
<div class="pref">
<label for="showTrailingWhitespaceInput">Show Trailing Whitespace</label>
<input is="iron-input" type="checkbox" id="showTrailingWhitespaceInput"
on-tap="_handleShowTrailingWhitespaceTap">
</div>
<div class="pref">
<label for="syntaxHighlightInput">Syntax highlighting</label>
<input is="iron-input" type="checkbox" id="syntaxHighlightInput"

6
polygerrit-ui/app/elements/diff/gr-diff-preferences/gr-diff-preferences.js
View File

@@ -72,6 +72,7 @@
this._newPrefs = Object.assign({}, prefs);
this.$.contextSelect.value = prefs.context;
this.$.showTabsInput.checked = prefs.show_tabs;
this.$.showTrailingWhitespaceInput.checked = prefs.show_whitespace_errors;
this.$.lineWrappingInput.checked = prefs.line_wrapping;
this.$.syntaxHighlightInput.checked = prefs.syntax_highlighting;
},
@@ -91,6 +92,11 @@
this.set('_newPrefs.show_tabs', Polymer.dom(e).rootTarget.checked);
},
_handleShowTrailingWhitespaceTap: function(e) {
this.set('_newPrefs.show_whitespace_errors',
Polymer.dom(e).rootTarget.checked);
},
_handleSyntaxHighlightTap: function(e) {
this.set('_newPrefs.syntax_highlighting',
Polymer.dom(e).rootTarget.checked);

3
polygerrit-ui/app/elements/diff/gr-diff-preferences/gr-diff-preferences_test.html
View File

@@ -45,6 +45,7 @@ limitations under the License.
line_length: 100,
show_tabs: true,
tab_size: 8,
show_whitespace_errors: true,
syntax_highlighting: true,
};
assert.deepEqual(element.prefs, element._newPrefs);
@@ -55,6 +56,7 @@ limitations under the License.
element.$.fontSizeInput.bindValue = 10;
element.$.tabSizeInput.bindValue = 4;
MockInteractions.tap(element.$.showTabsInput);
MockInteractions.tap(element.$.showTrailingWhitespaceInput);
MockInteractions.tap(element.$.syntaxHighlightInput);
MockInteractions.tap(element.$.lineWrappingInput);
@@ -63,6 +65,7 @@ limitations under the License.
assert.equal(element._newPrefs.line_length, 80);
assert.equal(element._newPrefs.tab_size, 4);
assert.isFalse(element._newPrefs.show_tabs);
assert.isFalse(element._newPrefs.show_whitespace_errors);
assert.isTrue(element._newPrefs.line_wrapping);
assert.isFalse(element._newPrefs.syntax_highlighting);
});

4
polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
View File

@@ -165,6 +165,10 @@ limitations under the License.
/* >> character */
content: '\00BB';
}
.trailing-whitespace {
border-radius: .4em;
background-color: #FF9AD2;
}
</style>
<style include="gr-theme-default"></style>
<div class$="[[_computeContainerClass(_loggedIn, viewMode, displayLine)]]"

10
polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view.html
View File

@@ -262,6 +262,16 @@ limitations under the License.
on-change="_handleShowTabsChanged">
</span>
</section>
<section>
<span class="title">Show Trailing Whitespace</span>
<span class="value">
<input
id="showTrailingWhitespace"
type="checkbox"
checked$="[[_diffPrefs.show_whitespace_errors]]"
on-change="_handleShowTrailingWhitespaceChanged">
</span>
</section>
<section>
<span class="title">Syntax Highlighting</span>
<span class="value">

5
polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view.js
View File

@@ -214,6 +214,11 @@
this.set('_diffPrefs.show_tabs', this.$.showTabs.checked);
},
_handleShowTrailingWhitespaceChanged: function() {
this.set('_diffPrefs.show_whitespace_errors',
this.$.showTrailingWhitespace.checked);
},
_handleSyntaxHighlightingChanged: function() {
this.set('_diffPrefs.syntax_highlighting',
this.$.syntaxHighlighting.checked);

2
polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view_test.html
View File

@@ -198,6 +198,8 @@ limitations under the License.
.firstElementChild.bindValue, diffPreferences.font_size);
assert.equal(valueOf('Show Tabs', 'diffPreferences')
.firstElementChild.checked, diffPreferences.show_tabs);
assert.equal(valueOf('Show Trailing Whitespace', 'diffPreferences')
.firstElementChild.checked, diffPreferences.show_whitespace_errors);
assert.equal(valueOf('Fit to Screen', 'diffPreferences')
.firstElementChild.checked, diffPreferences.line_wrapping);