Fix search addons: Fix problem of skipping some results and incorrect text selection after resize window

[ntchjb]

When function _findInLine is run in reverse mode (find previous), it skipped the results that is wrapped, and also skipped the line that has isWrapped === true although there are other results in the line. When resizing the window and search again, it highlights incorrect position.

So, for skipping wrapped result problem, I added a condition to check whether start column index is in the range or not. If not, then lastIndexOf function should begin matching at the end of the string.

For skipping wrapped line, I think the part of code inside _findInLine that did a return when found properties isWrapped === true can be removed because we should also scan other results in wrapped line.

For resizing window, I saw that the value at this._terminal._core.buffer.lines.get(y).length didn't change when the window was resized, but for this._terminal.cols, the value is changed, so I decided to use this._terminal.cols to match with searching range.

[Tyriar]

Testing ti locally on master and your branch it doesn't seem to work on either? 😕

The idea behind this isWrapped check is to ignore all rows flagged as wrapped and only take the first row (which always has isWrapped = false

[ntchjb]

Oh sorry, I misunderstand the idea. I will put back the return block and try to fix else where. The problem is that if the wrapped line has multiple matches, the search result will highlight only one result and skipped others then go to next line.

[ntchjb]

I fixed the problem by controlling index of col before passing to _findInLine. When the function scans on unwrapped line, it should have the value of col high enough so that the function can scan through all wrapped lines below the unwrapped line.

[ntchjb]

For the latest commit, I try to modify searching function to support wide characters and combined characters by mapping between buffer index and string index. I'm not sure if stringIndexToBufferIndex and bufferIndexToStringIndex method should be moved to Buffer.ts or not. For stringIndexToBufferIndex method, it is different from the original in Buffer.ts that the method also counts empty cells as whitespace. So that for the following test case:

term.core.write('Hello World\r\ntest\n123....hello');
const hello2 = term.searchHelper.findInLine('Hello', 2);

return the result as {col: 11, row: 2, term: 'Hello'}.

I'm not sure why \n in write function results in several empty cells before 123....hello string.

[ntchjb]

This PR might relate to #1686 and #1801

Edit: A PR may be created separately for wide characters support feature.

[Tyriar]

For the latest commit, I try to modify searching function to support wide characters and combined characters by mapping between buffer index and string index.

It might be better to revert the latest commit and do that in another one as this was so close. I'm happy to merge if we revert that, provided that "- Fix bug when search selection skipped some result" in the latest commit message is not something we need to worry about?

Also on the actual solution, that's way too much code to copy over to the addon as it will quickly get out of sync.

I'm not sure why \n in write function results in several empty cells before 123....hello string.

That's happening because you did \n which moves the cursor to the following line, but you missed the \r which moves the cursor to the start of the line.

[ntchjb]

OK, I reverted the latest commit to the previous commit. However, at the latest commit, there is a fix that relates to the previous commit, so I make an additional commit to complete the fix.

[Tyriar]

It looks like this change is grabbing the cumulativeCols and calling _findInLine multiple times for a single wrapped row. This kind of goes against the design and _findInLines is meant to get the full unwrapped line and search it fully. I think the actual root cause is somewhere inside _findInLine and calling it multiple times on the same row is getting around it.

After this section searchStringLine is meant to contain the full wrapped line so we can search it in full:

xterm.js/src/addons/search/SearchHelper.ts

Lines 190 to 203 in 509ce5f

	if (this._terminal._core.buffer.lines.get(row).isWrapped) {
	return;
	}
	let stringLine = this._linesCache ? this._linesCache[row] : void 0;
	if (stringLine === void 0) {
	stringLine = this.translateBufferLineToStringWithWrap(row, true);
	if (this._linesCache) {
	this._linesCache[row] = stringLine;
	}
	}
	const searchTerm = searchOptions.caseSensitive ? term : term.toLowerCase();
	const searchStringLine = searchOptions.caseSensitive ? stringLine : stringLine.toLowerCase();

[ntchjb]

Oh, you were right. _findInLine should return the result at the first time, otherwise, go to the next unwrapped line.

[ntchjb]

I reverted fineNext mechanism to the old one, firstly, scan for the first unwrapped line and run _findInLine. If the result not found, then scan for the next unwrapped line and run _findInLine. If the result still not found, then try to scan from the first unwrapped row in the buffer.

[Tyriar]

I just tried testing this again and it still seems broken, to test I'm typing a bunch of characters and then trying to find previous a term on a wrapped line:

The circled "df" doesn't get found though.

[ntchjb]

I'm sorry, but I couldn't reproduce the problem. I tested in demo server using yarn start. The search results found df keywords in both forward and backward which covers all matching words. Here is my snapshot.

I used column with width cols 87 and rows 26. Here is the text results

bash-3.2$ echo '
> lkmasdjasdfknajlsdlnkafdslnkasdflnjalsdfnkjdasfnlkjadsfnljksdajklsadfllfsdakafdsfadsnlkadf
> '

lkmasdjasdfknajlsdlnkafdslnkasdflnjalsdfnkjdasfnlkjadsfnljksdajklsadfllfsdakafdsfadsnlkadf

bash-3.2$ 

df words has been found 10 times in total.

I run the demo server on macOS 10.14 with Google Chrome 72

[ntchjb]

If you mean that, after typed a bunch of the string and ran findPrevious search without any selection at the first time, and it didn't select matching string at the end of the first line, then, I changed the code in findPrevious to start scanning at the last row first if there is no text selection.

[Tyriar]

Thanks, seems to work great now! I fixed a minor issue which started findPrevious at the top of the buffer rather than where the viewport was.