Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unify trim/delete/insert events as 'move' events. #1948

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 16 additions & 30 deletions src/Buffer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
import { IMarker } from 'xterm';
import { BufferLine } from './BufferLine';
import { reflowLargerApplyNewLayout, reflowLargerCreateNewLayout, reflowLargerGetLinesToRemove, reflowSmallerGetNewLineLengths } from './BufferReflow';
import { CircularList, IDeleteEvent, IInsertEvent } from './common/CircularList';
import { CircularList, IMoveEvent } from './common/CircularList';
import { EventEmitter } from './common/EventEmitter';
import { DEFAULT_COLOR } from './renderer/atlas/Types';
import { BufferIndex, CharData, IBuffer, IBufferLine, IBufferStringIterator, IBufferStringIteratorResult, ITerminal } from './Types';
Expand Down Expand Up @@ -385,7 +385,7 @@ export class Buffer implements IBuffer {
if (toInsert.length > 0) {
// Record buffer insert events and then play them back backwards so that the indexes are
// correct
const insertEvents: IInsertEvent[] = [];
const insertEvents: IMoveEvent[] = [];

// Record original lines so they don't get overridden when we rearrange the list
const originalLines: BufferLine[] = [];
Expand All @@ -409,9 +409,9 @@ export class Buffer implements IBuffer {

// Create insert events for later
insertEvents.push({
index: originalLineIndex + 1,
amount: nextToInsert.newLines.length
} as IInsertEvent);
start: originalLineIndex + 1,
amount: - nextToInsert.newLines.length
} as IMoveEvent);

countInsertedSoFar += nextToInsert.newLines.length;
nextToInsert = toInsert[++nextToInsertIndex];
Expand All @@ -423,13 +423,14 @@ export class Buffer implements IBuffer {
// Update markers
let insertCountEmitted = 0;
for (let i = insertEvents.length - 1; i >= 0; i--) {
insertEvents[i].index += insertCountEmitted;
this.lines.emit('insert', insertEvents[i]);
insertEvents[i].start += insertCountEmitted;
this.lines.emit('move', insertEvents[i]);
insertCountEmitted += insertEvents[i].amount;
}
const amountToTrim = Math.max(0, originalLinesLength + countToInsert - this.lines.maxLength);
if (amountToTrim > 0) {
this.lines.emitMayRemoveListeners('trim', amountToTrim);
this.lines.emitMayRemoveListeners('trim', amountToTrim); // deprecated
this.lines.emitMayRemoveListeners('move', { start: 0, amount: amountToTrim});
}
}
}
Expand Down Expand Up @@ -549,29 +550,14 @@ export class Buffer implements IBuffer {
public addMarker(y: number): Marker {
const marker = new Marker(y);
this.markers.push(marker);
marker.register(this.lines.addDisposableListener('trim', amount => {
marker.line -= amount;
// The marker should be disposed when the line is trimmed from the buffer
if (marker.line < 0) {
marker.dispose();
}
}));
marker.register(this.lines.addDisposableListener('insert', (event: IInsertEvent) => {
if (marker.line >= event.index) {
marker.line += event.amount;
}
}));
marker.register(this.lines.addDisposableListener('delete', (event: IDeleteEvent) => {
// Delete the marker if it's within the range
if (marker.line >= event.index && marker.line < event.index + event.amount) {
marker.dispose();
}

// Shift the marker if it's after the deleted range
if (marker.line > event.index) {
marker.register(this.lines.addDisposableListener('move', (event: IMoveEvent) => {
if (marker.line >= event.start && (! event.end || marker.line < event.end)) {
marker.line -= event.amount;
}
}));
if (marker.line < event.start ||
(event.end && marker.line >= event.end)) {
marker.dispose();
}
}}));
marker.register(marker.addDisposableListener('dispose', () => this._removeMarker(marker)));
return marker;
}
Expand Down
8 changes: 4 additions & 4 deletions src/BufferReflow.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

import { FILL_CHAR_DATA } from './Buffer';
import { BufferLine } from './BufferLine';
import { CircularList, IDeleteEvent } from './common/CircularList';
import { CircularList, IMoveEvent } from './common/CircularList';
import { IBufferLine } from './Types';

export interface INewLayoutResult {
Expand Down Expand Up @@ -119,10 +119,10 @@ export function reflowLargerCreateNewLayout(lines: CircularList<IBufferLine>, to
const countToRemove = toRemove[++nextToRemoveIndex];

// Tell markers that there was a deletion
lines.emit('delete', {
index: i - countRemovedSoFar,
lines.emit('move', {
start: i - countRemovedSoFar,
amount: countToRemove
} as IDeleteEvent);
} as IMoveEvent);

i += countToRemove - 1;
countRemovedSoFar += countToRemove;
Expand Down
17 changes: 9 additions & 8 deletions src/SelectionManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@
*/

import { ITerminal, ISelectionManager, IBuffer, CharData, IBufferLine } from './Types';
import { XtermListener } from './common/Types';
import { XtermListener } from './common/Types'; // deprecated
import { IMoveEvent } from './common/CircularList';
import { MouseHelper } from './ui/MouseHelper';
import * as Browser from './core/Platform';
import { CharMeasure } from './ui/CharMeasure';
Expand Down Expand Up @@ -102,7 +103,7 @@ export class SelectionManager extends EventEmitter implements ISelectionManager

private _mouseMoveListener: EventListener;
private _mouseUpListener: EventListener;
private _trimListener: XtermListener;
private _moveListener: XtermListener;

private _mouseDownTimeStamp: number;

Expand Down Expand Up @@ -133,13 +134,13 @@ export class SelectionManager extends EventEmitter implements ISelectionManager
private _initListeners(): void {
this._mouseMoveListener = event => this._onMouseMove(<MouseEvent>event);
this._mouseUpListener = event => this._onMouseUp(<MouseEvent>event);
this._trimListener = (amount: number) => this._onTrim(amount);
this._moveListener = (event: IMoveEvent) => this._onMove(event);

this.initBuffersListeners();
}

public initBuffersListeners(): void {
this._terminal.buffer.lines.on('trim', this._trimListener);
this._terminal.buffer.lines.on('move', this._moveListener);
this._terminal.buffers.on('activate', e => this._onBufferActivate(e));
}

Expand Down Expand Up @@ -335,8 +336,8 @@ export class SelectionManager extends EventEmitter implements ISelectionManager
* Handle the buffer being trimmed, adjust the selection position.
* @param amount The amount the buffer is being trimmed.
*/
private _onTrim(amount: number): void {
const needsRefresh = this._model.onTrim(amount);
private _onMove(event: IMoveEvent): void {
const needsRefresh = this._model.onMove(event);
if (needsRefresh) {
this.refresh();
}
Expand Down Expand Up @@ -658,8 +659,8 @@ export class SelectionManager extends EventEmitter implements ISelectionManager
// reverseIndex) and delete in a splice is only ever used when the same
// number of elements was just added. Given this is could actually be
// beneficial to leave the selection as is for these cases.
e.inactiveBuffer.lines.off('trim', this._trimListener);
e.activeBuffer.lines.on('trim', this._trimListener);
e.inactiveBuffer.lines.off('move', this._moveListener);
e.activeBuffer.lines.on('move', this._moveListener);
}

/**
Expand Down
8 changes: 4 additions & 4 deletions src/SelectionModel.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,21 +63,21 @@ describe('SelectionManager', () => {
});
});

describe('onTrim', () => {
describe('onMove', () => {
it('should trim a portion of the selection when a part of it is trimmed', () => {
model.selectionStart = [0, 0];
model.selectionEnd = [10, 2];
model.onTrim(1);
model.onMove({start: 0, amount: 1});
assert.deepEqual(model.finalSelectionStart, [0, 0]);
assert.deepEqual(model.finalSelectionEnd, [10, 1]);
model.onTrim(1);
model.onMove({start: 0, amount: 1});
assert.deepEqual(model.finalSelectionStart, [0, 0]);
assert.deepEqual(model.finalSelectionEnd, [10, 0]);
});
it('should clear selection when it is trimmed in its entirety', () => {
model.selectionStart = [0, 0];
model.selectionEnd = [10, 0];
model.onTrim(1);
model.onMove({start: 0, amount: 1});
assert.deepEqual(model.finalSelectionStart, null);
assert.deepEqual(model.finalSelectionEnd, null);
});
Expand Down
13 changes: 8 additions & 5 deletions src/SelectionModel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
*/

import { ITerminal } from './Types';
import { IMoveEvent } from './common/CircularList';

/**
* Represents a selection within the buffer. This model only cares about column
Expand Down Expand Up @@ -112,13 +113,15 @@ export class SelectionModel {
* @param amount The amount the buffer is being trimmed.
* @return Whether a refresh is necessary.
*/
public onTrim(amount: number): boolean {
public onMove(event: IMoveEvent): boolean {
// Adjust the selection position based on the trimmed amount.
if (this.selectionStart) {
this.selectionStart[1] -= amount;
if (this.selectionStart && this.selectionStart[1] >= event.start
&& (! event.end || this.selectionStart[1] < event.end)) {
Copy link
Member

@Tyriar Tyriar Mar 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I think the current way is much more readable and explicit in explaining what is actually happening, the need to check for end? and handle it is one of the main reasons I think this.

Another good example:

// Where is it moving?
model.onMove({start: 0, amount: 1});

A negative amount is also a little confusing unless you go and read the definition of IMoveEvent.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thoughts @jerch?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gonna answer below to stay in the thread flow...

this.selectionStart[1] -= event.amount;
}
if (this.selectionEnd) {
this.selectionEnd[1] -= amount;
if (this.selectionEnd && this.selectionEnd[1] >= event.start
&& (! event.end || this.selectionEnd[1] < event.end)) {
this.selectionEnd[1] -= event.amount;
}

// The selection has moved off the buffer, clear it.
Expand Down
29 changes: 17 additions & 12 deletions src/common/CircularList.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,13 @@
import { EventEmitter } from './EventEmitter';
import { ICircularList } from './Types';

export interface IInsertEvent {
index: number;
amount: number;
}

export interface IDeleteEvent {
index: number;
export interface IMoveEvent {
/** Index of first line affected (including ybase). */
start: number;
/** Index of (line following the) last line affected.
* This is the old index (pre-insert/delete), and is exclusive. */
end?: number;
/** Number of lines deleted (if positive) or inserted (if negative). */
amount: number;
}

Expand Down Expand Up @@ -101,7 +101,8 @@ export class CircularList<T> extends EventEmitter implements ICircularList<T> {
this._array[this._getCyclicIndex(this._length)] = value;
if (this._length === this._maxLength) {
this._startIndex = ++this._startIndex % this._maxLength;
this.emitMayRemoveListeners('trim', 1);
this.emitMayRemoveListeners('move', { start: 0, amount: 1 });
this.emitMayRemoveListeners('trim', 1); // deprecated
} else {
this._length++;
}
Expand All @@ -117,7 +118,8 @@ export class CircularList<T> extends EventEmitter implements ICircularList<T> {
throw new Error('Can only recycle when the buffer is full');
}
this._startIndex = ++this._startIndex % this._maxLength;
this.emitMayRemoveListeners('trim', 1);
this.emitMayRemoveListeners('move', { start: 0, amount: 1 });
this.emitMayRemoveListeners('trim', 1); // deprecated
return this._array[this._getCyclicIndex(this._length - 1)]!;
}

Expand Down Expand Up @@ -167,7 +169,8 @@ export class CircularList<T> extends EventEmitter implements ICircularList<T> {
const countToTrim = (this._length + items.length) - this._maxLength;
this._startIndex += countToTrim;
this._length = this._maxLength;
this.emitMayRemoveListeners('trim', countToTrim);
this.emitMayRemoveListeners('move', { start: 0, amount: countToTrim });
this.emitMayRemoveListeners('trim', countToTrim); // deprecated
} else {
this._length += items.length;
}
Expand All @@ -183,7 +186,8 @@ export class CircularList<T> extends EventEmitter implements ICircularList<T> {
}
this._startIndex += count;
this._length -= count;
this.emitMayRemoveListeners('trim', count);
this.emitMayRemoveListeners('move', { start: 0, amount: count });
this.emitMayRemoveListeners('trim', count); // deprecated
}

public shiftElements(start: number, count: number, offset: number): void {
Expand All @@ -207,7 +211,8 @@ export class CircularList<T> extends EventEmitter implements ICircularList<T> {
while (this._length > this._maxLength) {
this._length--;
this._startIndex++;
this.emitMayRemoveListeners('trim', 1);
this.emitMayRemoveListeners('move', { start: 0, amount: 1 });
this.emitMayRemoveListeners('trim', 1); // deprecated
}
}
} else {
Expand Down
1 change: 1 addition & 0 deletions src/common/Types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,4 +34,5 @@ export interface ICircularList<T> extends IEventEmitter {
splice(start: number, deleteCount: number, ...items: T[]): void;
trimStart(count: number): void;
shiftElements(start: number, count: number, offset: number): void;
emitMayRemoveListeners(type: string, ...args: any[]): void;
}