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

Progress toward #1686: some desirable selection behaviour #2526

Closed
wants to merge 2 commits 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
171 changes: 171 additions & 0 deletions addons/xterm-addon-search/src/CharWidth.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,171 @@
/**
* Copyright (c) 2016 The xterm.js authors. All rights reserved.
* @license MIT
*/

import { fill } from './TypedArrayUtils';

export const wcwidth = (function(opts: {nul: number, control: number}): (ucs: number) => number {
Copy link
Member

@Tyriar Tyriar Nov 7, 2019

Choose a reason for hiding this comment

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

If we're going to need internal code in an addon we can actually reference it directly using the same trick as the webgl addon:

"paths": {
"common/*": [ "../../../src/common/*" ],
"browser/*": [ "../../../src/browser/*" ]
},
"strict": true
},
"include": [
"./**/*",
"../../../typings/xterm.d.ts"
],
"references": [
{ "path": "../../../src/common" },
{ "path": "../../../src/browser" }
]

alias: {
common: path.resolve('../../out/common'),
browser: path.resolve('../../out/browser')
}

import { IRenderService } from 'browser/services/Services';
import { IColorSet } from 'browser/Types';

Care needs to be used here as when webpack does its thing it will pull in all code from the file (and linked from that file), but CharWidth is a pretty standalone thing so it should be good.

// extracted from https://www.cl.cam.ac.uk/%7Emgk25/ucs/wcwidth.c
// combining characters
const COMBINING_BMP = [
[0x0300, 0x036F], [0x0483, 0x0486], [0x0488, 0x0489],
[0x0591, 0x05BD], [0x05BF, 0x05BF], [0x05C1, 0x05C2],
[0x05C4, 0x05C5], [0x05C7, 0x05C7], [0x0600, 0x0603],
[0x0610, 0x0615], [0x064B, 0x065E], [0x0670, 0x0670],
[0x06D6, 0x06E4], [0x06E7, 0x06E8], [0x06EA, 0x06ED],
[0x070F, 0x070F], [0x0711, 0x0711], [0x0730, 0x074A],
[0x07A6, 0x07B0], [0x07EB, 0x07F3], [0x0901, 0x0902],
[0x093C, 0x093C], [0x0941, 0x0948], [0x094D, 0x094D],
[0x0951, 0x0954], [0x0962, 0x0963], [0x0981, 0x0981],
[0x09BC, 0x09BC], [0x09C1, 0x09C4], [0x09CD, 0x09CD],
[0x09E2, 0x09E3], [0x0A01, 0x0A02], [0x0A3C, 0x0A3C],
[0x0A41, 0x0A42], [0x0A47, 0x0A48], [0x0A4B, 0x0A4D],
[0x0A70, 0x0A71], [0x0A81, 0x0A82], [0x0ABC, 0x0ABC],
[0x0AC1, 0x0AC5], [0x0AC7, 0x0AC8], [0x0ACD, 0x0ACD],
[0x0AE2, 0x0AE3], [0x0B01, 0x0B01], [0x0B3C, 0x0B3C],
[0x0B3F, 0x0B3F], [0x0B41, 0x0B43], [0x0B4D, 0x0B4D],
[0x0B56, 0x0B56], [0x0B82, 0x0B82], [0x0BC0, 0x0BC0],
[0x0BCD, 0x0BCD], [0x0C3E, 0x0C40], [0x0C46, 0x0C48],
[0x0C4A, 0x0C4D], [0x0C55, 0x0C56], [0x0CBC, 0x0CBC],
[0x0CBF, 0x0CBF], [0x0CC6, 0x0CC6], [0x0CCC, 0x0CCD],
[0x0CE2, 0x0CE3], [0x0D41, 0x0D43], [0x0D4D, 0x0D4D],
[0x0DCA, 0x0DCA], [0x0DD2, 0x0DD4], [0x0DD6, 0x0DD6],
[0x0E31, 0x0E31], [0x0E34, 0x0E3A], [0x0E47, 0x0E4E],
[0x0EB1, 0x0EB1], [0x0EB4, 0x0EB9], [0x0EBB, 0x0EBC],
[0x0EC8, 0x0ECD], [0x0F18, 0x0F19], [0x0F35, 0x0F35],
[0x0F37, 0x0F37], [0x0F39, 0x0F39], [0x0F71, 0x0F7E],
[0x0F80, 0x0F84], [0x0F86, 0x0F87], [0x0F90, 0x0F97],
[0x0F99, 0x0FBC], [0x0FC6, 0x0FC6], [0x102D, 0x1030],
[0x1032, 0x1032], [0x1036, 0x1037], [0x1039, 0x1039],
[0x1058, 0x1059], [0x1160, 0x11FF], [0x135F, 0x135F],
[0x1712, 0x1714], [0x1732, 0x1734], [0x1752, 0x1753],
[0x1772, 0x1773], [0x17B4, 0x17B5], [0x17B7, 0x17BD],
[0x17C6, 0x17C6], [0x17C9, 0x17D3], [0x17DD, 0x17DD],
[0x180B, 0x180D], [0x18A9, 0x18A9], [0x1920, 0x1922],
[0x1927, 0x1928], [0x1932, 0x1932], [0x1939, 0x193B],
[0x1A17, 0x1A18], [0x1B00, 0x1B03], [0x1B34, 0x1B34],
[0x1B36, 0x1B3A], [0x1B3C, 0x1B3C], [0x1B42, 0x1B42],
[0x1B6B, 0x1B73], [0x1DC0, 0x1DCA], [0x1DFE, 0x1DFF],
[0x200B, 0x200F], [0x202A, 0x202E], [0x2060, 0x2063],
[0x206A, 0x206F], [0x20D0, 0x20EF], [0x302A, 0x302F],
[0x3099, 0x309A], [0xA806, 0xA806], [0xA80B, 0xA80B],
[0xA825, 0xA826], [0xFB1E, 0xFB1E], [0xFE00, 0xFE0F],
[0xFE20, 0xFE23], [0xFEFF, 0xFEFF], [0xFFF9, 0xFFFB]
];
const COMBINING_HIGH = [
[0x10A01, 0x10A03], [0x10A05, 0x10A06], [0x10A0C, 0x10A0F],
[0x10A38, 0x10A3A], [0x10A3F, 0x10A3F], [0x1D167, 0x1D169],
[0x1D173, 0x1D182], [0x1D185, 0x1D18B], [0x1D1AA, 0x1D1AD],
[0x1D242, 0x1D244], [0xE0001, 0xE0001], [0xE0020, 0xE007F],
[0xE0100, 0xE01EF]
];
// binary search
function bisearch(ucs: number, data: number[][]): boolean {
let min = 0;
let max = data.length - 1;
let mid;
if (ucs < data[0][0] || ucs > data[max][1]) {
return false;
}
while (max >= min) {
mid = (min + max) >> 1;
if (ucs > data[mid][1]) {
min = mid + 1;
} else if (ucs < data[mid][0]) {
max = mid - 1;
} else {
return true;
}
}
return false;
}
function wcwidthHigh(ucs: number): 0 | 1 | 2 {
if (bisearch(ucs, COMBINING_HIGH)) {
return 0;
}
if ((ucs >= 0x20000 && ucs <= 0x2fffd) || (ucs >= 0x30000 && ucs <= 0x3fffd)) {
return 2;
}
return 1;
}
const control = opts.control | 0;

// create lookup table for BMP plane
const table = new Uint8Array(65536);
fill(table, 1);
table[0] = opts.nul;
// control chars
fill(table, opts.control, 1, 32);
fill(table, opts.control, 0x7f, 0xa0);

// apply wide char rules first
// wide chars
fill(table, 2, 0x1100, 0x1160);
table[0x2329] = 2;
table[0x232a] = 2;
fill(table, 2, 0x2e80, 0xa4d0);
table[0x303f] = 1; // wrongly in last line

fill(table, 2, 0xac00, 0xd7a4);
fill(table, 2, 0xf900, 0xfb00);
fill(table, 2, 0xfe10, 0xfe1a);
fill(table, 2, 0xfe30, 0xfe70);
fill(table, 2, 0xff00, 0xff61);
fill(table, 2, 0xffe0, 0xffe7);

// apply combining last to ensure we overwrite
// wrongly wide set chars:
// the original algo evals combining first and falls
// through to wide check so we simply do here the opposite
// combining 0
for (let r = 0; r < COMBINING_BMP.length; ++r) {
fill(table, 0, COMBINING_BMP[r][0], COMBINING_BMP[r][1] + 1);
}

return function (num: number): number {
if (num < 32) {
return control | 0;
}
if (num < 127) {
return 1;
}
if (num < 65536) {
return table[num];
}
// do a full search for high codepoints
return wcwidthHigh(num);
};
})({nul: 0, control: 0}); // configurable options

