Add support for Insert-Replace Mode (IRM) and SM/RM #8689

DHowett · 2021-01-02T03:50:36Z

This pull request adds support for IRM. I chose a brute-force strategy:
every time we insert text into a row, back up the characters and
attributes and stamp them down at the end of the inserted range.

Pros:

Easy
Fast implementation
Relies on preexisting DBCS lead/trail calculation and pre-formed
CharRowCells

Cons:

Applies brute force
Expensive in terms of allocations, especially when inserting only a
single character. Most output inserted is going to be on the order
of single characters. 120 single characters, inserted one by one,
with a memcpy() after each one.

I consider the cons to be acceptable enough for this scenario. It's rare,
and it seems like the most common use is in interactive shells where we
need to operate at "user speed". We can always optimize it later.

I chose to give SCREEN_INFORMATION knowledge of whether it was in
insert mode, rather than the text buffer, adapter or any other
component. When insert mode is on, the text buffer is instructed to
insert--a request it passes directly on to ROW. I've tried to keep
branch misprediction to a minimum (with some optimizer hints) and reduce
default-case allocations¹.

¹ by using unique_ptr<...> to store copied CharRow and ATTR_ROW,
s.t. there is no cost in the normal case except 2*sizeof(void*) bytes
on the stack in ROW::WriteCells

Closes #1947

Validation

Tests for IRM must be added
Tests for cool new ATTR_ROW stuff added, pass

DHowett · 2021-01-02T03:52:41Z

