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

Prevent search result from being deselected if it is the only result #2491

Merged
merged 9 commits into from Oct 22, 2019

Conversation

miggs125
Copy link
Contributor

fixes #2456

Prevents single results from being deselected after pressing enter multiple times on a successful search. I couldn't find tests for addons so I wasn't sure if some needed to be made. I will be happy to include them if they are needed.

remove leading whitespace
prevent single search term deselection in findPrevious
@Tyriar
Copy link
Member

Tyriar commented Oct 21, 2019

@miggs125 sorry I just overwrote your commit. I think the test I added should be good now, what was that last commit you added on?

@miggs125
Copy link
Contributor Author

@Tyriar I saw that some tests had not passed after you added the test suite so I commited the fix just recently. The last commit on my branch should be 0e6fde5

@miggs125
Copy link
Contributor Author

@Tyriar for some reason the tests you added are passing on my local copy of the repo but not for the CI tests, not sure what is going on here.

image

@miggs125
Copy link
Contributor Author

I modified the search tests to output the result of the functions being tested just to confirm nothing unexpected was going on and got this:

image

Based on this, the fix appears to be working as expected so I am unsure as to why the CI tests are failing.

@@ -97,6 +97,9 @@ export class SearchAddon implements ITerminalAddon {
}
}

// If there is only one result, return false.
if (!result && currentSelection) return false;
Copy link
Member

Choose a reason for hiding this comment

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

The test fails for me, it's because it returns false if there's no selection. I think what we actually want to do is return true instead, this make sense since we succeeded but just didn't touch the selection as it would be redundant to do so.

@@ -59,12 +59,12 @@ export class SearchAddon implements ITerminalAddon {

let startCol = 0;
let startRow = 0;

let currentSelection = null;
Copy link
Member

Choose a reason for hiding this comment

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

Let's make this let currentSelect: ISelectionPosition | undefined, using undefined saves us from initializing it.

Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

Looks great, merging after CI passes 👌

@Tyriar Tyriar merged commit d9f58c0 into xtermjs:master Oct 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Search should not deselect search term if it's the only result
2 participants