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

Further simplification of the ConGetSet interface #16969

New Issue
Closed
opened 2026-01-31 05:28:44 +00:00 by claunia · 8 comments
claunia commented 2026-01-31 05:28:44 +00:00
Owner
Copy Link

Originally created by @j4james on GitHub (Mar 10, 2022).

Originally assigned to: @j4james on GitHub.

Description of the new feature/enhancement

I've been working for a while now on a refactoring the ConhostInternalGetSet code (as a follow up to PR #12247), trying to move as much as possible into the AdaptDispatch class, and condensing the ConGetSet interface down to a minimal set of operations.

The idea is that the the AdaptDispatch class could then be used in Windows Terminal in place of the existing TerminalDispatch (i.e. issue #3849), and only have a handful of methods in the ConGetSet interface needing terminal-specific implementations.

I'm not done yet, but I'm far enough along that I'm satisfied that this is the right approach, and I wanted to let others know my plans, and check whether you're happy with what I'm doing.

Proposed technical implementation details (optional)

The general idea is that we provide the means for AdaptDispatch to access the high-level structures that are common to both conhost and terminal (like the Renderer and the TextBuffer), and then a lot of the operations like EraseAll, InsertLines, and ScrollRegion can be implemented entirely in AdaptDispatch, avoiding the need to implement them twice.

In some cases, the structures we need are static, and can be passed through to AdaptDispatch in the constructor (e.g. the Renderer, RenderSettings, and TerminalInput). Others are dynamic, but can easily be retrieve via new methods in the ConGetSet interface (e.g. the active TextBuffer, and the active virtual viewport).

Taking this approach, I've so far been able to eliminate 31 methods from the ConGetSet interface (adding only 4 new ones), and I think there are still more that could be nuked eventually.

My current plan was to submit this as two PRs: the first covering the refactoring of ConhostInternalGetSet, and the second dealing with the reuse of Adaptdispatch in the terminal (I haven't looked at that in much detail yet, so it might be more complicated than I imagine). Even just the first bit is quite a large change, though, so maybe you'd prefer it split up even more?

Originally created by @j4james on GitHub (Mar 10, 2022). Originally assigned to: @j4james on GitHub. # Description of the new feature/enhancement I've been working for a while now on a refactoring the `ConhostInternalGetSet` code (as a follow up to PR #12247), trying to move as much as possible into the `AdaptDispatch` class, and condensing the `ConGetSet` interface down to a minimal set of operations. The idea is that the the `AdaptDispatch` class could then be used in Windows Terminal in place of the existing `TerminalDispatch` (i.e. issue #3849), and only have a handful of methods in the `ConGetSet` interface needing terminal-specific implementations. I'm not done yet, but I'm far enough along that I'm satisfied that this is the right approach, and I wanted to let others know my plans, and check whether you're happy with what I'm doing. # Proposed technical implementation details (optional) The general idea is that we provide the means for `AdaptDispatch` to access the high-level structures that are common to both conhost and terminal (like the `Renderer` and the `TextBuffer`), and then a lot of the operations like `EraseAll`, `InsertLines`, and `ScrollRegion` can be implemented entirely in `AdaptDispatch`, avoiding the need to implement them twice. In some cases, the structures we need are static, and can be passed through to `AdaptDispatch` in the constructor (e.g. the `Renderer`, `RenderSettings`, and `TerminalInput`). Others are dynamic, but can easily be retrieve via new methods in the `ConGetSet` interface (e.g. the active `TextBuffer`, and the active virtual viewport). Taking this approach, I've so far been able to eliminate 31 methods from the `ConGetSet` interface (adding only 4 new ones), and I think there are still more that could be nuked eventually. My current plan was to submit this as two PRs: the first covering the refactoring of `ConhostInternalGetSet`, and the second dealing with the reuse of `Adaptdispatch` in the terminal (I haven't looked at that in much detail yet, so it might be more complicated than I imagine). Even just the first bit is quite a large change, though, so maybe you'd prefer it split up even more?
claunia commented 2026-01-31 05:28:46 +00:00
Author
Owner
Copy Link

@lhecker commented on GitHub (Mar 11, 2022):

Love it! I'm personally entirely fine with reviewing large PRs. The amount of work is the same no matter how many PRs it's split up in after all. I'm not sure whether we could merge something like that within this month. In general however, I'd be absolutely looking forward to this!

...even if this is absolutely shredding my SMALL_RECT-refactor with merge conflicts already. 😄

@lhecker commented on GitHub (Mar 11, 2022): Love it! I'm personally entirely fine with reviewing large PRs. The amount of work is the same no matter how many PRs it's split up in after all. I'm not sure whether we could merge something like that within this month. In general however, I'd be absolutely looking forward to this! ...even if this is absolutely shredding my `SMALL_RECT`-refactor with merge conflicts already. 😄
claunia commented 2026-01-31 05:28:46 +00:00
Author
Owner
Copy Link

@DHowett commented on GitHub (Mar 11, 2022):

I am absolute in favor of this. Thank you 😄

@DHowett commented on GitHub (Mar 11, 2022): I am _absolute_ in favor of this. Thank you :smile:
claunia commented 2026-01-31 05:28:46 +00:00
Author
Owner
Copy Link

@j4james commented on GitHub (Mar 12, 2022):

...even if this is absolutely shredding my SMALL_RECT-refactor with merge conflicts already.

I was kind of worried about that. I hate to ask this, but it would be helpful if we could delay those changes until after AdaptDispatch and TerminalDispatch are merged (assuming that's not going to block you for other work). At that point most of the terminal code is going to be nuked anyway, and the AdaptDispatch code is going to be significantly rewritten. And I'm almost sure you're going to want to refactor some of the new stuff I've added, because right now it's still a bit of a mish-mash of the old and new classes.

@j4james commented on GitHub (Mar 12, 2022): > ...even if this is absolutely shredding my `SMALL_RECT`-refactor with merge conflicts already. I was kind of worried about that. I hate to ask this, but it would be helpful if we could delay those changes until after `AdaptDispatch` and `TerminalDispatch` are merged (assuming that's not going to block you for other work). At that point most of the terminal code is going to be nuked anyway, and the `AdaptDispatch` code is going to be significantly rewritten. And I'm almost sure you're going to want to refactor some of the new stuff I've added, because right now it's still a bit of a mish-mash of the old and new classes.
claunia commented 2026-01-31 05:28:46 +00:00
Author
Owner
Copy Link

@lhecker commented on GitHub (Mar 12, 2022):

@j4james I'll probably open a draft PR with my changes in April, since I already have them ready anyways (and am just continuing to rebase it on main). I can wait until after your changes are merged to actually open it for review. 🙂

@lhecker commented on GitHub (Mar 12, 2022): @j4james I'll probably open a draft PR with my changes in April, since I already have them ready anyways (and am just continuing to rebase it on main). I can wait until after your changes are merged to actually open it for review. 🙂
claunia commented 2026-01-31 05:28:46 +00:00
Author
Owner
Copy Link

@j4james commented on GitHub (Mar 12, 2022):

Oh, I think I misunderstood you. I was under the impression that PR #12207 was your SMALL_RECT refactor. If you've still got another follow up PR in the works, then you might as well do them both first. If #12207 gets merged, I'm likely going to have to start from scratch anyway. It's pointless me then inserting my changes in the middle of your two PRs, which is just going to force you to rewrite as well.

@j4james commented on GitHub (Mar 12, 2022): Oh, I think I misunderstood you. I was under the impression that PR #12207 was your `SMALL_RECT` refactor. If you've still got another follow up PR in the works, then you might as well do them both first. If #12207 gets merged, I'm likely going to have to start from scratch anyway. It's pointless me then inserting my changes in the middle of your two PRs, which is just going to force you to rewrite as well.
claunia commented 2026-01-31 05:28:46 +00:00
Author
Owner
Copy Link

@lhecker commented on GitHub (Mar 12, 2022):

@j4james If you have to start from scratch, I can just wait with my PR. #12207 indeed is only about 20% of my changes, but almost all of them are basic type and member renames throughout the code base. This makes it it's trivial to rebase it, so please go ahead with your changes, as I'll simply rebase my stuff later. 🙂

@lhecker commented on GitHub (Mar 12, 2022): @j4james If you have to start from scratch, I can just wait with my PR. #12207 indeed is only about 20% of my changes, but almost all of them are basic type and member renames throughout the code base. This makes it it's trivial to rebase it, so please go ahead with your changes, as I'll simply rebase my stuff later. 🙂
claunia commented 2026-01-31 05:28:47 +00:00
Author
Owner
Copy Link

@j4james commented on GitHub (Mar 15, 2022):

@lhecker I've opened a draft PR with what I've got so far so you can see what I'm trying to do, and why I think it would be problematic to merge after #12207. It wouldn't be the end of the world if I needed to redo it, though - it'll just take a bit more time.

@j4james commented on GitHub (Mar 15, 2022): @lhecker I've opened a draft PR with what I've got so far so you can see what I'm trying to do, and why I think it would be problematic to merge after #12207. It wouldn't be the end of the world if I needed to redo it, though - it'll just take a bit more time.
claunia commented 2026-01-31 05:28:47 +00:00
Author
Owner
Copy Link

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

:tada:This issue was addressed in #12703, 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 #12703, 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 Issue-Feature Product-Meta 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#16969
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?