Implement buffer restore by lhecker · Pull Request #16598 · microsoft/terminal

lhecker · 2024-01-24T16:58:54Z

This changeset allows Windows Terminal to dump its buffer contents as
UTF-16LE VT text onto disk and restore it later. This functionality is
enabled whenever persistedWindowLayout is being used.

Closes #961
Closes #16741

Validation Steps Performed

Open multiple windows with multiple tabs and restart the app
Everything's restored ✅
Reopen a tab with output from RenderingTests.exe
Everything's restored ✅
Closing tabs and windows with Ctrl+W deletes their buffer dumps ✅
Closing tabs doesn't create buffer dumps ✅

lhecker · 2024-01-24T17:03:42Z

src/cascadia/TerminalApp/AppLogic.cpp

    }

-    AppLogic::AppLogic() :
-        _reloadState{ std::chrono::milliseconds(100), []() { ApplicationState::SharedInstance().Reload(); } }


I believe not monitoring state.json for changes anymore is important as it prevents disturbing the session state while session persistence is still ongoing. That's because when ApplicationState flushes to disk, this FS monitor will be triggered and reload the ApplicationState again.

I figured it's not a problem anymore to do this since we're effectively a singleton application now.

This contains all the parts of #16598 that aren't specific to session restore, but are required for the code in #16598: * Adds new GUID<>String functions that remove the `{}` brackets. * Adds `SessionId` to the `ITerminalConnection` interface. * Flush the `ApplicationState` before we terminate the process. * Not monitoring `state.json` for changes is important as it prevents disturbing the session state while session persistence is ongoing. That's because when `ApplicationState` flushes to disk, the FS monitor will be triggered and reload the `ApplicationState` again.

zadjii-msft · 2024-03-04T17:36:18Z

notes:

In AppHost::_CloseRequested,
- if the last peasant is closed, we call AppHost::_quit then bail.
- Otherwise, we tell the monarch to remove us then PostQuitMessage
AppHost::_quit
- hop off to a BG thread, clear the PersistedWindowLayouts, then RequestQuitAll
  - RequestQuitAll handled in Monarch::_handleQuitAll, which will ONCE in a process:
    - call Quit on each peasant, which raises _QuitRequestedHandlers, which is handled in
      - AppHost::_QuitRequested, which
        
        calls down to TerminalWindow::Quit
        
        which calls down to _root->PersistState(), which saves that window.
        
        Then AppHost calls PostQuitMessage to exit it's hwnd
LastTabClosed was just replaced by CloseWindowRequested

zadjii-msft

Most of this I'm cool with, but I have a couple last questions. Honestly, not as scary as I expected

zadjii-msft · 2024-03-04T15:52:07Z

src/cascadia/TerminalControl/ControlCore.cpp


+    void ControlCore::PersistToPath(const wchar_t* path) const
+    {
+        const wil::unique_handle file{ CreateFileW(path, GENERIC_WRITE, FILE_SHARE_READ | FILE_SHARE_DELETE, nullptr, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, nullptr) };


kinda weird to me that the actual mechanics of reading/writing the file are down at the level of the control.

Benefit is that the file contents don't need to ever traverse a winrt boundary. And theoretically, anyone using a TermControl could then also get fairly trivial session restore from a file.

Downsides: we can't reuse our file loader (that Dustin's moving in #15329 so maybe this isn't important), and should other consumers get this? I suppose why not?

Okay I think I'm on board

src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp

src/cascadia/TerminalApp/Pane.cpp

src/cascadia/TerminalApp/AppCommandlineArgs.h

src/cascadia/TerminalControl/ControlCore.cpp

src/cascadia/WindowsTerminal/WindowEmperor.cpp

src/cascadia/WindowsTerminal/AppHost.cpp

src/cascadia/TerminalApp/TerminalPage.cpp

zadjii-msft · 2024-03-04T17:29:57Z

src/cascadia/TerminalApp/TerminalPage.cpp

+        {
+            const auto settingsDir = CascadiaSettings::SettingsDirectory();
+            const auto idStr = Utils::GuidToPlainString(id);
+            const auto path = fmt::format(FMT_COMPILE(L"{}\\buffer_{}.txt"), settingsDir, idStr);


some part of me wants to abstract out this fmt::format call to a common util lib, so that the app and control don't need to be updated in parallel if we change this, but also meh

Yeah, I also felt a bit disgusted about putting this here, especially when considering the counterpart in WindowEmperor. But I couldn't come up with a good way to put it anywhere else. I think the saving grace of this code is that it's at least encapsulated to just 3 "small" sections of TerminalPage and WindowEmperor.

funny enough, it was my plan to add support for automatic recordings which would use state/sessionIdGuid files!

src/cascadia/WindowsTerminal/AppHost.cpp

…r-restore

zadjii-msft

I'm commander Shepard, and this is my favorite release of the Terminal

DHowett

Honestly I am exceptionally cool with this. Everything makes sense, it simplifies the state persistence logic severely, and it gives us a generic TextBuffer -> VT function (!)

DHowett · 2024-03-28T20:13:23Z

src/buffer/out/TextColor.h

-        return _index;
-    }
-
+    BYTE GetIndex() const noexcept;


dumb question - does moving this out of the header harm the inlineability of it? does LTCG largely help with that?

inline isn't a hint for inlining anymore, so it shouldn't make any difference. LTCG makes no difference either I think, because if anything it's probably inlined during initial compilation already due to its small size.

Ah, I don't mean because of the inline keyword. Method bodies in header files can be inlined automatically, by the compiler, without the linker's help (LTCG). If you move the body into the cpp file, it can't be inlined during initial compilation because the compiler can only see the body from a single translation unit. It would require LTCG to come through and inline it at the call sites, now, because the other translation units couldn't see the body at compile time.

Ah good point. This would be a good reason to move it back to the header.

src/cascadia/TerminalApp/Resources/en-US/Resources.resw

DHowett · 2024-03-28T20:40:34Z

src/cascadia/TerminalApp/Resources/en-US/Resources.resw

    <value>Open with the given profile. Accepts either the name or GUID of a profile</value>
  </data>
+  <data name="CmdSessionIdArgDesc" xml:space="preserve">
+    <value>Sets the WT_SESSION variable</value>


Suggested change

<value>Sets the WT_SESSION variable</value>

<value>Sets the WT_SESSION variable; must be a GUID</value>

maybe? or, is there an opportunity for like, yeeting an environment variable injection into this arg?

DHowett · 2024-03-28T20:43:24Z

src/cascadia/TerminalApp/TerminalPage.cpp

+        {
+            const auto settingsDir = CascadiaSettings::SettingsDirectory();
+            const auto idStr = Utils::GuidToPlainString(id);
+            const auto path = fmt::format(FMT_COMPILE(L"{}\\buffer_{}.txt"), settingsDir, idStr);


funny enough, it was my plan to add support for automatic recordings which would use state/sessionIdGuid files!

src/cascadia/TerminalControl/ControlCore.cpp

DHowett · 2024-03-28T20:48:12Z

src/cascadia/TerminalControl/ControlCore.cpp

+            }
+        }
+
+        const auto lock = _terminal->LockForReading();


