Separate pruning of elevated/unelevated session buffers #19546
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
carlos-zamora
left a comment
carlos-zamora
left a comment
There was a problem hiding this comment.
There's a few changes I'm confused about:
- Package-Dev.appxmanifest
- common.build.pre.props
I also think we want to keep the distinction between PersistLayout and PersistAll for #19341. May be worth testing persistence with both values, to be safe.
| sessionIds.emplace(terminalArgs.SessionId()); | ||
| control.PersistTo(reinterpret_cast<int64_t>(file.get())); | ||
| bufferFilenames.emplace(std::move(filename)); |
There was a problem hiding this comment.
Shouldn't we only do this if _app.Logic().Settings().GlobalSettings().FirstWindowPreference() == FirstWindowPreference::PersistedLayoutAndContent?
There was a problem hiding this comment.
Above, I initialize the persistBuffers variable with exactly that. This entire block of code is skipped if it's false.
| MovePane, | ||
| PersistLayout, | ||
| PersistAll | ||
| Persist, |
There was a problem hiding this comment.
Don't we want to keep the distinction between PersistLayout and PersistAll? See #19341
There was a problem hiding this comment.
Not anymore. Now, this code doesn't persist the content anymore, so any Persist will be a PersistLayout.
| } | ||
| break; | ||
| } | ||
| case BuildStartupKind::PersistLayout: |
There was a problem hiding this comment.
Seems like we'd want to keep the branches separate
There was a problem hiding this comment.
Hmm you're right that what used to be PersistLayout, now also gets assigned a SessionId. I wonder if that's a problem... It does cause our app to try and load the corresponding buffer_{guid}.txt file, which doesn't exist (because contents didn't get persisted), but that will just silently fail. I'm not 100% sure how big of a deal this is. It's nice that we have 1 enum less IMO.
microsoft-github-policy-service
bot
added
Needs-Author-Feedback
Previously, launching an unelevated session after an elevated one would delete the latter's persisted buffers, and vice versa of course. Also, elevated buffers didn't have an ACL forbidding access to unelevated users. That's also fixed now.
Closes #19526
Validation Steps Performed
buffer_are renamed toelevated_if needed ✅