src/buffer/out/Row.cpp

 {
    THROW_HR_IF(E_INVALIDARG, index >= _charRow.size());
    THROW_HR_IF(E_INVALIDARG, limitRight.value_or(0) >= _charRow.size());
    size_t currentIndex = index;

    // If we're given a right-side column limit, use it. Otherwise, the write limit is the final column index available in the char row.
    const auto finalColumnInRow = limitRight.value_or(_charRow.size() - 1);
+    const auto iteratorEmitsText = it && it->TextAttrBehavior() != TextAttributeBehavior::StoredOnly;
+    insert = insert && iteratorEmitsText; // we only support insert mode for textful iterators


CAVEAT: we should be able to insert attributes, but... nobody has asked for this, so why bother?

if by attribute you mean blink, underline, bold, double wide, or double height, then it's what the ANSI terminal protocol requires, and waiting for somebody to ask for it is probably the wrong approach.

if by attribute you mean something internal that doesn't impact ANSI compatibility, then i'd agree, "why bother?"

see also: https://invisible-island.net/vttest/vttest.html

Nope, this is an implementation detail.

This writer supports text, attributes (ANSI as well as Windows console-specific flags), and combined text+attributes. Attribute-only writing changes the style of text without changing the text itself, and is used to support some Win32 console APIs such as FillConsoleOutputAttribute.

Since there’s no need for us to do an insert operation on attributes only without simultaneously manipulating the text stored in the buffer, we can avoid the complexity. It would produce an interesting effect, but I am not aware of any VT or Console requirement to do so.

DHowett · 2021-01-02T03:53:11Z

src/buffer/out/Row.cpp

@@ -97,17 +97,29 @@ const UnicodeStorage& ROW::GetUnicodeStorage() const noexcept
 // - limitRight - right inclusive column ID for the last write in this row. (optional, will just write to the end of row if nullopt)
 // Return Value:
 // - iterator to first cell that was not written to this row.
-OutputCellIterator ROW::WriteCells(OutputCellIterator it, const size_t index, const std::optional<bool> wrap, std::optional<size_t> limitRight)
+OutputCellIterator ROW::WriteCells(OutputCellIterator it, const size_t index, const std::optional<bool> wrap, std::optional<size_t> limitRight, bool insert)


DESIGN QUESTION: Should insertion be a property on the OutputCellIterator instead of forced down into the row?

DHowett · 2021-01-02T03:53:37Z

src/buffer/out/Row.cpp

+        std::unique_ptr<CharRow> backupCharRow;
+        std::unique_ptr<ATTR_ROW> backupAttrRow;


NOTE: Memory profiler shows this causes 0 allocations, but it does incur a minor stack cost.

DHowett · 2021-01-02T03:54:53Z

src/host/screenInfo.cpp

@@ -2309,6 +2310,11 @@ OutputCellRect SCREEN_INFORMATION::ReadRect(const Viewport viewport) const
 // - will throw exception on error.
 OutputCellIterator SCREEN_INFORMATION::Write(const OutputCellIterator it)
 {
+    __assume(!_insertMode);


NOTE: This did nothing in my testing, but it makes me feel better. The optimizer actually deleted this function and TextBuffer::Write (and ...::Insert (!)) because it saw right through our trick. It lifted the boolean load from SCREEN_INFORMATION::_insertMode directly into WriteCharsLegacy and passed it down into the fully-realized ROW::Write(..., insert) (!)

vixie · 2021-01-02T22:15:28Z

in case my earlier comment on one of the individual changes isn't noticed there, i'll repeat something here:

i hope that this PR will be gated by successful completion of the vttest suite, including its insert/delete character/line tests.

https://invisible-island.net/vttest/vttest.html

DHowett · 2021-01-02T22:29:21Z

vttest (@vixie)

of course! 😄

DHowett · 2021-01-02T22:47:20Z

Sorry, now that I'm at a computer I can explain a bit better.

Attributes and attribute insertion

The console buffer implementation (shared with Windows Terminal) stores text and "attributes" separately. Attributes are everything you'd expect--graphic renditions as well as cell size (potentially; pending #8664)--and some things you wouldn't, like whether there's an OSC 8 hyperlink or what "grid lines"² are enabled.

Today, the Write method where the bulk of the changes were made supports overstriking text with text, attributes with attributes, or entire cells with text+attributes. This is the backbone for FillConsoleOutputCharacter (writes text only) and FillConsoleOutputAttribute (writes attributes only), and it also handles every other write operation (which is usually both text and attributes simultaneously.)

The caveat noted above is for the following scenario:

ORIGINAL
| Hello World                |
| RED  | GREEN               |
0                           29

Insert attributes only: BLUE (length=5); position 0

NEW
| Hello World                |
|BLUE | RED  | GREEN         |
0                           29

Since it only changes the attributes, " Hello" would remain, but the colors would shift underneath it such that " Hell" was in blue and "o World" was in red. Nothing in the literature suggests that we'll need to support it. Every change to the buffer coming from VT is accompanied by text.

² The Win32 console supports drawing borders around every cell; the bottom border is distinct from underline, the top border is recycled for overline, and the left/right borders are used in some CJK composition scenarios.

vttest

edit: thanks for reviewing!

DHowett · 2021-01-04T06:40:57Z

This implementation will absolutely hork UnicodeStorage. It tracks non-dbcs high unicode by row/column index. Shuffling the backing store for the row but not updating UnicodeStorage will make it blast into outer space.

We are likely overdue for a UnicodeStorage redesign.

vixie

i don't have a Windows software build environment, so i'm not in a position to test this PR. is there a .EXE i can try?

DHowett · 2021-04-26T23:22:12Z

@vixie sorry, we ended up running into some of our longstanding code rot and this work is, as a result, a bit delayed. I'm keeping it on the docket for 2021 such that we can finally put it to bed, and will keep this thread updated.

Once we're in a place where what you test is close to what we want to ship, I'll share a build of conhost/openconsole (without any of the terminal bits) to test with.
d

hellow554 · 2021-10-27T06:05:15Z

What happened to that poor bot?

zadjii-msft · 2021-10-27T11:04:40Z

@hellow554 This branch is pretty stale, I'm betting the spellcheck rules in this branch don't quite work for the updated version of the bot

WSLUser · 2022-06-29T14:35:09Z

Any chance this sequence will be updated with the latest changes? I think this would be a useful sequence to support.

DHowett · 2022-06-29T17:22:19Z

So, the significant blocker for IRM is that we didn't have a good way to shift our backing store character-by-character to handle insertion (due to some legacy baggage that we had accumulated over time ("UnicodeStorage", which is a hash map of coordinate positions to non-UCS-2 code units)). That's being actively worked on in #8000. Once that lands, we can dust this off.

zadjii-msft · 2022-07-05T14:07:58Z

@DHowett since the implementation of this is likely to change aggressively after #8000 lands, should we close out this draft for now?

DHowett · 2022-07-05T21:04:05Z

Nah, you're totally right. I can't land this in its current form, and it's just clogging the list up until it can land. I'll keep the branch.

vixie · 2022-10-11T07:02:35Z

Mike Griese wrote on 2022-07-05 07:08:

since the implementation of this is likely to change aggressively after #8000 lands, should we close out this draft for now?

if Terminal would stop identifying itself as "xterm" and instead answer as "vt100", then we could postpone implementing IRM and SM/RM until after #8000 lands. Terminal isn't "xterm" and that problem can be fixed several ways.

…

-- P Vixie

vixie · 2022-12-25T00:18:20Z

given this has been open for more than two years and that it's blocked on #8000 which has also been open for two years, can i ask that the TERM variable be set to something other than "xterm-256color" please? that's false, since xterm has insert mode, and this terminal emulator does not. if you won't change it to "vt220" then at least add a configuration setting for TERM. you can revert this when and if this terminal emulator ever actually becomes xterm compatible.

DHowett added 6 commits January 1, 2021 16:55

IRM

d07773a

Add tests for RunsInRange

b53cc32

fixup comment on GetAttrRunsInRan

5676d50

SM/RM

3a80c4b

Some tidying

bbb9876

Migrate spelling-0.0.19 changes from main

de12236

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 2, 2021

DHowett commented Jan 2, 2021

View reviewed changes

DHowett mentioned this pull request Jan 2, 2021

Add support for VT102 Insertion-Replace Mode (IRM) #1947

Closed

DHowett mentioned this pull request Jan 12, 2021

[Epic] Text Buffer rewrite #8000

Closed

15 tasks

vixie reviewed Apr 26, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

DHowett closed this Jun 29, 2022

DHowett reopened this Jun 29, 2022

DHowett closed this Jul 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Insert-Replace Mode (IRM) and SM/RM #8689

Add support for Insert-Replace Mode (IRM) and SM/RM #8689

DHowett commented Jan 2, 2021 •

edited

Loading

DHowett Jan 2, 2021

vixie Jan 2, 2021

DHowett Jan 2, 2021 •

edited

Loading

DHowett Jan 2, 2021

DHowett Jan 2, 2021

DHowett Jan 2, 2021 •

edited

Loading

vixie commented Jan 2, 2021

DHowett commented Jan 2, 2021

DHowett commented Jan 2, 2021 •

edited

Loading

DHowett commented Jan 4, 2021

vixie left a comment

DHowett commented Apr 26, 2021

This comment has been minimized.

hellow554 commented Oct 27, 2021

zadjii-msft commented Oct 27, 2021

WSLUser commented Jun 29, 2022 •

edited

Loading

DHowett commented Jun 29, 2022

zadjii-msft commented Jul 5, 2022

DHowett commented Jul 5, 2022

vixie commented Oct 11, 2022 via email

vixie commented Dec 25, 2022

		std::unique_ptr<CharRow> backupCharRow;
		std::unique_ptr<ATTR_ROW> backupAttrRow;

Add support for Insert-Replace Mode (IRM) and SM/RM #8689

Add support for Insert-Replace Mode (IRM) and SM/RM #8689

Conversation

DHowett commented Jan 2, 2021 • edited Loading

Validation

DHowett Jan 2, 2021

Choose a reason for hiding this comment

vixie Jan 2, 2021

Choose a reason for hiding this comment

DHowett Jan 2, 2021 • edited Loading

Choose a reason for hiding this comment

DHowett Jan 2, 2021

Choose a reason for hiding this comment

DHowett Jan 2, 2021

Choose a reason for hiding this comment

DHowett Jan 2, 2021 • edited Loading

Choose a reason for hiding this comment

vixie commented Jan 2, 2021

DHowett commented Jan 2, 2021

DHowett commented Jan 2, 2021 • edited Loading

Attributes and attribute insertion

vttest

DHowett commented Jan 4, 2021

vixie left a comment

Choose a reason for hiding this comment

DHowett commented Apr 26, 2021

This comment has been minimized.

hellow554 commented Oct 27, 2021

zadjii-msft commented Oct 27, 2021

WSLUser commented Jun 29, 2022 • edited Loading

DHowett commented Jun 29, 2022

zadjii-msft commented Jul 5, 2022

DHowett commented Jul 5, 2022

vixie commented Oct 11, 2022 via email

vixie commented Dec 25, 2022

DHowett commented Jan 2, 2021 •

edited

Loading

DHowett Jan 2, 2021 •

edited

Loading

DHowett Jan 2, 2021 •

edited

Loading

DHowett commented Jan 2, 2021 •

edited

Loading

WSLUser commented Jun 29, 2022 •

edited

Loading