starred/terminal
1
0
Fork 0
mirror of https://github.com/microsoft/terminal.git synced 2026-02-04 05:35:20 +00:00
Code Issues 19.0k Packages Projects Releases 20 Wiki Activity

De-duplicate code for TerminalDispatch and AdaptDispatch #5402

New Issue
Closed
opened 2026-01-31 00:12:32 +00:00 by claunia · 6 comments
claunia commented 2026-01-31 00:12:32 +00:00
Owner
Copy Link

Originally created by @zadjii-msft on GitHub (Dec 5, 2019).

Both these classes are doing a lot of the nearly same work at this point. We should probably not, and have more common implementations somehow.

IIRC, when I was writing it, I think part of the idea was to move ITerminalApi up and out of the Terminal to be shared, then have one Dispatch that calls into the API, then an adapter between that API and the conhost API

This is a super crude diagram

         +------------------+
         |  OutputEngine    |
         +---------+--------+
                   |
         +---------v--------+
         | OutputDispatch   |
         +--------+---------+
                  |
         +--------v---------+
         |   ITerminalApi   |
         +---+------------+-+
             |            |
             |            |
+------------v--+     +---v------------+
|  Terminal     |     |  HostAdapter   |
+---------------+     +-------+--------+
                              |
                      +-------v--------+
                      |   outputStream |
                      |      +         |
                      |      |         |
                      |      v         |
                      |   getset       |
                      +----------------+

In this model, Terminal and HostAdapter both implement ITerminalApi, so there only needs to be one Dispatcher. HostAdapter then calls into the conGetSet API we already have.

This would pretty horribly break tests that test the existing structure :/

I could be horribly off base with this though. I came up with this idea like 9 months ago and never wrote it down.

  • #5384 (xterm indexed color problem)
Originally created by @zadjii-msft on GitHub (Dec 5, 2019). Both these classes are doing a _lot_ of the nearly same work at this point. We should probably not, and have more common implementations somehow. **IIRC**, when I was writing it, I think part of the idea was to move `ITerminalApi` up and out of the Terminal to be shared, then have one Dispatch that calls into the API, then an adapter between that API and the conhost API This is a super crude diagram ``` +------------------+ | OutputEngine | +---------+--------+ | +---------v--------+ | OutputDispatch | +--------+---------+ | +--------v---------+ | ITerminalApi | +---+------------+-+ | | | | +------------v--+ +---v------------+ | Terminal | | HostAdapter | +---------------+ +-------+--------+ | +-------v--------+ | outputStream | | + | | | | | v | | getset | +----------------+ ``` In this model, `Terminal` and `HostAdapter` both implement `ITerminalApi`, so there only needs to be one Dispatcher. `HostAdapter` then calls into the `conGetSet` API we already have. This would pretty horribly break tests that test the existing structure :/ I could be horribly off base with this though. I came up with this idea like 9 months ago and never wrote it down. * [ ] #5384 (xterm indexed color problem)
claunia commented 2026-01-31 00:12:33 +00:00
Author
Owner
Copy Link

@miniksa commented on GitHub (Dec 5, 2019):

This sounds reasonable to me. Doesn't matter if it breaks the test. We've already identified that half of the adapter tests are pretty silly anyway. Probably need to revisit that anyway.

@miniksa commented on GitHub (Dec 5, 2019): This sounds reasonable to me. Doesn't matter if it breaks the test. We've already identified that half of the adapter tests are pretty silly anyway. Probably need to revisit that anyway.
claunia commented 2026-01-31 00:12:34 +00:00
Author
Owner
Copy Link

@miniksa commented on GitHub (Dec 17, 2019):

It looks like the TerminalDispatch methods don't really have tests on them either. So it's probably up to this task to consider testing overall.

@miniksa commented on GitHub (Dec 17, 2019): It looks like the TerminalDispatch methods don't really have tests on them either. So it's probably up to this task to consider testing overall.
claunia commented 2026-01-31 00:12:34 +00:00
Author
Owner
Copy Link

@j4james commented on GitHub (Dec 18, 2019):

My initial thought for this was that we would just drop the TerminalDispatch code and replace it entirely with AdaptDispatch. That already has all of the functionality that TerminalDispatch is trying to replicate, and it actually does have tests in place too.

Then the interface that splits the conhost and Terminal implementations could possibly be based on the current ConGetSet interface. It'll require quite a lot of cleanup before it's really suitable for that task, but I think that's probably the right place for it.

