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

Can we get rid of the IRenderTarget interface? #16844

New Issue
Open
opened 2026-01-31 05:24:52 +00:00 by claunia · 0 comments
claunia commented 2026-01-31 05:24:52 +00:00
Owner
Copy Link

Originally created by @j4james on GitHub (Feb 22, 2022).

Description of the new feature/enhancement

As I understand it, the IRenderTarget interface is primarily used by the TextBuffer class to support the concept of multiple buffers in conhost. So when a text buffer triggers a redraw, that goes through the ScreenBufferRenderTarget implementation, and it'll only forward the call to the renderer if we're in the active buffer.

But what if we added a flag in the TextBuffer class to track if it is the active buffer or not? That's easy to update whenever the active buffer changes, and then we can deal with the renderer directly. When the buffer needs to trigger a redraw, it simply checks the flag to see if it's active before forwarding the request to the renderer.

I've been working on an PR for this, and it looks to me like it's doable. It doesn't appear to make a great difference to the performance one way or the other, but it does shrink the binary, and I think it makes the code a little simpler. My main reason for wanting this, though, is because I'm hoping it will help with the AdaptDispatch refactoring I'm working on for #3849.

Proposed technical implementation details (optional)

TextBuffer gets a new field which tracks whether it's active or not, and has a reference to the actual Renderer class rather than IRenderTarget. When it needs to trigger a redraw (or any of the other IRenderTarget methods), it checks whether it's active or not before forwarding the request to the renderer.

Anywhere that is currently getting a IRenderTarget from the TextBuffer in order to trigger a redraw, would now just call the TriggerRedraw method on the TextBuffer class itself, since that will handle the active check which would previously have been the responsibility of the render target. All the trigger methods that are currently in IRenderTarget would now be exposed in TextBuffer.

One catch with this approach is that we'd need to rethink the initialization order in conhost. In the ConsoleInitializeConnectInfo function, we'd need to move the Renderer initialization up so it's constructed first, and only after that call the SetUpConsole function to initialize the buffer. Once they're both initialized, we can then call the Renderer::EnablePainting method to allow the render thread to start.

The other catch is that the renderer tries to setup its initial viewport in the constructor, but at that point in time the viewport size would be unknown. However, that can easily be addressed by moving the viewport initialization into the EnablePainting method, since that would still be called after the buffer is setup (as mentioned above).

Note that this order of initialization more closely matches the Windows Terminal implementation, which is one of the reasons why I think it'll be beneficial for #3849.

On the whole, the changes needed to make this work look fairly straightforward. The biggest pain is getting the unit tests to work again, because some of them rely on the IRenderTarget interface for mocking the renderer, and others depend on the order of initialization. They're all fixable, but it might be a bit messy.

Originally created by @j4james on GitHub (Feb 22, 2022). # Description of the new feature/enhancement As I understand it, the `IRenderTarget` interface is primarily used by the `TextBuffer` class to support the concept of multiple buffers in conhost. So when a text buffer triggers a redraw, that goes through the `ScreenBufferRenderTarget` implementation, and it'll only forward the call to the renderer if we're in the active buffer. But what if we added a flag in the `TextBuffer` class to track if it is the active buffer or not? That's easy to update whenever the active buffer changes, and then we can deal with the renderer directly. When the buffer needs to trigger a redraw, it simply checks the flag to see if it's active before forwarding the request to the renderer. I've been working on an PR for this, and it looks to me like it's doable. It doesn't appear to make a great difference to the performance one way or the other, but it does shrink the binary, and I think it makes the code a little simpler. My main reason for wanting this, though, is because I'm hoping it will help with the `AdaptDispatch` refactoring I'm working on for #3849. # Proposed technical implementation details (optional) `TextBuffer` gets a new field which tracks whether it's active or not, and has a reference to the actual `Renderer` class rather than `IRenderTarget`. When it needs to trigger a redraw (or any of the other `IRenderTarget` methods), it checks whether it's active or not before forwarding the request to the renderer. Anywhere that is currently getting a `IRenderTarget` from the `TextBuffer` in order to trigger a redraw, would now just call the `TriggerRedraw` method on the `TextBuffer` class itself, since that will handle the active check which would previously have been the responsibility of the render target. All the trigger methods that are currently in `IRenderTarget` would now be exposed in `TextBuffer`. One catch with this approach is that we'd need to rethink the initialization order in conhost. In the `ConsoleInitializeConnectInfo` function, we'd need to move the `Renderer` initialization up so it's constructed first, and only after that call the `SetUpConsole` function to initialize the buffer. Once they're both initialized, we can then call the `Renderer::EnablePainting` method to allow the render thread to start. The other catch is that the renderer tries to setup its initial viewport in the constructor, but at that point in time the viewport size would be unknown. However, that can easily be addressed by moving the viewport initialization into the `EnablePainting` method, since that would still be called *after* the buffer is setup (as mentioned above). Note that this order of initialization more closely matches the Windows Terminal implementation, which is one of the reasons why I think it'll be beneficial for #3849. On the whole, the changes needed to make this work look fairly straightforward. The biggest pain is getting the unit tests to work again, because some of them rely on the `IRenderTarget` interface for mocking the renderer, and others depend on the order of initialization. They're all fixable, but it might be a bit messy.
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-Rendering Help Wanted 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#16844
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?