/**
* Get the terminal cell width for a string.
*/
export function getStringCellWidth(s: string): number {
let result = 0;
const length = s.length;
for (let i = 0; i < length; ++i) {
let code = s.charCodeAt(i);
// surrogate pair first
if (0xD800 <= code && code <= 0xDBFF) {
if (++i >= length) {
// this should not happen with strings retrieved from
// Buffer.translateToString as it converts from UTF-32
// and therefore always should contain the second part
// for any other string we still have to handle it somehow:
// simply treat the lonely surrogate first as a single char (UCS-2 behavior)
return result + wcwidth(code);
}
const second = s.charCodeAt(i);
// convert surrogate pair to high codepoint only for valid second part (UTF-16)
// otherwise treat them independently (UCS-2 behavior)
if (0xDC00 <= second && second <= 0xDFFF) {
code = (code - 0xD800) * 0x400 + second - 0xDC00 + 0x10000;
} else {
result += wcwidth(second);
}
}
result += wcwidth(code);
}
return result;
}
23 changes: 12 additions & 11 deletions addons/xterm-addon-search/src/SearchAddon.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
*/

import { Terminal, IDisposable, ITerminalAddon, ISelectionPosition } from 'xterm';
import { getStringCellWidth } from './CharWidth'

export interface ISearchOptions {
regex?: boolean;
Expand Down Expand Up @@ -52,7 +53,7 @@ export class SearchAddon implements ITerminalAddon {
throw new Error('Cannot use addon until it has been loaded');
}

if (!term || term.length === 0) {
if (!term || getStringCellWidth(term) === 0) {
Copy link
Member

Choose a reason for hiding this comment

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

I think this will make searching for diacritical characters impossible? (like find all "`" in terminal)

I would remove the second condition completely, not sure why it was placed there in the first place (!term already catches term.length === 0 anyway).

this._terminal.clearSelection();
return false;
}
Expand Down Expand Up @@ -116,7 +117,7 @@ export class SearchAddon implements ITerminalAddon {
throw new Error('Cannot use addon until it has been loaded');
}

if (!term || term.length === 0) {
if (!term || getStringCellWidth(term) === 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

this._terminal.clearSelection();
return false;
}
Expand Down Expand Up @@ -211,7 +212,7 @@ export class SearchAddon implements ITerminalAddon {
*/
private _isWholeWord(searchIndex: number, line: string, term: string): boolean {
return (((searchIndex === 0) || (NON_WORD_CHARACTERS.indexOf(line[searchIndex - 1]) !== -1)) &&
(((searchIndex + term.length) === line.length) || (NON_WORD_CHARACTERS.indexOf(line[searchIndex + term.length]) !== -1)));
(((searchIndex + getStringCellWidth(term)) === line.length) || (NON_WORD_CHARACTERS.indexOf(line[searchIndex + getStringCellWidth(term)]) !== -1)));
Copy link
Member

Choose a reason for hiding this comment

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

I think term.length was right here since it evals data in the line string (thus the string index should be used). The wcwidth correction is only needed for buffer offset/index calculation.

}

/**
Expand Down Expand Up @@ -251,21 +252,21 @@ export class SearchAddon implements ITerminalAddon {
if (isReverseSearch) {
// This loop will get the resultIndex of the _last_ regex match in the range 0..col
while (foundTerm = searchRegex.exec(searchStringLine.slice(0, col))) {
resultIndex = searchRegex.lastIndex - foundTerm[0].length;
resultIndex = searchRegex.lastIndex - getStringCellWidth(foundTerm[0]);
term = foundTerm[0];
searchRegex.lastIndex -= (term.length - 1);
searchRegex.lastIndex -= (getStringCellWidth(term) - 1);
Copy link
Member

Choose a reason for hiding this comment

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

Same problem here - the regex operates on string data, thus go with the string index/length.

}
} else {
foundTerm = searchRegex.exec(searchStringLine.slice(col));
if (foundTerm && foundTerm[0].length > 0) {
resultIndex = col + (searchRegex.lastIndex - foundTerm[0].length);
if (foundTerm && getStringCellWidth(foundTerm[0]) > 0) {
resultIndex = col + (searchRegex.lastIndex - getStringCellWidth(foundTerm[0]));
Copy link
Member

Choose a reason for hiding this comment

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

Imho this does not work at all this way - it mixes cell offsets (col) with string offsets into resultIndex. Note that cell offsets must contain the wcwidth correction, while string offsets should never.

Possible fix:

  • do the regexp index handling/matching beforehand
  • if a match was found, take the match index and do a getStringCellWidth(stringLine.slice(0, matchIndex)) - this gives the correct cell offset of the start character
  • add getStringCellWidth(term) to that - cell offset of the end character + 1
  • do a divmod correction for both values to translate them into actual col/row values (col = startOffset % terminal.cols, row += startOffset / terminal.cols)

term = foundTerm[0];
}
}
} else {
if (isReverseSearch) {
if (col - searchTerm.length >= 0) {
resultIndex = searchStringLine.lastIndexOf(searchTerm, col - searchTerm.length);
if (col - getStringCellWidth(searchTerm) >= 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Same here but in opposite direction.

resultIndex = searchStringLine.lastIndexOf(searchTerm, col - getStringCellWidth(searchTerm));
}
} else {
resultIndex = searchStringLine.indexOf(searchTerm, col);
Expand All @@ -292,7 +293,7 @@ export class SearchAddon implements ITerminalAddon {
}
// Adjust the searchIndex to normalize emoji into single chars
const char = cell.char;
if (char.length > 1) {
if (char.length == 2 && getStringCellWidth(char) == 2) {
resultIndex -= char.length - 1;
Comment on lines +296 to 297
Copy link
Member

Choose a reason for hiding this comment

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

This code does not make any sense to me. It definitely does not (only) do what the comment says. Again resultIndex is involved in this, I think the whole block from line 288 on needs a rewrite introduced by the needed rewrites above.

}
// Adjust the searchIndex for empty characters following wide unicode
Expand Down Expand Up @@ -349,7 +350,7 @@ export class SearchAddon implements ITerminalAddon {
terminal.clearSelection();
return false;
}
terminal.select(result.col, result.row, result.term.length);
terminal.select(result.col, result.row, getStringCellWidth(result.term));
Copy link
Member

Choose a reason for hiding this comment

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

Yepp here getStringCellWidth is needed. Still result.col and result.row may contain wrong value due to the cell and string offset mixing above.
Also note, that you might have to do the divmod correction on the end position here (if the term happens to wrap at the rightmost column).

// If it is not in the viewport then we scroll else it just gets selected
if (result.row >= (terminal.buffer.viewportY + terminal.rows) || result.row < terminal.buffer.viewportY) {
let scroll = result.row - terminal.buffer.viewportY;
Expand Down
52 changes: 52 additions & 0 deletions addons/xterm-addon-search/src/TypedArrayUtils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
/**
* Copyright (c) 2018 The xterm.js authors. All rights reserved.
* @license MIT
*/

export type TypedArray = Uint8Array | Uint16Array | Uint32Array | Uint8ClampedArray
| Int8Array | Int16Array | Int32Array
| Float32Array | Float64Array;


/**
* polyfill for TypedArray.fill
* This is needed to support .fill in all safari versions and IE 11.
*/
export function fill<T extends TypedArray>(array: T, value: number, start?: number, end?: number): T {
// all modern engines that support .fill
if (array.fill) {
return array.fill(value, start, end) as T;
}
return fillFallback(array, value, start, end);
}

export function fillFallback<T extends TypedArray>(array: T, value: number, start: number = 0, end: number = array.length): T {
// safari and IE 11
// since IE 11 does not support Array.prototype.fill either
// we cannot use the suggested polyfill from MDN
// instead we simply fall back to looping
if (start >= array.length) {
return array;
}
start = (array.length + start) % array.length;
if (end >= array.length) {
end = array.length;
} else {
end = (array.length + end) % array.length;
}
for (let i = start; i < end; ++i) {
array[i] = value;
}
return array;
}

/**
* Concat two typed arrays `a` and `b`.
* Returns a new typed array.
*/
export function concat<T extends TypedArray>(a: T, b: T): T {
const result = new (a.constructor as any)(a.length + b.length);
result.set(a);
result.set(b, a.length);
return result;
}
13 changes: 7 additions & 6 deletions src/browser/services/SelectionService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import { ICharSizeService, IMouseService, ISelectionService } from 'browser/serv
import { IBufferService, IOptionsService, ICoreService } from 'common/services/Services';
import { getCoordsRelativeToElement } from 'browser/input/Mouse';
import { moveToCellSequence } from 'browser/input/MoveToCell';
import { getStringCellWidth } from 'common/CharWidth'

/**
* The number of pixels the mouse needs to be above or below the viewport in
Expand Down Expand Up @@ -263,7 +264,7 @@ export class SelectionService implements ISelectionService {
// we need to update the selection for middle click to paste selection.
if (Browser.isLinux && isLinuxMouseSelection) {
const selectionText = this.selectionText;
if (selectionText.length) {
if (getStringCellWidth(selectionText)) {
this._onLinuxMouseSelection.fire(this.selectionText);
}
}
Expand Down Expand Up @@ -663,7 +664,7 @@ export class SelectionService implements ISelectionService {

this._removeMouseDownListeners();

if (this.selectionText.length <= 1 && timeElapsed < ALT_CLICK_MOVE_CURSOR_TIME) {
if (getStringCellWidth(this.selectionText) <= 1 && timeElapsed < ALT_CLICK_MOVE_CURSOR_TIME) {
if (event.altKey && this._bufferService.buffer.ybase === this._bufferService.buffer.ydisp) {
const coordinates = this._mouseService.getCoords(
event,
Expand Down Expand Up @@ -702,7 +703,7 @@ export class SelectionService implements ISelectionService {
private _convertViewportColToCharacterIndex(bufferLine: IBufferLine, coords: [number, number]): number {
let charIndex = coords[0];
for (let i = 0; coords[0] >= i; i++) {
const length = bufferLine.loadCell(i, this._workCell).getChars().length;
const length = getStringCellWidth(bufferLine.loadCell(i, this._workCell).getChars());
if (this._workCell.getWidth() === 0) {
// Wide characters aren't included in the line string so decrement the
// index so the index is back on the wide character.
Expand Down Expand Up @@ -782,7 +783,7 @@ export class SelectionService implements ISelectionService {
}

// Adjust the end index for characters whose length are > 1 (emojis)
const length = bufferLine.getString(endCol).length;
const length = getStringCellWidth(bufferLine.getString(endCol));
if (length > 1) {
rightLongCharOffset += length - 1;
endIndex += length - 1;
Expand All @@ -791,7 +792,7 @@ export class SelectionService implements ISelectionService {
// Expand the string in both directions until a space is hit
while (startCol > 0 && startIndex > 0 && !this._isCharWordSeparator(bufferLine.loadCell(startCol - 1, this._workCell))) {
bufferLine.loadCell(startCol - 1, this._workCell);
const length = this._workCell.getChars().length;
const length = getStringCellWidth(this._workCell.getChars());
if (this._workCell.getWidth() === 0) {
// If the next character is a wide char, record it and skip the column
leftWideCharCount++;
Expand All @@ -807,7 +808,7 @@ export class SelectionService implements ISelectionService {
}
while (endCol < bufferLine.length && endIndex + 1 < line.length && !this._isCharWordSeparator(bufferLine.loadCell(endCol + 1, this._workCell))) {
bufferLine.loadCell(endCol + 1, this._workCell);
const length = this._workCell.getChars().length;
const length = getStringCellWidth(this._workCell.getChars());
if (this._workCell.getWidth() === 2) {
// If the next character is a wide char, record it and skip the column
rightWideCharCount++;
Expand Down