When inserting/deleting lines, preserve RGB/256 attributes #2668
Conversation
This file contains 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
zadjii-msft
added
Product-Conhost
DHowett-MSFT
approved these changes
Sep 5, 2019
ghost
requested review from
miniksa
reviewed
Sep 5, 2019
|
Would it not be worthwhile updating As for the testing, I've added a whole bunch of scrolling tests in PR #2505 which I think could easily be adapted to use 256 colors once this is merged. Although that PR is going to need to be rebased and updated first. |
carlos-zamora
approved these changes
Sep 7, 2019
zadjii-msft
added
the
AutoMerge
|
Hello @zadjii-msft! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
DHowett-MSFT
pushed a commit
that referenced
this pull request
Sep 23, 2019
* This fixes #832 by not mucking with roundtripping attributes. Still needs a test * Add a test * Lets just make this test test everything @miniksa https://media0.giphy.com/media/d7mMzaGDYkz4ZBziP6/giphy.gif * Remove dead code (cherry picked from commit bac69f7)
|
🎉 Handy links: |
ghost
mentioned this pull request
|
How this and same problem in WSL Ubuntu LTS 18.04 bash cmd shell was previously connected ? But if you start Ubuntu bash shell using this Terminal, then vim works fine. |
|
@kshji The Windows Terminal ships a standalone version of conhost.exe within its package that it uses to host console sessions. When you launch "ubuntu bash" directly, it's using the in-box conhost.exe, which was last updated whenever your OS was. When we make changes to the console code (like this PR), the Terminal is able to use those fixes before they're broadly available, because it takes a few weeks for those changes to be ingested by a Windows Insider build. |
3 tasks
5 tasks
|
This process not work so nice as it has told.
|
ghost
pushed a commit
that referenced
this pull request
Dec 10, 2019
## Summary of the Pull Request Operations that erase areas of the screen are typically meant to do so using the current color attributes, but with the rendition attributes reset (what we refer to as meta attributes). This also includes scroll operations that have to clear the area of the screen that has scrolled into view. The only exception is the _Erase Scrollback_ operation, which needs to reset the buffer with the default attributes. This PR updates all of these cases to apply the correct attributes when scrolling and erasing. ## PR Checklist * [x] Closes #2553 * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [x] Tests added/passed * [ ] Requires documentation to be updated * [ ] I've not really discussed this with core contributors. I'm ready to accept this work might be rejected in favor of a different grand plan. ## Detailed Description of the Pull Request / Additional comments My initial plan was to use a special case legacy attribute value to indicate the "standard erase attribute" which could safely be passed through the legacy APIs. But this wouldn't cover the cases that required default attributes to be used. And then with the changes in PR #2668 and #2987, it became clear that our requirements could be better achieved with a couple of new private APIs that wouldn't have to depend on legacy attribute hacks at all. To that end, I've added the `PrivateFillRegion` and `PrivateScrollRegion` APIs to the `ConGetSet` interface. These are just thin wrappers around the existing `SCREEN_INFORMATION::Write` method and the `ScrollRegion` function respectively, but with a simple boolean parameter to choose between filling with default attributes or the standard erase attributes (i.e the current colors but with meta attributes reset). With those new APIs in place, I could then update most scroll operations to use `PrivateScrollRegion`, and most erase operations to use `PrivateFillRegion`. The functions affected by scrolling included: * `DoSrvPrivateReverseLineFeed` (the RI command) * `DoSrvPrivateModifyLinesImpl` (the IL and DL commands) * `AdaptDispatch::_InsertDeleteHelper` (the ICH and DCH commands) * `AdaptDispatch::_ScrollMovement` (the SU and SD commands) The functions affected by erasing included: * `AdaptDispatch::_EraseSingleLineHelper` (the EL command, and most ED variants) * `AdaptDispatch::EraseCharacters` (the ECH command) While updating these erase methods, I noticed that both of them also required boundary fixes similar to those in PR #2505 (i.e. the horizontal extent of the erase operation should apply to the full width of the buffer, and not just the current viewport width), so I've addressed that at the same time. In addition to the changes above, there were also a few special cases, the first being the line feed handling, which required updating in a number of places to use the correct erase attributes: * `SCREEN_INFORMATION::InitializeCursorRowAttributes` - this is used to initialise the rows that pan into view when the viewport is moved down the buffer. * `TextBuffer::IncrementCircularBuffer` - this occurs when we scroll passed the very end of the buffer, and a recycled row now needs to be reinitialised. * `AdjustCursorPosition` - when within margin boundaries, this relies on a couple of direct calls to `ScrollRegion` which needed to be passed the correct fill attributes. The second special case was the full screen erase sequence (`ESC 2 J`), which is handled separately from the other ED sequences. This required updating the `SCREEN_INFORMATION::VtEraseAll` method to use the standard erase attributes, and also required changes to the horizontal extent of the filled area, since it should have been clearing the full buffer width (the same issue as the other erase operations mentioned above). Finally, there was the `AdaptDispatch::_EraseScrollback` method, which uses both scroll and fill operations, which could now be handled by the new `PrivateScrollRegion` and `PrivateFillRegion` APIs. But in this case we needed to fill with the default attributes rather than the standard erase attributes. And again this implementation needed some changes to make sure the full width of the active area was retained after the erase, similar to the horizontal boundary issues with the other erase operations. Once all these changes were made, there were a few areas of the code that could then be simplified quite a bit. The `FillConsoleOutputCharacterW`, `FillConsoleOutputAttribute`, and `ScrollConsoleScreenBufferW` were no longer needed in the `ConGetSet` interface, so all of that code could now be removed. The `_EraseSingleLineDistanceHelper` and `_EraseAreaHelper` methods in the `AdaptDispatch` class were also no longer required and could be removed. Then there were the hacks to handle legacy default colors in the `FillConsoleOutputAttributeImpl` and `ScrollConsoleScreenBufferWImpl` implementations. Since those hacks were only needed for VT operations, and the VT code no longer calls those methods, there was no longer a need to retain that behaviour (in fact there are probably some edge cases where that behaviour might have been considered a bug when reached via the public console APIs). ## Validation Steps Performed For most of the scrolling operations there were already existing tests in place, and those could easily be extended to check that the meta attributes were correctly reset when filling the revealed lines of the scrolling region. In the screen buffer tests, I made updates of that sort to the `ScrollOperations` method (handling SU, SD, IL, DL, and RI), the `InsertChars` and `DeleteChars` methods (ICH and DCH), and the `VtNewlinePastViewport` method (LF). I also added a new `VtNewlinePastEndOfBuffer` test to check the case where the line feed causes the viewport to pan past the end of the buffer. The erase operations, however, were being covered by adapter tests, and those aren't really suited for this kind of functionality (the same sort of issue came up in PR #2505). As a result I've had to reimplement those tests as screen buffer tests. Most of the erase operations are covered by the `EraseTests` method, except the for the scrollback erase which has a dedicated `EraseScrollbackTests` method. I've also had to replace the `HardReset` adapter test, but that was already mostly covered by the `HardResetBuffer` screen buffer test, which I've now extended slightly (it could do with some more checks, but I think that can wait for a future PR when we're fixing other RIS issues).
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 of the Pull Request
Instead of doing our lossy hack to try and roundtrip the current attributes to legacy attrs and back, just straight up call ScrollRegion directly with the current attributes.
References
This may have some affect on #2553. It certainly doesn't fix it, but it might make that issue's solution easier.
PR Checklist
Detailed Description of the Pull Request / Additional comments
Why didn't we do it this way in the first place? Simply put, the console has been a pretty heavily evolving codebase over the last four years. It wasn't until recently that
ScrollRegionwas updated to accept a fullTextAttribute(as opposed to aWORD). Frankly, the ability to use aTextAttributein that function means we can probably get rid of a bunch of similar hacks that do the same thing.Validation Steps Performed
Ran the tests
Tried the simple repro from the issue.
Tried the more complicated vim repro from the OP.