@j4james commented on GitHub (Dec 18, 2019): My initial thought for this was that we would just drop the `TerminalDispatch` code and replace it entirely with `AdaptDispatch`. That already has all of the functionality that `TerminalDispatch` is trying to replicate, and it actually does have tests in place too. Then the interface that splits the conhost and Terminal implementations could possibly be based on the current `ConGetSet` interface. It'll require quite a lot of cleanup before it's really suitable for that task, but I think that's probably the right place for it.
claunia commented 2026-01-31 00:12:35 +00:00
Author
Owner
Copy Link

@j4james commented on GitHub (Apr 11, 2021):

As I've mentioned elsewhere, I've been investigating ways to simplify the ConGetSet interface, with the idea that it could eventually be merged with the ITerminalApi interface, and AdaptDispatch could then more easily be merged with TerminalDispatch. I'm going to go one step at a time on this, so I'm not going to dump a massive PR on you, but the list below covers some of the ideas I'm considering. If there's anything here you definitely don't want me to do, please let me know.

  1. Replace GetConsoleScreenBufferInfoEx with a set of methods providing the specific properties we actually need, namely the buffer size, the virtual viewport dimensions, and the cursor position. This should also let us get rid of all the MoveToBottom calls which I think are only there to make sure the viewport returned by GetConsoleScreenBufferInfoEx is aligned with the virtual viewport.
  2. Get rid of the DeleteLines, InsertLines, and PrivateReverseLineFeed methods. These operations can easily be implemented in AdaptDispatch using existing scrolling and filling APIs, and then Windows Terminal can eventually inherit that functionality without having to implement any additional interfaces.
  3. Of the methods we do want to keep, we need to rename them to better match the ITerminalApi interface, for example getting rid of all of those Private prefixes. And if we're going to be rewriting a lot of the interface, this seems like a good time to try and modernize things a bit, e.g. replacing COORD with til::point, and RECT with til::rectangle (assuming that's the current preferred standard). I'm also very keen to get rid of the boolean return values as error indicators - we should be using return values for returning things, and throwing exceptions for errors.
  4. Again while we're rewriting things, this might be a good time to consider simplifying some of the bloat in the conhost implementation. Like is it really necessary for every ConhostInternalGetSet method to call a separate DoSrvPrivateXXXX function to actually do the work. Why not just implement the code right there in the ConhostInternalGetSet class?
  5. In the interests of future expansion, I'd like to try and coalesce some of the APIs where that seems reasonable. For example, do we really need 5 different mouse mode methods (which will eventually require another 5 matching getter methods), when we could just have one method with a mode parameter? The same thing applies to the various keyboard modes - I think there are only three at the moment, but that list is going to grow. It would make future expansion a lot easier if that just meant adding one more enum value.
