Expose whether moving the cursor was successful
This is useful to handle things such as the next-file toast which is
currently handled by gr-cursor-manager directly, but should probably be
moved out to keep gr-cursor-manager independent from concepts like
files.
Change-Id: I74a531ddcd74274cf18aed888f4fc31400257e37
diff --git a/polygerrit-ui/app/elements/shared/gr-cursor-manager/gr-cursor-manager.ts b/polygerrit-ui/app/elements/shared/gr-cursor-manager/gr-cursor-manager.ts
index ffdd662..47a1cbb 100644
--- a/polygerrit-ui/app/elements/shared/gr-cursor-manager/gr-cursor-manager.ts
+++ b/polygerrit-ui/app/elements/shared/gr-cursor-manager/gr-cursor-manager.ts
@@ -34,6 +34,18 @@
// Time in which pressing n key again after the toast navigates to next file
const NAVIGATE_TO_NEXT_FILE_TIMEOUT_MS = 5000;
+/**
+ * Return type for cursor moves, that indicate whether a move was possible.
+ */
+export enum CursorMoveResult {
+ /** The cursor was successfully moved. */
+ MOVED,
+ /** There were no stops - the cursor was reset. */
+ NO_STOPS,
+ /** There was no more stop to move to - the cursor was clipped to the end. */
+ CLIPPED,
+}
+
@customElement('gr-cursor-manager')
export class GrCursorManager extends GestureEventListeners(
LegacyElementMixin(PolymerElement)
@@ -104,16 +116,16 @@
* back to first instead of to last.
* @param navigateToNextFile Navigate to next unreviewed file
* if user presses next on the last diff chunk
+ * @return If a move was performed or why not.
* @private
*/
-
next(
condition?: Function,
getTargetHeight?: (target: HTMLElement) => number,
clipToTop?: boolean,
navigateToNextFile?: boolean
- ) {
- this._moveCursor(
+ ): CursorMoveResult {
+ return this._moveCursor(
1,
condition,
getTargetHeight,
@@ -122,8 +134,8 @@
);
}
- previous(condition?: Function) {
- this._moveCursor(-1, condition);
+ previous(condition?: Function): CursorMoveResult {
+ return this._moveCursor(-1, condition);
}
/**
@@ -269,6 +281,7 @@
* back to first instead of to last.
* @param navigateToNextFile Navigate to next unreviewed file
* if user presses next on the last diff chunk
+ * @return If a move was performed or why not.
* @private
*/
_moveCursor(
@@ -277,27 +290,25 @@
getTargetHeight?: (target: HTMLElement) => number,
clipToTop?: boolean,
navigateToNextFile?: boolean
- ) {
+ ): CursorMoveResult {
if (!this.stops.length) {
this.unsetCursor();
- return;
+ return CursorMoveResult.NO_STOPS;
}
this._unDecorateTarget();
const newIndex = this._getNextindex(delta, condition, clipToTop);
+ const newTarget = newIndex !== -1 ? this.stops[newIndex] : null;
- let newTarget = null;
- if (newIndex !== -1) {
- newTarget = this.stops[newIndex];
- }
+ const clipped = this.index === newIndex;
/*
* If user presses n on the last diff chunk, show a toast informing user
* that pressing n again will navigate them to next unreviewed file.
* If click happens within the time limit, then navigate to next file
*/
- if (navigateToNextFile && this.index === newIndex && this.isAtEnd()) {
+ if (navigateToNextFile && clipped && this.isAtEnd()) {
if (
this._lastDisplayedNavigateToNextFileToast &&
Date.now() - this._lastDisplayedNavigateToNextFileToast <=
@@ -311,7 +322,7 @@
bubbles: true,
})
);
- return;
+ return CursorMoveResult.CLIPPED;
}
this._lastDisplayedNavigateToNextFileToast = Date.now();
this.dispatchEvent(
@@ -323,14 +334,14 @@
bubbles: true,
})
);
- return;
+ return CursorMoveResult.CLIPPED;
}
this.index = newIndex;
this.target = newTarget as HTMLElement;
if (!newTarget) {
- return;
+ return CursorMoveResult.NO_STOPS;
}
if (getTargetHeight) {
@@ -344,6 +355,8 @@
}
this._decorateTarget();
+
+ return clipped ? CursorMoveResult.CLIPPED : CursorMoveResult.MOVED;
}
_decorateTarget() {
diff --git a/polygerrit-ui/app/elements/shared/gr-cursor-manager/gr-cursor-manager_test.js b/polygerrit-ui/app/elements/shared/gr-cursor-manager/gr-cursor-manager_test.js
index bc07d84..5c0bb42 100644
--- a/polygerrit-ui/app/elements/shared/gr-cursor-manager/gr-cursor-manager_test.js
+++ b/polygerrit-ui/app/elements/shared/gr-cursor-manager/gr-cursor-manager_test.js
@@ -18,6 +18,7 @@
import '../../../test/common-test-setup-karma.js';
import './gr-cursor-manager.js';
import {html} from '@polymer/polymer/lib/utils/html-tag.js';
+import {CursorMoveResult} from './gr-cursor-manager.js';
const basicTestFixutre = fixtureFromTemplate(html`
<gr-cursor-manager cursor-target-class="targeted"></gr-cursor-manager>
@@ -66,10 +67,11 @@
assert.isFalse(element.isAtEnd());
// Progress the cursor.
- element.next();
+ let result = element.next();
// Confirm that the next stop is selected and that the previous stop is
// unselected.
+ assert.equal(result, CursorMoveResult.MOVED);
assert.equal(element.index, 3);
assert.equal(element.target, list.children[3]);
assert.isTrue(element.isAtEnd());
@@ -77,19 +79,23 @@
assert.isTrue(list.children[3].classList.contains('targeted'));
// Progress the cursor.
- element.next();
+ result = element.next();
// We should still be at the end.
+ assert.equal(result, CursorMoveResult.CLIPPED);
assert.equal(element.index, 3);
assert.equal(element.target, list.children[3]);
assert.isTrue(element.isAtEnd());
// Wind the cursor all the way back to the first stop.
- element.previous();
- element.previous();
- element.previous();
+ result = element.previous();
+ assert.equal(result, CursorMoveResult.MOVED);
+ result = element.previous();
+ assert.equal(result, CursorMoveResult.MOVED);
+ result = element.previous();
+ assert.equal(result, CursorMoveResult.MOVED);
- // The element state should reflect the end of the list.
+ // The element state should reflect the start of the list.
assert.equal(element.index, 0);
assert.equal(element.target, list.children[0]);
assert.isTrue(element.isAtStart());
@@ -113,8 +119,9 @@
test('next() goes to first element when no cursor is set', () => {
element.stops = list.querySelectorAll('li');
- element.next();
+ const result = element.next();
+ assert.equal(result, CursorMoveResult.MOVED);
assert.equal(element.index, 0);
assert.equal(element.target, list.children[0]);
assert.isTrue(list.children[0].classList.contains('targeted'));
@@ -122,10 +129,23 @@
assert.isFalse(element.isAtEnd());
});
- test('next() goes to first element when no cursor is set', () => {
- element.stops = list.querySelectorAll('li');
- element.previous();
+ test('next() resets the cursor when there are no stops', () => {
+ element.stops = [];
+ const result = element.next();
+ assert.equal(result, CursorMoveResult.NO_STOPS);
+ assert.equal(element.index, -1);
+ assert.isNotOk(element.target);
+ assert.isFalse(list.children[1].classList.contains('targeted'));
+ assert.isFalse(element.isAtStart());
+ assert.isFalse(element.isAtEnd());
+ });
+
+ test('previous() goes to last element when no cursor is set', () => {
+ element.stops = list.querySelectorAll('li');
+ const result = element.previous();
+
+ assert.equal(result, CursorMoveResult.MOVED);
const lastIndex = list.children.length - 1;
assert.equal(element.index, lastIndex);
assert.equal(element.target, list.children[lastIndex]);
@@ -134,6 +154,18 @@
assert.isTrue(element.isAtEnd());
});
+ test('previous() resets the cursor when there are no stops', () => {
+ element.stops = [];
+ const result = element.previous();
+
+ assert.equal(result, CursorMoveResult.NO_STOPS);
+ assert.equal(element.index, -1);
+ assert.isNotOk(element.target);
+ assert.isFalse(list.children[1].classList.contains('targeted'));
+ assert.isFalse(element.isAtStart());
+ assert.isFalse(element.isAtEnd());
+ });
+
test('_moveCursor', () => {
// Initialize the cursor with its stops.
element.stops = list.querySelectorAll('li');
