Finding multiple instances on the same line #1763

noamyogev84 · 2018-10-19T21:13:55Z

Description of change

Implemented search next/previous starting at current selection.
Added ISearchIndex to support search from specific row and column in findInLine
Added unit tests to search.test.ts

Tyriar · 2018-10-20T20:05:09Z

src/addons/search/Interfaces.ts

  regex?: boolean;
  wholeWord?: boolean;
  caseSensitive?: boolean;
+  matchMultiple?: boolean;


I think this should just maybe always be on?

OK, removing from ISearchOptions.

Tyriar · 2018-10-20T20:06:01Z

src/addons/search/SearchHelper.ts

+   * @param searchOptions search options,
+   * @return An array of matches or first match.
+   */
+  private _getNextMatch(term: string, searchString: string, row: number, searchOptions: ISearchOptions): ISearchResult {


Instead of reimplementing a bunch of the search stuff, could we start the search at the end of the current selection? Instead of the current master behavior which is to start the search on the line following the selection.

Not so sure about this one.

Instead of the current master behavior which is to start the search on the line following the selection

The part that starts the search is this -

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

Lines 41 to 43 in fc8640a

// Search from ydisp + 1 to end

for (let y = startRow + 1; y < this._terminal._core.buffer.ybase + this._terminal.rows; y++) {

result = this._findInLine(term, y, searchOptions);

I think i'll start by changing this part. then I'll have to somehow ignore the prefix of the line (what comes before the current selection).
@Tyriar , Is this the right direction?

Sorry about the huge delay, I was on vacation. Yes this is the right approach 👍

Tyriar · 2018-10-20T20:06:51Z

src/addons/search/search.test.ts

+      const find4 = term.searchHelper.findInLine('aa', 1, searchOptions);
+      const find5 = term.searchHelper.findInLine('aa', 1, searchOptions);
+      expect(find0).eql({col: 0, row: 0, term: 'aa'});
+      expect(find1).eql({col: 1, row: 0, term: 'aa'});


My expectation is for the second search to give col: 9. Another example:

Searching aaaa first would highlight the first two, searching again would highlight the last 2.

OK, removing the "overlapping term" implementation.

start search from current selection. add unit tests.

noamyogev84 · 2018-11-30T18:58:04Z

Hi @Tyriar . I'll appreciate if you can review my PR. some concerns:

Match by regex is a bit tricky. I'm pretty sure this is the nastiest part of the implementation (not very efficient) hope to hear your thoughts.
Added a new wrapper function findFromIndex to search.test.ts. but I should probably change all other tests to use findInLine in its new version. what do you think?

Thanks

Tyriar · 2018-12-07T16:26:09Z

@noamyogev84 btw it's much easier to review with GitHub when you don't rebase as I can't review just the new changes now.

Tyriar · 2018-12-07T16:26:51Z

src/addons/search/Interfaces.ts

  regex?: boolean;
  wholeWord?: boolean;
  caseSensitive?: boolean;
+  reverseSearch?: boolean;


Why is this being added? Seems to be complicating the changes an awful lot and there isn't a request for such a feature?

@Tyriar Hi...
I removed the reverseSearch option from ISearchOptions. but I have to somehow provide this option to findInLine. don't you agree?

I'm a little confused, we don't want to do any "reverse" searching afaict, say the selection is at (5,5), we want to:

Do a search from (5,5) to the end of the line

Search all following lines until the end

Search each line from the start to (0,5)

Search from (0,5) to (4,5)

When a result is hit, we want to do a forward search on the same line from the end of the result.

That's exactly what i do when findNext is called. but what about findPrevious?
What is your expectation if let's say there's a selection on (5,5) and the user is clicking on the find previous button?
I guess what i'm saying is that now, I need to be able to find multiple matches in the same line going backwards, and that logic is within findInLine.

Sorry for the confusion here.... :)

Oh you're totally right, I agree wholeheartedly with this now 😄:

I removed the reverseSearch option from ISearchOptions. but I have to somehow provide this option to findInLine. don't you agree?

Glad to hear that!
So assuming you prefer the option where ISearchOptions doesn't contain reverseSearch, and we pass it separatly to findInLine, Are there any more changes required? I'll appreciate your review.

@Tyriar pinging you once more. :)
Do you think we can move forward with this? any changes required?

add isReverseSearch argument to findInLine modify unit tests

Tyriar

Thanks for your patience with this, finally got around to looking at it again and merging in the latest changes (some big conflicts with incremental search being added).

Tyriar · 2018-12-26T23:53:07Z

src/addons/search/SearchHelper.ts

-      if (result) {
-        break;
+    // Search startRow
+    result = this._findInLine(term, { row: startRow, col: startCol }, searchOptions);


I'll make a change to remove the ISearchIndex object and just pass in the raw values. The reason being that a search over 10000 lines would create 10000 objects that basically immediately get discarded, so it's not worth the added niceness of named params.

Tyriar · 2018-12-27T00:21:22Z

@noamyogev84 thanks again 😃, pulling into VS Code here microsoft/vscode#65711

noamyogev84 · 2018-12-27T20:56:46Z

@Tyriar it was a great experience! Thanks.

Tyriar requested changes Oct 20, 2018

View reviewed changes

Tyriar added the work-in-progress Do not merge label Nov 24, 2018

implement find multiple matches in line.

376c790

start search from current selection. add unit tests.

noamyogev84 force-pushed the issue_1654 branch from 7cd6e8a to 376c790 Compare November 30, 2018 18:47

Merge branch 'master' into issue_1654

38adfcd

Tyriar requested changes Dec 7, 2018

View reviewed changes

remove reverseSearch from ISearchOptions

278d696

add isReverseSearch argument to findInLine modify unit tests

nojvek mentioned this pull request Dec 11, 2018

Search refactor proposal #1833

Closed

noamyogev84 and others added 5 commits December 12, 2018 11:59

Merge branch 'master' into issue_1654

e00c7d5

Merge remote-tracking branch 'origin/master' into pr/noamyogev84/1763

bb97325

Fix incremental search

204e42a

Get multiple matches working after incremental changes

76302e2

Fix previous search, remove incremental previous search

e0d535e

Tyriar approved these changes Dec 26, 2018

View reviewed changes

Tyriar added this to the 3.10.0 milestone Dec 26, 2018

Tyriar added 2 commits December 26, 2018 16:03

Remove ISearchIndex object

1a1f7e9

Comment how the regex reverse search while works

d698faa

Tyriar removed the work-in-progress Do not merge label Dec 27, 2018

Tyriar merged commit b48f910 into xtermjs:master Dec 27, 2018

	// Search from ydisp + 1 to end
	for (let y = startRow + 1; y < this._terminal._core.buffer.ybase + this._terminal.rows; y++) {
	result = this._findInLine(term, y, searchOptions);

Finding multiple instances on the same line #1763

Finding multiple instances on the same line #1763

Uh oh!

Conversation

noamyogev84 commented Oct 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noamyogev84 commented Nov 30, 2018

Uh oh!

Tyriar commented Dec 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tyriar Dec 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tyriar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tyriar commented Dec 27, 2018

Uh oh!

noamyogev84 commented Dec 27, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

noamyogev84 commented Oct 19, 2018 •

edited

Loading

Tyriar Dec 9, 2018 •

edited

Loading