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.

Sign up for GitHub

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

Jump to bottom

Refactor Search module to have different implementations for Terminal and ConHost #3542

Closed
KaiyuWang16 opened this issue Nov 12, 2019 · 2 comments
Closed

Refactor Search module to have different implementations for Terminal and ConHost #3542

KaiyuWang16 opened this issue Nov 12, 2019 · 2 comments
Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Tag-Fix Doesn't match tag requirements Product-Conhost For issues in the Console codebase Product-Terminal The new Windows Terminal.
Milestone
Engineering Impro...

Comments

@KaiyuWang16
Copy link
Contributor

Description of the new feature/enhancement

This is a follow up of #3279. Search module is migrated as a shared component for Terminal and ConHost. However, Color method is used in ConHost but not in Terminal. For now, in Terminal directory we only call Terminal::ColorSelection, which does nothing but just throws exception. We need to consider refactor the codes to provide ConHost and Terminal different implementation.

A possible fix is to make Search an abstract class and provide different implementation in Terminal and ConHost.

Proposed technical implementation details (optional)

Search class can be an abstract class, and we implement it in Terminal and ConHost directory. In this way we can have more specific realization of the Color method.

Search module is well-designed and implemented and can fit Terminal and ConHost's needs.
@KaiyuWang16 KaiyuWang16 added the Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. label Nov 12, 2019
@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Nov 12, 2019
@KaiyuWang16 KaiyuWang16 mentioned this issue Nov 12, 2019
Merged
5 tasks
@DHowett-MSFT DHowett-MSFT added Area-UserInterface Issues pertaining to the user interface of the Console or Terminal Issue-Task It's a feature request, but it doesn't really need a major design. Product-Conhost For issues in the Console codebase Product-Terminal The new Windows Terminal. and removed Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Nov 14, 2019
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Nov 14, 2019
@DHowett-MSFT DHowett-MSFT added this to the Terminal v1.0 milestone Nov 14, 2019
@carlos-zamora
Copy link
Member

This seems more of a CodeHealth thing to me. I'm throwing it into our v2 milestone and assigning it to myself for now.

This might be a random thing we should do as a part of working on Search v2 in #3920.

@carlos-zamora carlos-zamora added Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. and removed Area-UserInterface Issues pertaining to the user interface of the Console or Terminal labels May 12, 2020
@carlos-zamora carlos-zamora self-assigned this May 12, 2020
@carlos-zamora carlos-zamora removed their assignment Oct 21, 2021
@lhecker
Copy link
Member

lhecker commented Aug 21, 2023

I'm gonna go ahead and close this issue. Both applications now use the exact same functionality. Also, I rewrote Search the past few days and it's now 33 LOC. It's not a maintenance burden anymore. 🙂

@lhecker lhecker closed this as completed Aug 21, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Tag-Fix Doesn't match tag requirements label Aug 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Tag-Fix Doesn't match tag requirements Product-Conhost For issues in the Console codebase Product-Terminal The new Windows Terminal.
Projects
None yet
Milestone
Engineering Improvements
Development

No branches or pull requests
7 participants
@DHowett @lhecker @carlos-zamora @KaiyuWang16 @DHowett-MSFT @zadjii-msft @cinnamon-msft