Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for IRM (Insert Replace Mode) #14700

Merged
5 commits merged into from
Jan 19, 2023
Merged

Conversation

j4james
Copy link
Collaborator

@j4james j4james commented Jan 18, 2023

This PR add support for the ANSI Insert/Replace mode (IRM), which
determines whether output characters are inserted at the active cursor
position, moving existing content to the right, or whether they should
overwrite the content that is already there.

The implementation is a bit of a hack. When that mode is enabled, it
first measures how many cells the string is expected to occupy, then
scrolls the target line right by that amount before writing out the new
text.

In the longer term it might be better if this was implemented entirely
in the TextBuffer itself, so the scrolling could take place at the
same time as the content was being written.

Validation Steps Performed

I've added a very basic unit test that verifies the mode is working as
expected. But I've also done a lot more manual testing, confirming edge
cases like wide characters, double-width lines, and both with and
without wrapping mode enabled.

Closes #1947

@ghost ghost added Area-VT Virtual Terminal sequence support Issue-Task It's a feature request, but it doesn't really need a major design. Product-Conhost For issues in the Console codebase labels Jan 18, 2023
@lhecker
Copy link
Member

lhecker commented Jan 18, 2023

Here's my approach: https://github.com/microsoft/terminal/compare/dev/lhecker/1947-irm
It contains a lot more fluff than your PR, which is why I prefer yours. TextBuffer::InsertLine is the relevant function.

The reason it contains more "fluff" is because I initially tried to implement IRM with proper Unicode support in mind (which means that we need to avoid remeasuring strings) and resulted in a rather complex changeset since it requires a generic text replacement and insertion function. To get it done faster, I removed that code and replaced it with a simpler one that remeasures strings via OutputCellIterator. Still, two new functions remained: ROW::GetText and ROW:CopyTo. But I need the former in the future anyways in order to be able to remove our many buffer iterators (and simplifying text extraction) and the latter to fix #14696 & to implement buffer snapshotting.

...but I failed to implement moving the text attributes until now, whereas your code already (presumably) works flawlessly, while being a rather simple changeset as well. I like it! If you see anything useful in my branch, please feel free to use it. 😊

@lhecker lhecker added this to the Terminal v1.17 milestone Jan 19, 2023
@DHowett
Copy link
Member

DHowett commented Jan 19, 2023

I am 100% on board for this, it's so delightfully simple

@j4james
Copy link
Collaborator Author

j4james commented Jan 19, 2023

@lhecker Your branch is doing almost exactly what I hoped you'd be doing. So if we merge this PR first, it would ideal if you could do a follow up at some point that just replaces my _ScrollRectHorizontally hack with your InsertLine call once it's ready, because I'm assuming it should be a lot more efficient.

I haven't looked at your actual implementation in much detail, but the one key thing that I think is still missing from the InsertLine interface is the ability to specify the line width. It's important that AdaptDispatch has control of that in order to support margins in the future.

And I think we might also need to be able to pass in the wrap flag. It's not obvious what the expected behavior should be with IRM enabled, but it seemed reasonable to me that we should handle that the same as replace mode. I should probably test on some other terminals and see what they do.

@j4james j4james marked this pull request as ready for review January 19, 2023 14:17
@DHowett
Copy link
Member

DHowett commented Jan 19, 2023

but it seemed reasonable to me that we should handle that the same as replace mode. I should probably test on some other terminals and see what they do.

Agreed. I originally looked at how gnome-terminal handled it, and i wasn't terribly surprised by the results. If I recall, it kept the wrap flag on rows up until you got to the end of the line, at which point normal newline/wrapping behavior took over.

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nit, but I'm just blown away by how the foundation you've been laying over the past year has made this look so easy. Leonard as well, of course 🙂

@@ -395,6 +395,7 @@ namespace Microsoft::Console::VirtualTerminal::DispatchTypes

enum ModeParams : VTInt
{
IRM_InsertReplaceMode = ANSIStandardMode(4),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love that my weird "tagged enum" thing didn't turn into YAGNI. I added it because we'd eventually do IRM, and need SM/RM. 😄

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's definitely been useful. I used it for differentiating the ANSI and private DSR operations as well.

src/terminal/parser/telemetry.cpp Outdated Show resolved Hide resolved
src/terminal/parser/telemetry.cpp Outdated Show resolved Hide resolved
Co-authored-by: Dustin L. Howett <dustin@howett.net>
@DHowett DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jan 19, 2023
@ghost
Copy link

ghost commented Jan 19, 2023

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

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 (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 7813953 into microsoft:main Jan 19, 2023
@j4james j4james deleted the feature-irm branch January 19, 2023 20:51
@ghost
Copy link

ghost commented Jan 24, 2023

🎉Windows Terminal Preview v1.17.1023 has been released which incorporates this pull request.:tada:

Handy links:

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-VT Virtual Terminal sequence support AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Task It's a feature request, but it doesn't really need a major design. Product-Conhost For issues in the Console codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for VT102 Insertion-Replace Mode (IRM)
4 participants