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

Closed
opened 2026-01-31 00:01:17 +00:00 by claunia · 2 comments
Owner

Originally created by @KaiyuWang16 on GitHub (Nov 12, 2019).

Description of the new feature/enhancement

This is a follow up of https://github.com/microsoft/terminal/pull/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.

Originally created by @KaiyuWang16 on GitHub (Nov 12, 2019). <!-- 🚨🚨🚨🚨🚨🚨🚨🚨🚨🚨 I ACKNOWLEDGE THE FOLLOWING BEFORE PROCEEDING: 1. If I delete this entire template and go my own path, the core team may close my issue without further explanation or engagement. 2. If I list multiple bugs/concerns in this one issue, the core team may close my issue without further explanation or engagement. 3. If I write an issue that has many duplicates, the core team may close my issue without further explanation or engagement (and without necessarily spending time to find the exact duplicate ID number). 4. If I leave the title incomplete when filing the issue, the core team may close my issue without further explanation or engagement. 5. If I file something completely blank in the body, the core team may close my issue without further explanation or engagement. All good? Then proceed! --> # Description of the new feature/enhancement This is a follow up of https://github.com/microsoft/terminal/pull/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. <!-- A clear and concise description of what the problem is that the new feature would solve. Describe why and how a user would use this new functionality (if applicable). --> # 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. <!-- A clear and concise description of what you want to happen. --> Search module is well-designed and implemented and can fit Terminal and ConHost's needs.
Author
Owner

@carlos-zamora commented on GitHub (May 13, 2020):

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 commented on GitHub (May 13, 2020): 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.
Author
Owner

@lhecker commented on GitHub (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 commented on GitHub (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. 🙂
Sign in to join this conversation.
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: starred/terminal#4946