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

[PR #12568] [MERGED] Eliminate the IRenderTarget interface #29112

New Issue
Open
opened 2026-01-31 09:32:53 +00:00 by claunia · 0 comments
claunia commented 2026-01-31 09:32:53 +00:00
Owner
Copy Link

📋 Pull Request Information

Original PR: https://github.com/microsoft/terminal/pull/12568
Author: @j4james
Created: 2/24/2022
Status: ✅ Merged
Merged: 3/14/2022
Merged by: @undefined

Base: main ← Head: nuke-rendertarget

📝 Commits (8)

  • fd38db6 Track active buffer status in TextBuffer.
  • beb0963 Get rid of SCREEN_INFORMATION::GetRenderTarget.
  • 167c5f8 Avoid using GetRenderTarget when triggering redraws.
  • d53f509 Replace IRenderTarget with Renderer in TextBuffer.
  • a603951 Get the unit tests working.
  • a5cc853 Remove remaining usage of IRenderTarget.
  • b3fb6e8 Merge branch 'main' into nuke-rendertarget
  • 76e4487 Add comments clarifying the order of initialization.

📊 Changes

45 files changed (+435 additions, -535 deletions)

View changed files

📝 src/buffer/out/cursor.cpp (+1 -1)
📝 src/buffer/out/textBuffer.cpp (+77 -23)
📝 src/buffer/out/textBuffer.hpp (+20 -7)
📝 src/buffer/out/ut_textbuffer/ReflowTests.cpp (+5 -5)
📝 src/cascadia/TerminalControl/ControlCore.cpp (+1 -2)
📝 src/cascadia/TerminalCore/Terminal.cpp (+15 -14)
📝 src/cascadia/TerminalCore/Terminal.hpp (+3 -2)
📝 src/cascadia/TerminalCore/TerminalApi.cpp (+2 -2)
📝 src/cascadia/TerminalCore/TerminalSelection.cpp (+1 -1)
📝 src/cascadia/TerminalCore/terminalrenderdata.cpp (+1 -1)
📝 src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp (+7 -5)
📝 src/cascadia/UnitTests_TerminalCore/InputTest.cpp (+3 -3)
📝 src/cascadia/UnitTests_TerminalCore/ScreenSizeLimitsTest.cpp (+12 -13)
📝 src/cascadia/UnitTests_TerminalCore/ScrollTest.cpp (+46 -26)
📝 src/cascadia/UnitTests_TerminalCore/SelectionTest.cpp (+45 -45)
📝 src/cascadia/UnitTests_TerminalCore/TerminalApiTest.cpp (+21 -21)
📝 src/cascadia/UnitTests_TerminalCore/TerminalBufferTests.cpp (+5 -3)
📝 src/host/CursorBlinker.cpp (+1 -1)
➖ src/host/ScreenBufferRenderTarget.cpp (+0 -122)
➖ src/host/ScreenBufferRenderTarget.hpp (+0 -46)

...and 25 more files

📄 Description

Summary of the Pull Request

The purpose of the IRenderTarget interface was to support the concept
of multiple buffers in conhost. When a text buffer needed to trigger a
redraw, the render target implementation would be responsible for
deciding whether to forward that redraw to the renderer, depending on
whether the buffer was active or not.

This PR instead introduces a flag in the TextBuffer class to track
whether it is active or not, and it can then simply check the flag to
decide whether it needs to trigger a redraw or not. That way it can work
with the Renderer directly, and we can avoid a bunch of virtual calls
for the various redraw methods.

PR Checklist

Detailed Description of the Pull Request / Additional comments

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

For this to work, though, the Renderer needed to be available before
the TextBuffer could be constructed, which required a change in the
conhost initialization order. In the ConsoleInitializeConnectInfo
function, I had to move the Renderer initialization up so it could be
constructed before the call to SetUpConsole (which initializes the
buffer). Once both are ready, the Renderer::EnablePainting method is
called to start the render thread.

The other catch is that the renderer used to setup its initial viewport
in the constructor, but with the new initialization order, the viewport
size would not be known at that point. I've addressed that problem by
moving the viewport initialization into the EnablePainting method,
since that will only be called after the buffer is setup.

Validation Steps Performed

The changes in architecture required a number of tweaks to the unit
tests. Some were using a dummy IRenderTarget implementation which now
needed to be replaced with a full Renderer (albeit with mostly null
parameters). In the case of the scroll test, a mock IRenderTarget was
used to track the scroll delta, which now had to be replaced with a mock
RenderEngine instead.

Some tests previously relied on having just a TextBuffer but without a
Renderer, and now they require both. And tests that were constructing
the TextBuffer and Renderer themselves had to be updated to use the
new initialization order, i.e. with the renderer constructed first.

Semantically, though, the tests still essentially work the same way as
they did before, and they all still pass.

🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.

## 📋 Pull Request Information **Original PR:** https://github.com/microsoft/terminal/pull/12568 **Author:** [@j4james](https://github.com/j4james) **Created:** 2/24/2022 **Status:** ✅ Merged **Merged:** 3/14/2022 **Merged by:** [@undefined](undefined) **Base:** `main` ← **Head:** `nuke-rendertarget` --- ### 📝 Commits (8) - [`fd38db6`](https://github.com/microsoft/terminal/commit/fd38db6b4bc6b280ae901346477d1b3888fb2805) Track active buffer status in TextBuffer. - [`beb0963`](https://github.com/microsoft/terminal/commit/beb0963414cb6bbe95629a4758f30c6f9ecf2ef7) Get rid of SCREEN_INFORMATION::GetRenderTarget. - [`167c5f8`](https://github.com/microsoft/terminal/commit/167c5f8198d9c3132e6471f11d2fcec24d2d87ae) Avoid using GetRenderTarget when triggering redraws. - [`d53f509`](https://github.com/microsoft/terminal/commit/d53f509a3f2542d3b4e4e3c2c26d010c37616e43) Replace IRenderTarget with Renderer in TextBuffer. - [`a603951`](https://github.com/microsoft/terminal/commit/a603951f1c82a12b3acee82d71715b33e8128369) Get the unit tests working. - [`a5cc853`](https://github.com/microsoft/terminal/commit/a5cc853b5ea445aa23a87eaf20c760dfdceff36c) Remove remaining usage of IRenderTarget. - [`b3fb6e8`](https://github.com/microsoft/terminal/commit/b3fb6e867ee3eac368dc62e1d11ab9d1f7ee07cd) Merge branch 'main' into nuke-rendertarget - [`76e4487`](https://github.com/microsoft/terminal/commit/76e4487e85c8bdf8550452deb2b3e22765ebf879) Add comments clarifying the order of initialization. ### 📊 Changes **45 files changed** (+435 additions, -535 deletions) <details> <summary>View changed files</summary> 📝 `src/buffer/out/cursor.cpp` (+1 -1) 📝 `src/buffer/out/textBuffer.cpp` (+77 -23) 📝 `src/buffer/out/textBuffer.hpp` (+20 -7) 📝 `src/buffer/out/ut_textbuffer/ReflowTests.cpp` (+5 -5) 📝 `src/cascadia/TerminalControl/ControlCore.cpp` (+1 -2) 📝 `src/cascadia/TerminalCore/Terminal.cpp` (+15 -14) 📝 `src/cascadia/TerminalCore/Terminal.hpp` (+3 -2) 📝 `src/cascadia/TerminalCore/TerminalApi.cpp` (+2 -2) 📝 `src/cascadia/TerminalCore/TerminalSelection.cpp` (+1 -1) 📝 `src/cascadia/TerminalCore/terminalrenderdata.cpp` (+1 -1) 📝 `src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp` (+7 -5) 📝 `src/cascadia/UnitTests_TerminalCore/InputTest.cpp` (+3 -3) 📝 `src/cascadia/UnitTests_TerminalCore/ScreenSizeLimitsTest.cpp` (+12 -13) 📝 `src/cascadia/UnitTests_TerminalCore/ScrollTest.cpp` (+46 -26) 📝 `src/cascadia/UnitTests_TerminalCore/SelectionTest.cpp` (+45 -45) 📝 `src/cascadia/UnitTests_TerminalCore/TerminalApiTest.cpp` (+21 -21) 📝 `src/cascadia/UnitTests_TerminalCore/TerminalBufferTests.cpp` (+5 -3) 📝 `src/host/CursorBlinker.cpp` (+1 -1) ➖ `src/host/ScreenBufferRenderTarget.cpp` (+0 -122) ➖ `src/host/ScreenBufferRenderTarget.hpp` (+0 -46) _...and 25 more files_ </details> ### 📄 Description ## Summary of the Pull Request The purpose of the `IRenderTarget` interface was to support the concept of multiple buffers in conhost. When a text buffer needed to trigger a redraw, the render target implementation would be responsible for deciding whether to forward that redraw to the renderer, depending on whether the buffer was active or not. This PR instead introduces a flag in the `TextBuffer` class to track whether it is active or not, and it can then simply check the flag to decide whether it needs to trigger a redraw or not. That way it can work with the `Renderer` directly, and we can avoid a bunch of virtual calls for the various redraw methods. ## PR Checklist * [x] Closes #12551 * [x] CLA signed. * [ ] Tests added/passed * [ ] Documentation updated. * [ ] Schema updated. * [x] I've discussed this with core contributors already. Issue number where discussion took place: #12551 ## Detailed Description of the Pull Request / Additional comments Anywhere that had previously been getting an `IRenderTarget` from the `TextBuffer` class in order to trigger a redraw, will now just call the `TriggerRedraw` method on the `TextBuffer` class itself, since that will handle the active check which used to be the responsibility of the render target. All the redraw methods that were in `IRenderTarget` are now exposed in `TextBuffer` instead. For this to work, though, the `Renderer` needed to be available before the `TextBuffer` could be constructed, which required a change in the conhost initialization order. In the `ConsoleInitializeConnectInfo` function, I had to move the `Renderer` initialization up so it could be constructed before the call to `SetUpConsole` (which initializes the buffer). Once both are ready, the `Renderer::EnablePainting` method is called to start the render thread. The other catch is that the renderer used to setup its initial viewport in the constructor, but with the new initialization order, the viewport size would not be known at that point. I've addressed that problem by moving the viewport initialization into the `EnablePainting` method, since that will only be called after the buffer is setup. ## Validation Steps Performed The changes in architecture required a number of tweaks to the unit tests. Some were using a dummy `IRenderTarget` implementation which now needed to be replaced with a full `Renderer` (albeit with mostly null parameters). In the case of the scroll test, a mock `IRenderTarget` was used to track the scroll delta, which now had to be replaced with a mock `RenderEngine` instead. Some tests previously relied on having just a `TextBuffer` but without a `Renderer`, and now they require both. And tests that were constructing the `TextBuffer` and `Renderer` themselves had to be updated to use the new initialization order, i.e. with the renderer constructed first. Semantically, though, the tests still essentially work the same way as they did before, and they all still pass. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
claunia added the pull-request label 2026-01-31 09:32:53 +00:00
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 pull-request
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#29112
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?