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 a system message to session restore #17113

Merged
merged 3 commits into from Apr 24, 2024
Merged

Conversation

lhecker
Copy link
Member

@lhecker lhecker commented Apr 23, 2024

This adds a system message which displays the time at which the
buffer snapshot was written to disk.

Additionally, this PR moves the snapshot loading into a background
thread, so that the UI thread is unblocked and that multiple
tabs/panes can load simultaneously.

Closes #17031
Closes #17074

Validation Steps Performed

Repeatedly closing and opening WT adds more and more messages.
Currently, the messages get somewhat corrupted due to a bug
in our line-wrap handling, or some similar part.

co_return;
}

const auto core = winrt::get_self<ControlCore>(_core);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const auto core = winrt::get_self<ControlCore>(_core);
const auto core = winrt::get_self<ControlCore>(self->_core);

Shouldn't we use self here?

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't need to. WinRT types like this are immovable and so the this pointer will refer to the same object after the suspension as it did before. self is just another safe/managed pointer.

{
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.

// 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.

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.

@@ -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

winrt::fire_and_forget TermControl::_restoreInBackground()
{
const auto path = std::exchange(_restorePath, {});
const auto dispatcher = Dispatcher();
Copy link
Member

Choose a reason for hiding this comment

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

theoretically you could do a https://learn.microsoft.com/en-us/windows/uwp/cpp-and-winrt-apis/concurrency-2#programming-with-thread-affinity-in-mind:

{
    winrt::apartment_context ui_thread; // Capture calling context.

    co_await winrt::resume_background();
    
    // Do compute-bound work here.

    co_await ui_thread; // Switch back to calling context.

    textblock.Text(L"Done!"); // Ok if we really were called from the UI thread.
}

but meh

Copy link
Member Author

Choose a reason for hiding this comment

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

And we won't have to replace that with wil::apartment_context for some weird reason soon right?
...
...
...right?

I'll change it, because I get the feeling this is how it's supposed to be done.

Copy link
Member

Choose a reason for hiding this comment

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

Leonard, can we file a followup task to replace all instances of "save the dispatcher and return to it" with this more correct thing?

Copy link
Member Author

@lhecker lhecker Apr 24, 2024

Choose a reason for hiding this comment

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

There's actually only 2 places which fit this pattern:

  • AppHost::_RenameWindowRequested
    This one should probably not yield to a background thread at all. It would probably get fixed if/when/ever we clean up the peasant/monarch code.
  • TerminalPage::_PasteFromClipboardHandler
    Here, I'm not entirely sure whether PasteFromClipboard is always raised from the foreground thread. It's probably not, so I think it can't use winrt::apartment_context.

@lhecker lhecker added this pull request to the merge queue Apr 24, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 24, 2024
@DHowett DHowett added this pull request to the merge queue Apr 24, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Apr 24, 2024
@DHowett DHowett merged commit 0c3c747 into main Apr 24, 2024
18 of 20 checks passed
@DHowett DHowett deleted the dev/lhecker/17074-restore-message branch April 24, 2024 21:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants