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 - add search box control and implement search experience #3590

Conversation

KaiyuWang16
Copy link
Contributor

@KaiyuWang16 KaiyuWang16 commented Nov 15, 2019

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.

The PR that migrates the Conhost search module: #3279
Spec (still actively updating): #3299

PR Checklist

  • Closes Feature Request: Search #605
  • CLA signed. If not, go over here 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

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.
  5. Close by clicking 'X'.
  6. 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.

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.

@KaiyuWang16 KaiyuWang16 changed the title Dev/kawa/605 search experice implementation add search box control and search experience Search - add search box control and implement search experience Nov 15, 2019
src/cascadia/TerminalApp/AppKeyBindings.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/AppActionHandlers.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalCore/terminalrenderdata.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalControl/SearchBoxControl.h Outdated Show resolved Hide resolved
src/cascadia/TerminalControl/SearchBoxControl.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalControl/SearchBoxControl.xaml Outdated Show resolved Hide resolved
src/cascadia/TerminalControl/SearchBoxControl.xaml Outdated Show resolved Hide resolved
src/cascadia/TerminalControl/SearchBoxControl.xaml Outdated Show resolved Hide resolved
@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Nov 20, 2019
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Nov 20, 2019
@KaiyuWang16 KaiyuWang16 removed the request for review from DHowett-MSFT November 20, 2019 04:34
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

I've only taken a cursory glance at a couple files so far, but I'm just gonna go ahead and pre-emptively block this PR until the spec (#3299) is merged

I am updating the spec today.

src/cascadia/TerminalApp/AppKeyBindingsSerialization.cpp Outdated Show resolved Hide resolved
@ghost ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Dec 11, 2019
@KaiyuWang16 KaiyuWang16 mentioned this pull request Dec 12, 2019
20 tasks
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

Fundamentally, this works so that's good. But there's definitely a few problems that need resolving before this can get merged. For me, the two remaining blocking issues are:

  • the placeholder text color is wrong in dark mode
  • RenderData::GetTextBufferEndPosition should not be GetCursorPosition

The other things I'm concerned about, but not blocking over:

  • "Find..." needs to be localizable
  • Search event should have a delegate with more args
  • Escape should be handled in the control too, as opposed to bubbling
  • The whole _goForward=!_goForward;_Search();_goForward=!_goForward thing is weird

src/cascadia/TerminalControl/SearchBoxControl.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalControl/TermControl.cpp Show resolved Hide resolved
src/buffer/out/search.cpp Show resolved Hide resolved
@ghost ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Dec 12, 2019
@DHowett-MSFT
Copy link
Contributor

There are still some outstanding comments.

@microsoft microsoft deleted a comment from zadjii-msft Dec 16, 2019
@KaiyuWang16
Copy link
Contributor Author

There are still some outstanding comments.

Unblocking ones are moved to #3942

Copy link
Contributor

@DHowett-MSFT DHowett-MSFT left a comment

Choose a reason for hiding this comment

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

I'm comfortable with this as long as we follow up on the followup issues.

Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning those blocking things up! We can fix the rest in post 😝

@zadjii-msft
Copy link
Member

(I'm merging this since there's a hyper-quorum on it)

@zadjii-msft zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Dec 17, 2019
@ghost
Copy link

ghost commented Dec 17, 2019

Hello @zadjii-msft!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit d7ae8e6 into master Dec 17, 2019
@ghost ghost deleted the dev/kawa/605-Search-Experice-Implementation-Add-SearchBoxControl-and-Search-Experience branch December 17, 2019 15:52
@KaiyuWang16
Copy link
Contributor Author

Thanks for cleaning those blocking things up! We can fix the rest in post 😝

Thanks for merging ! I am working on the follow-ups these days.

@ghost
Copy link

ghost commented Jan 14, 2020

🎉Windows Terminal Preview v0.8.10091.0 has been released which incorporates this pull request.:tada:

Handy links:

@ghost ghost mentioned this pull request Jan 14, 2020
@kell18
Copy link

kell18 commented Jan 14, 2020

Yeees!

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) AutoMerge Marked for automatic merge by the bot when requirements are met Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: Search
8 participants