From f66706c1b653f64cee8ec613423ad914e13aa036 Mon Sep 17 00:00:00 2001 From: Ole Rehmsen Date: Tue, 13 Nov 2018 10:21:54 +0100 Subject: [PATCH] Unify ranges to use snake_case 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 --- .../gr-diff-comment-thread.js | 22 ++-- .../gr-diff-comment-thread_test.html | 16 ++- .../gr-diff-highlight/gr-diff-highlight.js | 8 +- .../gr-diff-highlight_test.html | 112 +++++++++--------- .../diff/gr-diff-host/gr-diff-host.js | 13 +- .../diff/gr-diff-host/gr-diff-host_test.html | 8 +- .../app/elements/diff/gr-diff/gr-diff.js | 6 +- .../gr-selection-action-box.js | 8 +- .../gr-selection-action-box_test.html | 8 +- 9 files changed, 103 insertions(+), 98 deletions(-) diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.js b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.js index f3e3249591..a2439d70fd 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.js +++ b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.js @@ -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; diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread_test.html b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread_test.html index 58648bf4c0..188149721c 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread_test.html +++ b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread_test.html @@ -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')); diff --git a/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight.js b/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight.js index 577eec6eda..b80c210a6f 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight.js +++ b/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight.js @@ -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) { diff --git a/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight_test.html b/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight_test.html index 98d55c04ac..d3ae66bb8f 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight_test.html +++ b/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight_test.html @@ -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'); }); diff --git a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.js b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.js index 5e4a3fd701..814c72688a 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.js +++ b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.js @@ -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; } /** diff --git a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.html b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.html index c7ee1c2811..423bdc673f 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.html +++ b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.html @@ -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( 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 76a62b8b64..8b9c1d9341 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js +++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js @@ -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)) { diff --git a/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box.js b/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box.js index 0f848778c4..fa5c810b8a 100644 --- a/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box.js +++ b/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box.js @@ -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, diff --git a/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box_test.html b/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box_test.html index 4f1065adbb..dece36600f 100644 --- a/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box_test.html +++ b/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box_test.html @@ -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;