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

proposal to fix issue 21600 #21859

Merged

Conversation

wimspaargaren
Copy link
Contributor

Hello, I got a proposal to resolve #21600.

The showPreviousSearchTerm function tried to get the previous search term, but since clear() does not add a value to the searchHistory the showPreviousSearchTerm returned the one last search term.

This is my first pull request, so please let me know if anything is wrong.

Hope it helps!

@mention-bot
Copy link

@wimspaargaren, thanks for your PR! By analyzing the history of the files in this pull request, we identified @sandy081 and @jrieken to be potential reviewers.

@msftclas
Copy link

msftclas commented Mar 3, 2017

This seems like a small (but important) contribution, so no Contribution License Agreement is required at this point. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

@sandy081 sandy081 added this to the March 2017 milestone Mar 3, 2017
@sandy081 sandy081 self-requested a review March 3, 2017 09:00
@sandy081
Copy link
Member

sandy081 commented Mar 3, 2017

@wimspaargaren Thanks for the PR. I will take a look and get back to you. Before that as a first step, can you please make sure that (following) builds are green and also have the proper formatting.

@wimspaargaren
Copy link
Contributor Author

@sandy081 Yes I will look into it now.

@wimspaargaren
Copy link
Contributor Author

@sandy081 I fixed the formatting issues.

@wimspaargaren
Copy link
Contributor Author

@sandy081 I found that my previous solution introduced a new bug, so I created a simpler, and I think also better, solution to fix the bug. Let me know if I should adjust anything.

@sandy081
Copy link
Member

sandy081 commented Mar 6, 2017

@wimspaargaren New approach looks good.. but this approach made me think about another related case.. what if user types some text after clearing and try to get the previous term (using ctrl up) ?

How about adding the current search term to the history, when user asks for previous term ?

@wimspaargaren
Copy link
Contributor Author

wimspaargaren commented Mar 6, 2017

@sandy081 if I understand you correctly you want the following:

If a user enters a search term, doesn't hit enter, but presses ctrl up instead, the current search term needs to be added to the history.

I will look further into this tomorrow afternoon and let you know when I came up with a solution.

@wimspaargaren
Copy link
Contributor Author

@sandy081 I couldn't resist to implement it already :) Is this what you meant, and if so do you think this is a good solution?
I added an extra function to the HistoryNavigator called addIfNotPresent, which checks if a HistoryElement is already present in the history. When using the normal add, showPreviousTerm(alt up) would start toggling between the last two history elements(because they are keep getting added to the history).
I see travis isn't happy about me, but I don't really understand from the travis log, which test is going wrong. Could you maybe give some more information on how to resolve this? When I run "scripts\test" on windows everything seems to be passing.

@sandy081
Copy link
Member

sandy081 commented Mar 7, 2017

@wimspaargaren Thats cool and this is what I mentioned in my comment.

The test failing in the Travis build is due to the other changes in the stream. Its not because of this so no need to worry.

LGTM. Thanks for the PR. Merged.

@sandy081 sandy081 merged commit 56c54ef into microsoft:master Mar 7, 2017
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clearing search results - first history item is wrong
4 participants