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.

Sign up for GitHub

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

Jump to bottom

Add a system message to session restore #17113

Merged
merged 3 commits into from Apr 24, 2024
Merged

Add a system message to session restore #17113

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
73a225a
Add a system message to session restore
lhecker Apr 23, 2024
8d1b45c
Address feedback
lhecker Apr 23, 2024
72850eb
Address feedback
lhecker Apr 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/actions/spelling/expect/expect.txt
View file
Open in desktop
Expand Up @@ -1718,6 +1718,7 @@ sysparams
sysparamsext
SYSTEMHAND
SYSTEMMENU
SYSTEMTIME
tabview
TAdd
taef
Expand Down
46 changes: 43 additions & 3 deletions src/cascadia/TerminalControl/ControlCore.cpp
View file
Open in desktop
Expand Up @@ -1796,6 +1796,38 @@ namespace winrt::Microsoft::Terminal::Control::implementation
return;
}

FILETIME lastWriteTime;
SYSTEMTIME lastWriteSystemTime;
if (!GetFileTime(file.get(), nullptr, nullptr, &lastWriteTime) ||
!FileTimeToSystemTime(&lastWriteTime, &lastWriteSystemTime))
{
return;
Copy link
Member

Choose a reason for hiding this comment

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

should we explode entirely if this fails? or just skip printing the "[restored ]" message?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I think this is fine. If either of these fails, the file is probably not safe to read anyway.

}

wchar_t dateBuf[256];
const auto dateLen = GetDateFormatEx(nullptr, 0, &lastWriteSystemTime, nullptr, &dateBuf[0], ARRAYSIZE(dateBuf), nullptr);
wchar_t timeBuf[256];
const auto timeLen = GetTimeFormatEx(nullptr, 0, &lastWriteSystemTime, nullptr, &timeBuf[0], ARRAYSIZE(timeBuf));

std::wstring message;
if (dateLen > 0 && timeLen > 0)
{
const auto msg = RS_(L"SessionRestoreMessage");
const std::wstring_view date{ &dateBuf[0], gsl::narrow_cast<size_t>(dateLen) };
const std::wstring_view time{ &timeBuf[0], gsl::narrow_cast<size_t>(timeLen) };
// This escape sequence string
// * sets the color to white on a bright black background ("\x1b[100;37m")
// * prints " [Restored <date> <time>] <spaces until end of line> "
// * resets the color ("\x1b[m")
// * newlines
// * clears the screen ("\x1b[2J")
// The last step is necessary because we launch ConPTY without PSEUDOCONSOLE_INHERIT_CURSOR by default.
// This will cause ConPTY to emit a \x1b[2J sequence on startup to ensure it and the terminal are in-sync.
// If we didn't do a \x1b[2J ourselves as well, the user would briefly see the last state of the terminal,
// before it's quickly scrolled away once ConPTY has finished starting up, which looks weird.
message = fmt::format(FMT_COMPILE(L"\x1b[100;37m [{} {} {}]\x1b[K\x1b[m\r\n\x1b[2J"), msg, date, time);
Copy link
Member

Choose a reason for hiding this comment

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

Should we also start with a \n, so that the message always gets printed on a newline? (I'm imagining a scenario where the user was at the prompt, so the scrollback now looks like

C:\users\me>some-long --command [Restored 1/23/45 6:07])
Microsoft Windows [Version 10.0.26201.5000]
(c) Microsoft Corporation. All rights reserved.

C:\users\me>

Copy link
Member Author

Choose a reason for hiding this comment

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

The serializer will always end the dump in a trailing newline. However, if you look further down below, you'll see that I check that the cursor is at the start of the line/row, just to be sure, and otherwise I'm printing another newline.

}

wchar_t buffer[32 * 1024];
DWORD read = 0;

Expand All @@ -1821,9 +1853,17 @@ namespace winrt::Microsoft::Terminal::Control::implementation
}
}

// This pushes the restored contents up into the scrollback.
const auto lock = _terminal->LockForWriting();
_terminal->Write(L"\x1b[2J");
{
const auto lock = _terminal->LockForWriting();

// Normally the cursor should already be at the start of the line, but let's be absolutely sure it is.
if (_terminal->GetCursorPosition().x != 0)
{
_terminal->Write(L"\r\n");
}

_terminal->Write(message);
}
}

void ControlCore::_rendererWarning(const HRESULT hr, wil::zwstring_view parameter)
Expand Down
4 changes: 4 additions & 0 deletions src/cascadia/TerminalControl/Resources/en-US/Resources.resw
View file
Open in desktop
Expand Up @@ -296,4 +296,8 @@ Please either install the missing font or choose another one.</value>
<value>Select output</value>
<comment>The tooltip for a button for selecting all of a command's output</comment>
</data>
<data name="SessionRestoreMessage" xml:space="preserve">
<value>Restored</value>
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to put the args in here too? I'm not sure if there's any locales where the translation would require re-ordering those bits

<comment>"Restored" as in "This content was restored"</comment>
</data>
</root>
47 changes: 43 additions & 4 deletions src/cascadia/TerminalControl/TermControl.cpp
View file
Open in desktop
Expand Up @@ -1181,11 +1181,12 @@ namespace winrt::Microsoft::Terminal::Control::implementation

if (!_restorePath.empty())
{
winrt::get_self<ControlCore>(_core)->RestoreFromPath(_restorePath.c_str());
_restorePath = {};
_restoreInBackground();
}
else
{
_core.Connection().Start();
}

_core.Connection().Start();
}
else
{
Expand Down Expand Up @@ -1272,6 +1273,44 @@ namespace winrt::Microsoft::Terminal::Control::implementation
return true;
}

winrt::fire_and_forget TermControl::_restoreInBackground()
{
const auto path = std::exchange(_restorePath, {});
const auto weakSelf = get_weak();
winrt::apartment_context uiThread;

try
{
co_await winrt::resume_background();

const auto self = weakSelf.get();
if (!self)
{
co_return;
}

winrt::get_self<ControlCore>(_core)->RestoreFromPath(path.c_str());
}
CATCH_LOG();

try
{
co_await uiThread;

const auto self = weakSelf.get();
if (!self)
{
co_return;
}

if (const auto connection = _core.Connection())
{
connection.Start();
Copy link
Member

Choose a reason for hiding this comment

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

1% worried about starting the connection on the BG thread instead of the window thread, but I don't actually see anything in ConptyConnection::Start that looks like it needs the main thread.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm also quite worried about this but looking at how it writes _hOutputThread and so on, I think I'll add some code to switch back to the main thread. While that adds to the latency, I think it's worth it.

}
}
CATCH_LOG();
}

void TermControl::_CharacterHandler(const winrt::Windows::Foundation::IInspectable& /*sender*/,
const Input::CharacterReceivedRoutedEventArgs& e)
{
Expand Down
1 change: 1 addition & 0 deletions src/cascadia/TerminalControl/TermControl.h
View file
Open in desktop
Expand Up @@ -319,6 +319,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
Reattach
};
bool _InitializeTerminal(const InitializeReason reason);
winrt::fire_and_forget _restoreInBackground();
void _SetFontSize(int fontSize);
void _TappedHandler(const Windows::Foundation::IInspectable& sender, const Windows::UI::Xaml::Input::TappedRoutedEventArgs& e);
void _KeyDownHandler(const Windows::Foundation::IInspectable& sender, const Windows::UI::Xaml::Input::KeyRoutedEventArgs& e);
Expand Down