@j4james commented on GitHub (Apr 11, 2021): As I've mentioned elsewhere, I've been investigating ways to simplify the `ConGetSet` interface, with the idea that it could eventually be merged with the `ITerminalApi` interface, and `AdaptDispatch` could then more easily be merged with `TerminalDispatch`. I'm going to go one step at a time on this, so I'm not going to dump a massive PR on you, but the list below covers some of the ideas I'm considering. If there's anything here you definitely _don't_ want me to do, please let me know. 1. Replace `GetConsoleScreenBufferInfoEx` with a set of methods providing the specific properties we actually need, namely the buffer size, the virtual viewport dimensions, and the cursor position. This should also let us get rid of all the `MoveToBottom` calls which I think are only there to make sure the viewport returned by `GetConsoleScreenBufferInfoEx` is aligned with the virtual viewport. 2. Get rid of the `DeleteLines`, `InsertLines`, and `PrivateReverseLineFeed` methods. These operations can easily be implemented in `AdaptDispatch` using existing scrolling and filling APIs, and then Windows Terminal can eventually inherit that functionality without having to implement any additional interfaces. 3. Of the methods we do want to keep, we need to rename them to better match the `ITerminalApi` interface, for example getting rid of all of those `Private` prefixes. And if we're going to be rewriting a lot of the interface, this seems like a good time to try and modernize things a bit, e.g. replacing `COORD` with `til::point`, and `RECT` with `til::rectangle` (assuming that's the current preferred standard). I'm also very keen to get rid of the boolean return values as error indicators - we should be using return values for returning things, and throwing exceptions for errors. 4. Again while we're rewriting things, this might be a good time to consider simplifying some of the bloat in the conhost implementation. Like is it really necessary for every `ConhostInternalGetSet` method to call a separate `DoSrvPrivateXXXX` function to actually do the work. Why not just implement the code right there in the `ConhostInternalGetSet` class? 5. In the interests of future expansion, I'd like to try and coalesce some of the APIs where that seems reasonable. For example, do we really need 5 different mouse mode methods (which will eventually require another 5 matching getter methods), when we could just have one method with a mode parameter? The same thing applies to the various keyboard modes - I think there are only three at the moment, but that list is going to grow. It would make future expansion a lot easier if that just meant adding one more enum value.
claunia commented 2026-01-31 00:12:35 +00:00
Author
Owner
Copy Link

@miniksa commented on GitHub (Jul 9, 2021):

I love all 5 of these things, @j4james. Feel free to take them on in whatever staging you deem sensible so we can have them reviewed.

@miniksa commented on GitHub (Jul 9, 2021): I love all 5 of these things, @j4james. Feel free to take them on in whatever staging you deem sensible so we can have them reviewed.
claunia commented 2026-01-31 00:12:35 +00:00
Author
Owner
Copy Link

@ghost commented on GitHub (May 24, 2022):

:tada:This issue was addressed in #13024, which has now been successfully released as Windows Terminal Preview v1.14.143.🎉

Handy links:

@ghost commented on GitHub (May 24, 2022): :tada:This issue was addressed in #13024, which has now been successfully released as `Windows Terminal Preview v1.14.143`.:tada: Handy links: * [Release Notes](https://github.com/microsoft/terminal/releases/tag/v1.14.143) * [Store Download](https://www.microsoft.com/store/apps/9n8g5rfz9xk3?cid=storebadge&ocid=badge)
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
⛺ Reserved
A11yCO
A11yMAS
A11ySev1
A11ySev2
A11ySev3
A11yTTValidated
A11yUsable
A11yVoiceAccess
A11yWCAG
Area-Accessibility
Area-AtlasEngine
Area-AzureShell
Area-Build
Area-Build
Area-Chat
Area-CmdPal
Area-CodeHealth
Area-Commandline
Area-CookedRead
Area-DefApp
Area-Extensibility
Area-Fonts
Area-GroupPolicy
Area-i18n
Area-Input
Area-Interaction
Area-Interop
Area-Localization
Area-Output
Area-Performance
Area-Portable
Area-Quality
Area-Remoting
Area-Rendering
Area-Schema
Area-Server
Area-Settings
Area-SettingsUI
Area-ShellExtension
Area-ShellExtension
Area-ShellExtension
Area-Suggestions
Area-Suggestions
Area-TerminalConnection
Area-TerminalControl
Area-Theming
Area-UserInterface
Area-VT
Area-Windowing
Area-WPFControl
AutoMerge
Blocking-Ingestion
Culprit-Centennial
Culprit-WinUI
Disability-All
Disability-Blind
Disability-LowVision
Disability-Mobility
External-Blocked-WinUI3
Fixed
Gathering-Data
good first issue
HCL-E+D
HCL-WindowsTerminal
Help Wanted
Impact-Compatibility
Impact-Compliance
Impact-Correctness
Impact-Visual
In-PR
InclusionBacklog
InclusionBacklog-Windows TerminalWin32
InclusionCommitted-202206
Issue-Bug
Issue-Docs
Issue-Feature
Issue-Feature
Issue-Question
Issue-Samples
Issue-Scenario
Issue-Task
Needs-Attention
Needs-Author-Feedback
Needs-Bisect
Needs-Discussion
Needs-Repro
Needs-Tag-Fix
Needs-Tag-Fix
Needs-Triage
No-Recent-Activity
Priority-0
Priority-1
Priority-2
Priority-3
Product-Cmd.exe
Product-Colortool
Product-Colortool
Product-Colortool
Product-Conhost
Product-Conpty
Product-Meta
Product-Powershell
Product-Terminal
Product-WSL
pull-request
Mirrored from GitHub Pull Request
 Resolution-Answered
Resolution-By-Design
Resolution-Duplicate
Resolution-External
Resolution-Fix-Available
Resolution-Fix-Committed
Resolution-No-Repro
Resolution-Won't-Fix
Severity-Blocking
Severity-Crash
Severity-DataLoss
spam
this-will-be-a-breaking-change
Tracking-External
WindowsTerminal_Win32
Work-Item
zAskModeBug
zInbox-Bug
No Label Area-CodeHealth Area-VT Issue-Task Product-Conhost Product-Terminal Resolution-Fix-Committed
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
claunia
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: starred/terminal#5402
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?