this is the read lock, should it be the write lock?

DHowett · 2024-03-28T20:49:29Z

src/cascadia/TerminalControl/IControlSettings.idl


-        Boolean EnableUnfocusedAcrylic;
+        Boolean EnableUnfocusedAcrylic { get; };
+        Guid SessionId { get; };


I didn't see this one get used; do we use it?

Yes, via the TerminalSettings object to get the sessionId into the connection.

DHowett · 2024-03-28T20:50:18Z

src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp


+// This returns something akin to %LOCALAPPDATA%\Packages\WindowsTerminalDev_8wekyb3d8bbwe\LocalState
+// just like SettingsPath(), but without the trailing \settings.json.
+winrt::hstring CascadiaSettings::SettingsDirectory()


Oh dang! @zadjii-msft you could have used this for WT_SETTINGS_DIR

THATS WHY I thought this method already existed!

DHowett · 2024-03-28T20:52:34Z

src/cascadia/WindowsTerminal/AppHost.cpp

+            si.cb = sizeof(si);
+            wil::unique_process_information pi;
+
+            LOG_IF_WIN32_BOOL_FALSE(CreateProcessW(nullptr,


holy sh--, multi-window restore starts WindowsTerminal.exe which then gets a handle to our Monarch and sends it a commandline.

What the heck.

We could just... use Monarch directly here. I know you didn't write this code but I think we missed it when we migrated to single-proc.

Yup. I felt like it would be too off-topic if I changed this part within the PR. But I agree that it's ripe for a refactoring.

…r-restore

lhecker · 2024-03-28T23:08:21Z

FYI the merge commit is non-trivial. If you do want to review my latest changes, you may have to review the merge as well.

j4james · 2024-03-29T01:21:27Z

src/buffer/out/textBuffer.cpp

+                    static constexpr Mapping mappings[] = {
+                        { CharacterAttributes::Intense, { 22, 1 } },
+                        { CharacterAttributes::Italics, { 23, 3 } },
+                        { CharacterAttributes::Blinking, { 25, 5 } },
+                        { CharacterAttributes::Invisible, { 28, 8 } },
+                        { CharacterAttributes::CrossedOut, { 29, 9 } },
+                        { CharacterAttributes::Faint, { 22, 2 } },
+                        { CharacterAttributes::TopGridline, { 55, 53 } },
+                        { CharacterAttributes::ReverseVideo, { 27, 7 } },
+                        { CharacterAttributes::BottomGridline, { 24, 4 } },
+                    };
+                    for (const auto& mapping : mappings)
+                    {
+                        if (WI_IsAnyFlagSet(attrDelta, mapping.attr))
+                        {
+                            const auto n = til::at(mapping.change, WI_IsAnyFlagSet(attr, mapping.attr));
+                            fmt::format_to(std::back_inserter(buffer), FMT_COMPILE(L"\x1b[{}m"), n);
+                        }
+                    }


I don't think this will work correctly when mixing intense and faint. For example, a sequence like \e[1mBOLD\e[2mBOTH\e[0;2mFAINT will assumedly serialize as \e[1mBOLD\e[2mBOTH\e[22mFAINT, which isn't correct (the FAINT text won't be faint).

It's also worth mentioning that BottomGridLine isn't the same as underline - we don't have a VT mapping for that. So AFAIK, it shouldn't ever show up in the Windows Terminal buffer.

Oh, I didn't even notice that I used 22 twice. Thanks for noticing this!
I've opened #16970 which should fix both issues.

zadjii-msft

I'm fine dealing with the intense/faint thing in post.

This got lost in #16598. `TerminalPage` needs to ask the window where the it actually is, so it can persist it. Closes #17010

This got lost in #16598. `TerminalPage` needs to ask the window where the it actually is, so it can persist it. More details in #16598 (comment) Closes #17010

This contains all the changes of 5dda507, c4c5206, 9f08ee7, and 5f10159 (#16598 and all related PRs), but without the buffer restore feature. The hope is that these changes fix some rarer issues we've been hearing about, where persistence doesn't work correctly. ## Validation Steps Performed This changeset was tested on Windows 11 with 2 windows and 4 tabs where 1 tab had 2 mixed split panes. All windows and tabs got restored properly. It didn't crash on Windows 10.

This got lost in #16598. `TerminalPage` needs to ask the window where the it actually is, so it can persist it. More details in #16598 (comment) Closes #17010 (cherry picked from commit 643f716) Service-Card-Id: 92360686 Service-Version: 1.20

microsoft-github-policy-service bot added Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Area-UserInterface Issues pertaining to the user interface of the Console or Terminal Product-Terminal The new Windows Terminal. labels Jan 24, 2024

lhecker commented Jan 24, 2024

View reviewed changes

lhecker added a commit that referenced this pull request Jan 25, 2024

General improvements in preparation for #16598

1b48358

lhecker mentioned this pull request Jan 25, 2024

General improvements in preparation for #16598 #16601

Merged

lhecker added a commit that referenced this pull request Jan 25, 2024

General improvements in preparation for #16598

5270766

Implement buffer restore

f9511d9

microsoft-github-policy-service bot added Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) labels Feb 21, 2024

lhecker force-pushed the dev/lhecker/961-buffer-restore branch from 5dbb09a to f9511d9 Compare February 21, 2024 15:23

lhecker marked this pull request as ready for review February 21, 2024 15:23

Fix AuditMode failures

eeb293a

zadjii-msft reviewed Mar 4, 2024

View reviewed changes

lhecker added 2 commits March 20, 2024 18:31

Address feedback

e4b7646

Merge remote-tracking branch 'origin/main' into dev/lhecker/961-buffe…

d3dbcf2

…r-restore

zadjii-msft approved these changes Mar 21, 2024

View reviewed changes

DHowett approved these changes Mar 28, 2024

View reviewed changes

lhecker mentioned this pull request Mar 28, 2024

Refactor Pane to be able to host non-terminal content #16170

Merged

1 task

lhecker added 2 commits March 28, 2024 23:58

Merge remote-tracking branch 'origin/main' into dev/lhecker/961-buffe…

f697f3a

…r-restore

Address feedback

a9ffc64

j4james reviewed Mar 29, 2024

View reviewed changes

zadjii-msft approved these changes Mar 29, 2024

View reviewed changes

zadjii-msft mentioned this pull request Mar 29, 2024

Problems with session startup and resumption #16487

Closed

zadjii-msft added this pull request to the merge queue Mar 29, 2024

Merged via the queue into main with commit c4c5206 Mar 29, 2024

zadjii-msft deleted the dev/lhecker/961-buffer-restore branch March 29, 2024 12:21

lhecker mentioned this pull request Apr 11, 2024

[1.20] Backport session persistence improvements #17049

Merged

zadjii-msft mentioned this pull request Apr 15, 2024

[1.21 canary] Session restore is restoring to the wrong screen coordinates and window size #17010

Closed

zadjii-msft added a commit that referenced this pull request Apr 16, 2024

Also remember to persist window positions

2ffe74d

This got lost in #16598. `TerminalPage` needs to ask the window where the it actually is, so it can persist it. Closes #17010

zadjii-msft mentioned this pull request Apr 16, 2024

Also remember to persist window positions #17066

Merged

mmseng mentioned this pull request May 16, 2024

Don't push the buffer contents into the scrollback on session restore #17274

Closed

ghost approved these changes Jun 2, 2024

View reviewed changes

	<value>Sets the WT_SESSION variable</value>
	<value>Sets the WT_SESSION variable; must be a GUID</value>

Conversation

lhecker commented Jan 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Validation Steps Performed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zadjii-msft commented Mar 4, 2024

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lhecker Mar 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

DHowett left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lhecker commented Mar 28, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

lhecker commented Jan 24, 2024 •

edited

Loading

lhecker Mar 20, 2024 •

edited

Loading