Migrate Search module as a shared component for Terminal Search #3279
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
KaiyuWang16
changed the title
carlos-zamora
requested changes
Oct 22, 2019
ghost
added
Needs-Author-Feedback
KaiyuWang16
requested review from
DHowett-MSFT,
miniksa,
bitcrazed and
cinnamon-msft
KaiyuWang16
removed request for
bitcrazed,
DHowett-MSFT,
miniksa and
cinnamon-msft
|
For now, you're migrating Search into a shared space. Do you plan on pushing this into master when done? If so, please update the PR title to reflect that. If not, what's the plan? Are you going to make branches targeting this one enabling more functionality? Just checking in. |
My original plan is to continue using this PR for the rest of the changes. But I will discuss with Dustin about this, I think it also makes sense to push the current change first. |
KaiyuWang16
changed the title
zadjii-msft
reviewed
Nov 5, 2019
There was a problem hiding this comment.
(realizing now I never sent this, sorry)
I think I'm okay with this, but it looks like @carlos-zamora has a bunch of other questions that I also want to see resolutions to before signing off.
In terms of commit order, I think I'd rather get this in as an atomic piece if at all possible, then add the Terminal UI as a wholly separate PR (which I believe is the plan)
miniksa
reviewed
Nov 7, 2019
miniksa
reviewed
Nov 13, 2019
|
I think this is close. Can you fix the conflict and address the few questions and we can probably get merged and move onto the next items you filed as follow ons? |
carlos-zamora
approved these changes
Nov 13, 2019
miniksa
approved these changes
Nov 14, 2019
KaiyuWang16
changed the title
Closed
5 tasks
ghost
pushed a commit
that referenced
this pull request
Dec 17, 2019
<!-- Enter a brief description/summary of your PR here. What does it fix/what does it change/how was it tested (even manually, if necessary)? --> This is the PR for feature Search: #605 This PR includes the newly introduced SearchBoxControl in TermControl dir, which is the search bar for the search experience. And the codes that enable Search in Windows Terminal. <!-- Other than the issue solved, is this relevant to any other issues/existing PRs? --> The PR that migrates the Conhost search module: #3279 Spec (still actively updating): #3299 <!-- Please review the items on the PR checklist before submitting--> ## PR Checklist * [x] Closes #605 * [ ] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [ ] Tests added/passed * [ ] Requires documentation to be updated * [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx <!-- Provide a more detailed description of the PR, other things fixed or any additional comments/features here --> These functionalities are included in the search experience. 1. Search in Terminal text buffer. 2. Automatic wrap-around. 3. Search up or down switch by clicking different buttons. 4. Search case sensitively/insensitively by clicking a button. S. Move the search box to the top/bottom by clicking a button. 6. Close by clicking 'X'. 7. Open search by ctrl + F. When the searchbox is open, the user could still interact with the terminal by clicking the terminal input area. While I already have the search functionalities, currently there are still some known to-do works and I will keep updating my PR: 1. Optimize the search box UI, this includes: 1) Theme adaptation. The search box background and font color should change according to the theme, 2) Add background. Currently the elements in search box are all transparent. However, we need a background. 3) Move button should be highlighted once clicked. 2. Accessibility: search process should be able to performed without mouse. Once the search box is focused, the user should be able to navigate between all interactive elements on the searchbox using keyboard. <!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well --> To test: 1. checkout this branch. 2. Build the project. 3. Start Windows Terminal and press Ctrl+F 4. The search box should appear on the top right corner.
This was referenced Jan 28, 2020
ghost
pushed a commit
that referenced
this pull request
Jan 31, 2020
Moved `FindText` to `UiaTextRangeBase`. Now that Search is a shared component (thanks #3279), I can just reuse it basically as-is. #3279 - Make Search a shared component #4018 - UiaTextRange Refactor I removed it from the two different kinds of UiaTextRange and put it in the base class. I needed a very minor change to ensure we convert from an inclusive end (from Search) to an exclusive end (in UTR). Worked with `FindText` was globally messed with in windows.h. So we had to do a few weird things there (thanks Michael). No need for additional tests because it _literally_ just sets up a Searcher and calls it.
donno2048
added a commit
to donno2048/terminal
that referenced
this pull request
Sep 28, 2020
<!-- Enter a brief description/summary of your PR here. What does it fix/what does it change/how was it tested (even manually, if necessary)? --> This is the PR for feature Search: #605 This PR includes the newly introduced SearchBoxControl in TermControl dir, which is the search bar for the search experience. And the codes that enable Search in Windows Terminal. <!-- Other than the issue solved, is this relevant to any other issues/existing PRs? --> The PR that migrates the Conhost search module: microsoft/terminal#3279 Spec (still actively updating): microsoft/terminal#3299 <!-- Please review the items on the PR checklist before submitting--> ## PR Checklist * [x] Closes #605 * [ ] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [ ] Tests added/passed * [ ] Requires documentation to be updated * [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx <!-- Provide a more detailed description of the PR, other things fixed or any additional comments/features here --> These functionalities are included in the search experience. 1. Search in Terminal text buffer. 2. Automatic wrap-around. 3. Search up or down switch by clicking different buttons. 4. Search case sensitively/insensitively by clicking a button. S. Move the search box to the top/bottom by clicking a button. 6. Close by clicking 'X'. 7. Open search by ctrl + F. When the searchbox is open, the user could still interact with the terminal by clicking the terminal input area. While I already have the search functionalities, currently there are still some known to-do works and I will keep updating my PR: 1. Optimize the search box UI, this includes: 1) Theme adaptation. The search box background and font color should change according to the theme, 2) Add background. Currently the elements in search box are all transparent. However, we need a background. 3) Move button should be highlighted once clicked. 2. Accessibility: search process should be able to performed without mouse. Once the search box is focused, the user should be able to navigate between all interactive elements on the searchbox using keyboard. <!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well --> To test: 1. checkout this branch. 2. Build the project. 3. Start Windows Terminal and press Ctrl+F 4. The search box should appear on the top right corner.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is the implementation of feature request #605
Search in the Terminal. For the first version, we realize case sensitive/insensitive exact text match in the text buffer of the Terminal screen.
For now, the PR only include the first necessary step - making the search module originally in host/ a shared component between Terminal and console host.
PR Checklist
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed