opendev/gerrit
Code Issues Proposed changes

Unify ranges to use snake_case

Browse Source 
For consistency with the API [1], to make it less confusing.

[1] https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#comment-range

Change-Id: I1a77af83bdeadfce4baa629a500a6ea04762e856
This commit is contained in:
Ole Rehmsen
2018-11-13 10:21:54 +01:00
parent 75d6058f4e
commit f66706c1b6
9 changed files with 103 additions and 98 deletions
Download Patch File Download Diff File Expand all files Collapse all files

22
polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.js
View File

@@ -48,11 +48,12 @@
* version is the one whose line number column is further to the left.
*
* range:
* The range of text that the comment refers to (startLine, startChar,
* endLine, endChar), serialized as JSON. If set, range's startLine
* will have the same value as line-num. Line numbers are 1-based,
* char numbers are 0-based. The start position (startLine, startChar)
* is inclusive, and the end position (endLine, endChar) is exclusive.
* The range of text that the comment refers to (start_line,
* start_character, end_line, end_character), serialized as JSON. If
* set, range's end_line will have the same value as line-num. Line
* numbers are 1-based, char numbers are 0-based. The start position
* (start_line, start_character) is inclusive, and the end position
* (end_line, end_character) is exclusive.
*/
properties: {
changeNum: String,
@@ -61,8 +62,8 @@
value() { return []; },
},
/**
* @type {?{startLine: number, startChar: number, endLine: number,
* endChar: number}}
* @type {?{start_line: number, start_character: number, end_line: number,
* end_character: number}}
*/
range: {
type: Object,
@@ -390,12 +391,7 @@
d.line = opt_lineNum;
}
if (opt_range) {
d.range = {
start_line: opt_range.startLine,
start_character: opt_range.startChar,
end_line: opt_range.endLine,
end_character: opt_range.endChar,
};
d.range = opt_range;
}
if (this.parentIndex) {
d.parent = this.parentIndex;

16
polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread_test.html
View File

@@ -724,15 +724,25 @@ limitations under the License.
});
test('reflects range to JSON serialized attribute if set', () => {
element.range = {startLine: 4, endLine: 5, startChar: 6, endChar: 7};
element.range = {
start_line: 4,
end_line: 5,
start_character: 6,
end_character: 7,
};
assert.deepEqual(
JSON.parse(element.getAttribute('range')),
{startLine: 4, endLine: 5, startChar: 6, endChar: 7});
{start_line: 4, end_line: 5, start_character: 6, end_character: 7});
});
test('removes range attribute if range is unset', () => {
element.range = {startLine: 4, endLine: 5, startChar: 6, endChar: 7};
element.range = {
start_line: 4,
end_line: 5,
start_character: 6,
end_character: 7,
};
element.range = undefined;
assert.notOk(element.hasAttribute('range'));

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

@@ -295,10 +295,10 @@
const root = Polymer.dom(this.root);
root.insertBefore(actionBox, root.firstElementChild);
actionBox.range = {
startLine: start.line,
startChar: start.column,
endLine: end.line,
endChar: end.column,
start_line: start.line,
start_character: start.column,
end_line: end.line,
end_character: end.column,
};
actionBox.side = start.side;
if (start.line === end.line) {

112
polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight_test.html
View File

@@ -318,10 +318,10 @@ limitations under the License.
const actionBox = element.$$('gr-selection-action-box');
assert.isTrue(element.isRangeSelected());
assert.deepEqual(getActionRange(), {
startLine: 138,
startChar: 5,
endLine: 138,
endChar: 12,
start_line: 138,
start_character: 5,
end_line: 138,
end_character: 12,
});
assert.equal(getActionSide(), 'left');
assert.notOk(actionBox.positionBelow);
@@ -337,10 +337,10 @@ limitations under the License.
const actionBox = element.$$('gr-selection-action-box');
assert.deepEqual(getActionRange(), {
startLine: 119,
startChar: 10,
endLine: 120,
endChar: 36,
start_line: 119,
start_character: 10,
end_line: 120,
end_character: 36,
});
assert.equal(getActionSide(), 'right');
assert.notOk(actionBox.positionBelow);
@@ -370,10 +370,10 @@ limitations under the License.
element._handleSelection();
assert.isTrue(element.isRangeSelected());
assert.deepEqual(getActionRange(), {
startLine: 119,
startChar: 10,
endLine: 120,
endChar: 36,
start_line: 119,
start_character: 10,
end_line: 120,
end_character: 36,
});
});
@@ -383,10 +383,10 @@ limitations under the License.
emulateSelection(startContent.firstChild, 10, endContent.firstChild, 2);
assert.isTrue(element.isRangeSelected());
assert.deepEqual(getActionRange(), {
startLine: 119,
startChar: 10,
endLine: 120,
endChar: 2,
start_line: 119,
start_character: 10,
end_line: 120,
end_character: 2,
});
assert.equal(getActionSide(), 'right');
});
@@ -404,10 +404,10 @@ limitations under the License.
emulateSelection(hl.firstChild, 2, hl.nextSibling, 7);
assert.isTrue(element.isRangeSelected());
assert.deepEqual(getActionRange(), {
startLine: 140,
startChar: 8,
endLine: 140,
endChar: 23,
start_line: 140,
start_character: 8,
end_line: 140,
end_character: 23,
});
assert.equal(getActionSide(), 'left');
});
@@ -418,10 +418,10 @@ limitations under the License.
emulateSelection(hl.previousSibling, 2, hl.firstChild, 3);
assert.isTrue(element.isRangeSelected());
assert.deepEqual(getActionRange(), {
startLine: 140,
startChar: 18,
endLine: 140,
endChar: 27,
start_line: 140,
start_character: 18,
end_line: 140,
end_character: 27,
});
});
@@ -431,10 +431,10 @@ limitations under the License.
emulateSelection(content.firstChild, 2, hl.firstChild, 2);
assert.isTrue(element.isRangeSelected());
assert.deepEqual(getActionRange(), {
startLine: 140,
startChar: 2,
endLine: 140,
endChar: 61,
start_line: 140,
start_character: 2,
end_line: 140,
end_character: 61,
});
assert.equal(getActionSide(), 'left');
});
@@ -470,10 +470,10 @@ limitations under the License.
emulateSelection(comment.firstChild, 2, endContent.firstChild, 4);
assert.isTrue(element.isRangeSelected());
assert.deepEqual(getActionRange(), {
startLine: 140,
startChar: 83,
endLine: 141,
endChar: 4,
start_line: 140,
start_character: 83,
end_line: 141,
end_character: 4,
});
assert.equal(getActionSide(), 'left');
});
@@ -485,10 +485,10 @@ limitations under the License.
emulateSelection(content.firstChild, 4, comment.firstChild, 1);
assert.isTrue(element.isRangeSelected());
assert.deepEqual(getActionRange(), {
startLine: 140,
startChar: 4,
endLine: 140,
endChar: 83,
start_line: 140,
start_character: 4,
end_line: 140,
end_character: 83,
});
assert.equal(getActionSide(), 'left');
});
@@ -517,10 +517,10 @@ limitations under the License.
emulateSelection(startContent.firstChild, 3, endContent.firstChild, 14);
assert.isTrue(element.isRangeSelected());
assert.deepEqual(getActionRange(), {
startLine: 130,
startChar: 3,
endLine: 146,
endChar: 14,
start_line: 130,
start_character: 3,
end_line: 146,
end_character: 14,
});
assert.equal(getActionSide(), 'right');
});
@@ -531,10 +531,10 @@ limitations under the License.
content.firstChild, 1, content.querySelector('span'), 0);
assert.isTrue(element.isRangeSelected());
assert.deepEqual(getActionRange(), {
startLine: 140,
startChar: 1,
endLine: 140,
endChar: 51,
start_line: 140,
start_character: 1,
end_line: 140,
end_character: 51,
});
assert.equal(getActionSide(), 'left');
});
@@ -546,10 +546,10 @@ limitations under the License.
content.querySelectorAll('span')[1].nextSibling, 1);
assert.isTrue(element.isRangeSelected());
assert.deepEqual(getActionRange(), {
startLine: 140,
startChar: 51,
endLine: 140,
endChar: 71,
start_line: 140,
start_character: 51,
end_line: 140,
end_character: 71,
});
assert.equal(getActionSide(), 'left');
});
@@ -582,10 +582,10 @@ limitations under the License.
emulateSelection(startContent.firstChild, 0, endContent.firstChild, 0);
assert.isTrue(element.isRangeSelected());
assert.deepEqual(getActionRange(), {
startLine: 119,
startChar: 0,
endLine: 119,
endChar: element._getLength(startContent),
start_line: 119,
start_character: 0,
end_line: 119,
end_character: element._getLength(startContent),
});
assert.equal(getActionSide(), 'right');
});
@@ -597,10 +597,10 @@ limitations under the License.
endContent.parentElement.previousElementSibling, 0);
assert.isTrue(element.isRangeSelected());
assert.deepEqual(getActionRange(), {
startLine: 146,
startChar: 0,
endLine: 146,
endChar: 84,
start_line: 146,
start_character: 0,
end_line: 146,
end_character: 84,
});
assert.equal(getActionSide(), 'right');
});

