opendev/gerrit
Code Issues Proposed changes

Fix diff cursor in file list

Browse Source 
There was an issue where line numbers would not properly highlight in
inline-diffs rendered in the file view.

There were two separate issues that contributed to this problem:
1 - The diff cursor was not notified of the line being selected and
    needing to move to that cursor stop.
2 - The diff cursor was not properly updated after being moved inside
    a dom-if statement.

Additionally, the way that line selection was previously done was not
extendable to a diff cursor that stored multiple files. It queried
stops based on line number and side. This change also stores the
path of the file in the cursor stop so that the correct diff/line is
selected in the case of multiples.

Bug: Issue 5977
Change-Id: I7496293e19a4e59a3855dc78e273de6a9852e556
This commit is contained in:
Becky Siegel
2017-04-13 14:48:31 -07:00
parent 63f9ee8e36
commit eccee3ce49
7 changed files with 204 additions and 17 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html
View File

@@ -325,6 +325,7 @@ limitations under the License.
path="[[file.__path]]"
prefs="[[_diffPrefs]]"
project-config="[[projectConfig]]"
on-line-selected="_onLineSelected"
view-mode="[[_getDiffViewMode(diffViewMode, _userPrefs)]]"></gr-diff>
</template>
</template>

14
polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js
View File

@@ -686,18 +686,19 @@
}
},
_filesChanged: function() {
this.async(function() {
_updateDiffCursor: function() {
var diffElements = Polymer.dom(this.root).querySelectorAll('gr-diff');
// Overwrite the cursor's list of diffs:
this.$.diffCursor.splice.apply(this.$.diffCursor,
['diffs', 0, this.$.diffCursor.diffs.length].concat(diffElements));
},
_filesChanged: function(files) {
Polymer.dom.flush();
var files = Polymer.dom(this.root).querySelectorAll('.file-row');
this.$.fileCursor.stops = files;
this.$.fileCursor.setCursorAtIndex(this.selectedIndex, true);
}.bind(this), 1);
},
_incrementNumFilesShown: function() {
@@ -769,6 +770,11 @@
return expandedFilesRecord.base.indexOf(path) !== -1;
},
_onLineSelected: function(e, detail) {
this.$.diffCursor.moveToLineNumber(detail.number, detail.side,
detail.path);
},
/**
* Handle splices to the list of expanded file paths. If there are any new
* entries in the expanded list, then render each diff corresponding in
@@ -798,6 +804,8 @@
this.$.reporting.timeEnd(timerName);
this.$.diffCursor.handleDiffUpdate();
}.bind(this));
this._updateDiffCursor();
this.$.diffCursor.handleDiffUpdate();
},
/**

168
polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html
View File

@@ -23,6 +23,7 @@ limitations under the License.
<script src="../../../bower_components/page/page.js"></script>
<script src="../../../scripts/util.js"></script>
<link rel="import" href="../../shared/gr-rest-api-interface/mock-diff-response_test.html">
<link rel="import" href="../../../bower_components/iron-test-helpers/iron-test-helpers.html">
<link rel="import" href="gr-file-list.html">
@@ -927,4 +928,171 @@ limitations under the License.
});
});
});
suite('gr-file-list inline diff tests', function() {
var element;
var sandbox;
var saveStub;
var setupDiff = function(diff) {
var mock = document.createElement('mock-diff-response');
diff._diff = mock.diffResponse;
diff._comments = {
left: [],
right: [],
};
diff.prefs = {
context: 10,
tab_size: 8,
font_size: 12,
line_length: 100,
cursor_blink_rate: 0,
line_wrapping: false,
intraline_difference: true,
show_line_endings: true,
show_tabs: true,
show_whitespace_errors: true,
syntax_highlighting: true,
auto_hide_diff_table_header: true,
theme: 'DEFAULT',
ignore_whitespace: 'IGNORE_NONE',
};
diff._renderDiffTable();
};
var renderAndGetNewDiffs = function(index) {
var diffs =
Polymer.dom(element.root).querySelectorAll('gr-diff');
for (var i = index; i < diffs.length; i++) {
setupDiff(diffs[i]);
}
element._updateDiffCursor();
element.$.diffCursor.handleDiffUpdate();
return diffs;
};
setup(function() {
sandbox = sinon.sandbox.create();
stub('gr-rest-api-interface', {
getLoggedIn: function() { return Promise.resolve(true); },
getPreferences: function() { return Promise.resolve({}); },
});
stub('gr-date-formatter', {
_loadTimeFormat: function() { return Promise.resolve(''); },
});
stub('gr-diff', {
reload: function() { return Promise.resolve(); },
});
element = fixture('basic');
element.numFilesShown = 75;
element.selectedIndex = 0;
element._files = [
{__path: '/COMMIT_MSG', lines_inserted: 9},
{
__path: 'file_added_in_rev2.txt',
lines_inserted: 1,
lines_deleted: 1,
size_delta: 10,
size: 100,
},
{
__path: 'myfile.txt',
lines_inserted: 1,
lines_deleted: 1,
size_delta: 10,
size: 100,
},
];
element._reviewed = ['/COMMIT_MSG', 'myfile.txt'];
element._loggedIn = true;
element.changeNum = '42';
element.patchRange = {
basePatchNum: 'PARENT',
patchNum: '2',
};
sandbox.stub(window, 'fetch', function() {
return Promise.resolve();
});
saveStub = sandbox.stub(element, '_saveReviewedState',
function() { return Promise.resolve(); });
flushAsynchronousOperations();
});
teardown(function() {
sandbox.restore();
});
test('cursor with individually opened files', function() {
MockInteractions.pressAndReleaseKeyOn(element, 73, null, 'i');
flushAsynchronousOperations();
var diffs = renderAndGetNewDiffs(0);
var diffStops = diffs[0].getCursorStops();
// 1 diff should be rendered.
assert.equal(diffs.length, 1);
// No line number is selected.
assert.isFalse(diffStops[10].classList.contains('target-row'));
// Tapping content on a line selects the line number.
MockInteractions.tap(Polymer.dom(
diffStops[10]).querySelectorAll('.contentText')[0]);
flushAsynchronousOperations();
assert.isTrue(diffStops[10].classList.contains('target-row'));
// Keyboard shortcuts are still moving the file cursor, not the diff
// cursor.
MockInteractions.pressAndReleaseKeyOn(element, 74, null, 'j');
flushAsynchronousOperations();
assert.isTrue(diffStops[10].classList.contains('target-row'));
assert.isFalse(diffStops[11].classList.contains('target-row'));
// The file cusor is now at 1.
assert.equal(element.$.fileCursor.index, 1);
MockInteractions.pressAndReleaseKeyOn(element, 73, null, 'i');
flushAsynchronousOperations();
var diffs = renderAndGetNewDiffs(1);
// Two diffs should be rendered.
assert.equal(diffs.length, 2);
var diffStopsFirst = diffs[0].getCursorStops();
var diffStopsSecond = diffs[1].getCursorStops();
// The line on the first diff is stil selected
assert.isTrue(diffStopsFirst[10].classList.contains('target-row'));
assert.isFalse(diffStopsSecond[10].classList.contains('target-row'));
});
test('cursor with toggle all files', function() {
MockInteractions.pressAndReleaseKeyOn(element, 73, 'shift', 'i');
flushAsynchronousOperations();
var diffs = renderAndGetNewDiffs(0);
var diffStops = diffs[0].getCursorStops();
// 1 diff should be rendered.
assert.equal(diffs.length, 3);
// No line number is selected.
assert.isFalse(diffStops[10].classList.contains('target-row'));
// Tapping content on a line selects the line number.
MockInteractions.tap(Polymer.dom(
diffStops[10]).querySelectorAll('.contentText')[0]);
flushAsynchronousOperations();
assert.isTrue(diffStops[10].classList.contains('target-row'));
// Keyboard shortcuts are still moving the file cursor, not the diff
// cursor.
MockInteractions.pressAndReleaseKeyOn(element, 74, null, 'j');
flushAsynchronousOperations();
assert.isFalse(diffStops[10].classList.contains('target-row'));
assert.isTrue(diffStops[11].classList.contains('target-row'));
// The file cusor is still at 0.
assert.equal(element.$.fileCursor.index, 0);
});
});
</script>

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

@@ -149,8 +149,8 @@
this._fixSide();
},
moveToLineNumber: function(number, side) {
var row = this._findRowByNumber(number, side);
moveToLineNumber: function(number, side, opt_path) {
var row = this._findRowByNumberAndFile(number, side, opt_path);
if (row) {
this.side = side;
this.$.cursorManager.setCursor(row);
@@ -376,8 +376,16 @@
}
},
_findRowByNumber: function(targetNumber, side) {
var stops = this.$.cursorManager.stops;
_findRowByNumberAndFile: function(targetNumber, side, opt_path) {
var stops;
if (opt_path) {
var diff = this.diffs.filter(function(diff) {
return diff.path === opt_path;
})[0];
stops = diff.getCursorStops();
} else {
stops = this.$.cursorManager.stops;
}
var selector;
for (var i = 0; i < stops.length; i++) {
selector = '.lineNum.' + side + '[data-value="' + targetNumber + '"]';

6
polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor_test.html
View File

@@ -266,13 +266,13 @@ limitations under the License.
assert.equal(cursorElement.getAddress(), '');
});
test('_findRowByNumber', function() {
test('_findRowByNumberAndFile', function() {
// Get the first ab row after the first chunk.
var row = Polymer.dom(diffElement.root).querySelectorAll('tr')[8];
// It should be line 8 on the right, but line 5 on the left.
assert.equal(cursorElement._findRowByNumber(8, 'right'), row);
assert.equal(cursorElement._findRowByNumber(5, 'left'), row);
assert.equal(cursorElement._findRowByNumberAndFile(8, 'right'), row);
assert.equal(cursorElement._findRowByNumberAndFile(5, 'left'), row);
});
});
</script>

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

@@ -186,6 +186,7 @@ limitations under the License.
id="diffBuilder"
comments="[[_comments]]"
diff="[[_diff]]"
diff-path="[[path]]"
view-mode="[[viewMode]]"
line-wrapping="[[lineWrapping]]"
is-image-diff="[[isImageDiff]]"

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

@@ -227,6 +227,7 @@
this.fire('line-selected', {
side: el.classList.contains('left') ? DiffSide.LEFT : DiffSide.RIGHT,
number: el.getAttribute('data-value'),
path: this.path,
});
},