Skip to content

Commit

Permalink
I think this is a valid test for #8663
Browse files Browse the repository at this point in the history
  • Loading branch information
zadjii-msft committed Feb 2, 2022
1 parent e687d17 commit 7ac2d6d
Show file tree
Hide file tree
Showing 4 changed files with 86 additions and 6 deletions.
10 changes: 10 additions & 0 deletions src/host/directio.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,16 @@ void EventsToUnicode(_Inout_ std::deque<std::unique_ptr<IInputEvent>>& inEvents,
try
{
SplitToOem(readEvents);

// After SplitToOem returns, readEvents might be bigger than it
// was before. It might be more than 2x the size it was before,
// because a single codepoint might have been expanded into more
// that a single char.
//
// As of GH #8663, InputBuffer::Read will have pre-emptively
// checked how much space each key is about to take up, and will
// only return as many as will fit in readBuffer _after_ a call
// to SplitToOem.
}
CATCH_LOG();
}
Expand Down
9 changes: 6 additions & 3 deletions src/host/inputBuffer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -434,10 +434,13 @@ void InputBuffer::_ReadBuffer(_Out_ std::deque<std::unique_ptr<IInputEvent>>& ou
}
}
}
// TODO!: If we do this, then readers who come asking for 4 events might
// When we do this, then readers who come asking for 4 events might
// only get 3 back, under the implication that they know they need to
// expand the events themselves. Who are all the InputBuffer::Read
// callers? Are they all expanding (in the case of !unicode) in post?
// expand the events themselves. There are only two callers:
// * DirectReadData::Notify
// * _DoGetConsoleInput
//
// and they both handle !unicode the same way.

if (performNormalRead)
{
Expand Down
9 changes: 9 additions & 0 deletions src/host/readDataDirect.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,15 @@ bool DirectReadData::Notify(const WaitTerminationReason TerminationReason,
try
{
SplitToOem(readEvents);
// After SplitToOem returns, readEvents might be bigger than it
// was before. It might be more than 2x the size it was before,
// because a single codepoint might have been expanded into more
// that a single char.
//
// As of GH #8663, InputBuffer::Read will have pre-emptively
// checked how much space each key is about to take up, and will
// only return as many as will fit in readBuffer _after_ a call
// to SplitToOem.
}
CATCH_LOG();
}
Expand Down
64 changes: 61 additions & 3 deletions src/host/ut_host/InputBufferTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -434,6 +434,17 @@ class InputBufferTests
std::deque<std::unique_ptr<IInputEvent>> outEvents;
size_t eventsRead = 0;
bool resetWaitEvent = false;

// GH #8663: We only insert 4 events. but we need to ask for 6 here.
// The Raised Hand emoji is U+270B in utf16, but it's 0xE2 0x9C 0x8B in utf-8.
Log::Comment(fmt::format(L"Codepage: {}", ServiceLocator::LocateGlobals().getConsoleInformation().CP).c_str());
ServiceLocator::LocateGlobals().getConsoleInformation().CP = 932;
Log::Comment(fmt::format(L"Changed to: {}", ServiceLocator::LocateGlobals().getConsoleInformation().CP).c_str());

// GH #8663: We only insert 4 events. When we ask for 4 here., we'll only get 3.
// InpuBuffer::_ReadBuffer, when called with !unicode, will only return
// as many events as _will_ fit into readCount, assuming the caller
// expands them with SplitToOem.
inputBuffer._ReadBuffer(outEvents,
recordInsertCount,
eventsRead,
Expand All @@ -442,12 +453,59 @@ class InputBufferTests
false,
false);

// TODO! I broke this test. This test expects 4 events to get read back, regardless of the fact that one would get expanded by ConvertToA.
// This might be okay, the test probably shouldn't be calling the private method directly.
// the dbcs record should have counted for two elements in
// the array, making it so that we get less events read
VERIFY_ARE_EQUAL(recordInsertCount - 1, eventsRead);
VERIFY_ARE_EQUAL(eventsRead, outEvents.size());
for (size_t i = 0; i < eventsRead; ++i)
{
VERIFY_ARE_EQUAL(outEvents[i]->ToInputRecord(), inRecords[i]);
}
}

TEST_METHOD(ReadingDbcsCharsPadsOutputArrayForEmoji)
{
Log::Comment(L"During a utf-8 read, make sure the input buffer leaves "
L"enough room for keys that could be expanded into more than two chars.");

// write a mouse event, key event, dbcs key event, mouse event
InputBuffer inputBuffer;
const unsigned int recordInsertCount = 4;
INPUT_RECORD inRecords[recordInsertCount];
inRecords[0].EventType = MOUSE_EVENT;
inRecords[1] = MakeKeyEvent(TRUE, 1, L'A', 0, L'A', 0);
inRecords[2] = MakeKeyEvent(TRUE, 1, 0x270B, 0, 0x270B, 0); // U+270B RAISED HAND
inRecords[3].EventType = MOUSE_EVENT;

std::deque<std::unique_ptr<IInputEvent>> inEvents;
for (size_t i = 0; i < recordInsertCount; ++i)
{
inEvents.push_back(IInputEvent::Create(inRecords[i]));
}

inputBuffer.Flush();
VERIFY_IS_GREATER_THAN(inputBuffer.Write(inEvents), 0u);

// read them out non-unicode style and compare
std::deque<std::unique_ptr<IInputEvent>> outEvents;
size_t eventsRead = 0;
bool resetWaitEvent = false;

// GH #8663: We only insert 4 events. but we need to ask for 6 here.
// The Raised Hand emoji is U+270B in utf16, but it's 0xE2 0x9C 0x8B in utf-8.
ServiceLocator::LocateGlobals().getConsoleInformation().CP = 65001;

inputBuffer._ReadBuffer(outEvents,
5,
eventsRead,
false,
resetWaitEvent,
false,
false);

// the dbcs record should have counted for two elements in
// the array, making it so that we get less events read
VERIFY_ARE_EQUAL(eventsRead, recordInsertCount - 1);
VERIFY_ARE_EQUAL(3, eventsRead);
VERIFY_ARE_EQUAL(eventsRead, outEvents.size());
for (size_t i = 0; i < eventsRead; ++i)
{
Expand Down

1 comment on commit 7ac2d6d

@github-actions

This comment was marked as outdated.

Please sign in to comment.