13
polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.js
View File

@@ -45,11 +45,6 @@
return !!(diff.binary && (isA || isB));
}
/** @typedef {{startLine: number, startChar: number,
* endLine: number, endChar: number}} */
Gerrit.Range;
/**
* Compare two ranges. Either argument may be falsy, but will only return
* true if both are falsy or if neither are falsy and have the same position
@@ -62,10 +57,10 @@
function rangesEqual(a, b) {
if (!a && !b) { return true; }
if (!a || !b) { return false; }
return a.startLine === b.startLine &&
a.startChar === b.startChar &&
a.endLine === b.endLine &&
a.endChar === b.endChar;
return a.start_line === b.start_line &&
a.start_character === b.start_character &&
a.end_line === b.end_line &&
a.end_character === b.end_character;
}
/**

8
polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.html
View File

@@ -966,10 +966,10 @@ limitations under the License.
// Try to fetch a thread with a different range.
range = {
startLine: 1,
startChar: 1,
endLine: 1,
endChar: 3,
start_line: 1,
start_character: 1,
end_line: 1,
end_character: 3,
};
assert.isOk(element._getOrCreateThread(

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

@@ -39,6 +39,10 @@
const FULL_CONTEXT = -1;
const LIMITED_CONTEXT = 10;
/** @typedef {{start_line: number, start_character: number,
* end_line: number, end_character: number}} */
Gerrit.Range;
Polymer({
is: 'gr-diff',
@@ -305,7 +309,7 @@
_handleCreateRangeComment(e) {
const range = e.detail.range;
const side = e.detail.side;
const lineNum = range.endLine;
const lineNum = range.end_line;
const lineEl = this.$.diffBuilder.getLineElByNumber(lineNum, side);
if (this._isValidElForComment(lineEl)) {

8
polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box.js
View File

@@ -34,10 +34,10 @@
range: {
type: Object,
value: {
startLine: NaN,
startChar: NaN,
endLine: NaN,
endChar: NaN,
start_line: NaN,
start_character: NaN,
end_line: NaN,
end_character: NaN,
},
},
positionBelow: Boolean,

8
polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box_test.html
View File

@@ -89,10 +89,10 @@ limitations under the License.
test('event fired contains playload', () => {
const side = 'left';
const range = {
startLine: 1,
startChar: 11,
endLine: 2,
endChar: 42,
start_line: 1,
start_character: 11,
end_line: 2,
end_character: 42,
};
element.side = 'left';
element.range = range;