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

Search addon: Return the number of results of a search #1659

Closed
Tyriar opened this issue Sep 5, 2018 · 4 comments · Fixed by #3716
Closed

Search addon: Return the number of results of a search #1659

Tyriar opened this issue Sep 5, 2018 · 4 comments · Fixed by #3716
Labels

Comments

@Tyriar
Copy link
Member

Tyriar commented Sep 5, 2018

This request may impact performance, this should be evaluated.

Plan issue: #705

@Tyriar Tyriar added type/enhancement Features or improvements to existing features help wanted addon good first issue labels Sep 5, 2018
@noamyogev84
Copy link
Contributor

noamyogev84 commented Sep 22, 2018

Hi @Tyriar
I'm trying to understand this enhancement. Currently the search addon only supports find next & find previous. what is your expectation regarding the number of results?

  • how should it be outputted?

@Tyriar
Copy link
Member Author

Tyriar commented Sep 23, 2018

@noamyogev84 currently the search addon returns a boolean, I think we should return a number instead:

public findNext(term: string, searchOptions?: ISearchOptions): boolean {

It's probably also a good idea to return only 0 or 1 unless a new option to return number of results (countResults: boolean?) is set:

export interface ISearchOptions {
regex?: boolean;
wholeWord?: boolean;
caseSensitive?: boolean;
}

@noamyogev84
Copy link
Contributor

noamyogev84 commented Sep 25, 2018

Hi @Tyriar,
Although I have some pretty straight forward implementation for this, I do have some wonderings:

  1. Why do we need this second iteration? (line 48)

    // 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);
    if (result) {
    break;
    }
    }
    // Search from the top to the current ydisp
    if (!result) {
    for (let y = 0; y < startRow; y++) {
    result = this._findInLine(term, y, searchOptions);
    if (result) {
    break;
    }
    }
    }

  2. Currently for multiple matches in the same line it only returns (and counts) the first occurrence. do you think this should first be redesigned before we return the number of results? In reference to Search addon: Allow finding multiple instances on the same line #1654

@Tyriar
Copy link
Member Author

Tyriar commented Oct 8, 2018

@noamyogev84 sorry about the delay. 1: the second iteration is because findNext does not start from the top:

  • First iteration: Search from the top of the viewport to the bottom of the buffer
  • Second iteration: Search from the top of the buffer to the top of the viewport

2: I think the code to collect the total should look a little different to the current code, if it's true we want to continue searching until we've wrapped around to the start.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants