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

Open
wants to merge 2 commits into
base: master
from

Conversation

@Silvyre
Copy link

Silvyre commented Oct 31, 2019

Some progress was made towards #1686:

  • Error-prone calls to String.length were replaced with calls to wcwidth.
    • (Note: I made a copy of common/CharWidth.ts within addons/xterm-addon-search/, as helper functions in src/common, e.g. wcwidth, cannot currently be imported into addons; I will open an issue to address this.)
  • Selections of full-width and diacritical characters are better now, but still require more work.
    • Selection of "full words" of diacritical and fullwidth characters now appear to fully function.
      • e.g. searching for ééé or ¥¥¥ after inserting echo -en 'combining: ééé\nfullwidth: ¥¥¥\n' into the demo now works well.
    • Selection of "partial words" of such characters is improved, but requires further tweaking. See: #1686 (comment)
Silvyre added 2 commits Oct 31, 2019

import { fill } from './TypedArrayUtils';

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

This comment has been minimized.

Copy link
@Tyriar

Tyriar Nov 7, 2019

Member

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.

@Tyriar Tyriar requested a review from jerch Nov 7, 2019
@Tyriar Tyriar added this to the 4.3.0 milestone Nov 7, 2019
Copy link
Member

jerch left a comment

❤️ Many thx! There are still some bugs in the code, esp. the mixing of string indices with cell indices will not work this way, see my remarks below. Lemme know if you need some help fixing it (the cell vs string offset separation can get confusing)

@@ -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) {

This comment has been minimized.

Copy link
@jerch

jerch Nov 8, 2019

Member

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).

@@ -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) {

This comment has been minimized.

Copy link
@jerch

jerch Nov 8, 2019

Member

Same as above.

@@ -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)));

This comment has been minimized.

Copy link
@jerch

jerch Nov 8, 2019

Member

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.

term = foundTerm[0];
searchRegex.lastIndex -= (term.length - 1);
searchRegex.lastIndex -= (getStringCellWidth(term) - 1);

This comment has been minimized.

Copy link
@jerch

jerch Nov 8, 2019

Member

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

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]));

This comment has been minimized.

Copy link
@jerch

jerch Nov 8, 2019

Member

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) {

This comment has been minimized.

Copy link
@jerch

jerch Nov 8, 2019

Member

Same here but in opposite direction.

if (char.length == 2 && getStringCellWidth(char) == 2) {
resultIndex -= char.length - 1;
Comment on lines +296 to 297

This comment has been minimized.

Copy link
@jerch

jerch Nov 8, 2019

Member

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.

@@ -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));

This comment has been minimized.

Copy link
@jerch

jerch Nov 8, 2019

Member

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).

@Silvyre

This comment has been minimized.

Copy link
Author

Silvyre commented Nov 8, 2019

Thank you so much for the review, @Tyriar @jerch!

I plan to find some time to address your comments in the following week!

@Tyriar Tyriar removed this from the 4.3.0 milestone Dec 5, 2019
@jerch

This comment has been minimized.

Copy link
Member

jerch commented Dec 6, 2019

@Silvyre Any progress on your side? Plz drop me a note if you need some help.

@Silvyre

This comment has been minimized.

Copy link
Author

Silvyre commented Dec 7, 2019

@Silvyre Any progress on your side? Plz drop me a note if you need some help.

Really appreciately the offer for help, and will definitely will take you up on it. I am a bit apprehensive of making further changes, as I am still not very confident in my understanding of cell and string offsetting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.