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

Fix session being persisted even when disabled #17211

Merged
merged 1 commit into from
May 8, 2024

Conversation

lhecker
Copy link
Member

@lhecker lhecker commented May 8, 2024

This fixes 2 bugs:

  • PersistState being called when the window is closed
    (as opposed to closing the tab). The settings check was missing.
  • Session cleanup running depending on whether the feature is
    currently enabled as opposed to whether it was enabled on launch.

Closes #17206
Closes #17207

Validation Steps Performed

  • Create a bunch of leftover buffer_*.txt files by running
    the current Dev version off of main
  • Build this branch, then open and close a window
  • All buffer_*.txt are gone and state.json is cleaned up ✅

@DHowett
Copy link
Member

DHowett commented May 8, 2024

Can you make sure this works properly in the multi-window case?

2 windows 2 tabs cases:

  • Close the second window with the X
  • Close the second window by closing each tab individually
  • Quit the application with all 4 tabs open by using command palette Quit

@carlos-zamora
Copy link
Member

Closes #17211

Closes this PR? haha

@lhecker
Copy link
Member Author

lhecker commented May 8, 2024

@DHowett When you say "this works" what do you mean: That it persists or that it doesn't persist?

If it's the latter, then yes this will work, because PersistState() is only ever called if ShouldUsePersistedLayout() is true. There's no alternate way to get into the PersistState() call that gets past this condition. Of course I can test this later, but this PR is IMO good to go.

@DHowett
Copy link
Member

DHowett commented May 8, 2024

If it's the latter, then yes this will work, because PersistState() is only ever called if ShouldUsePersistedLayout() is true.

ShouldUsePersistedLayout only reflects the user's preferences, it does not make any decisions based on how many windows currently exist.

I expect the top two cases to NOT persist, and the bottom case to persist two windows.

@lhecker
Copy link
Member Author

lhecker commented May 8, 2024

Oh I get what you mean now. So, the difference is AppHost::_CloseRequested() vs. AppHost::_quit() and its callback AppHost::_QuitRequested.

_CloseRequested() is called when the last tab of a window is closed or you close a window and there's more than one left. _quit() is called when using "Quit all". It's also called when there's only 1 window is left (see _CloseRequested() and its GetNumberOfPeasants()).

I'll test it but I think this should be helpful for understanding how it works. 😅

@lhecker
Copy link
Member Author

lhecker commented May 8, 2024

Alright, I hope I tested it properly, but it seemed to work correctly.

@DHowett DHowett added this pull request to the merge queue May 8, 2024
Merged via the queue into main with commit dbac3a1 May 8, 2024
20 checks passed
@DHowett DHowett deleted the dev/lhecker/17206-persistence-fixup branch May 8, 2024 21:31
DHowett pushed a commit that referenced this pull request May 13, 2024
This fixes 2 bugs:
* `PersistState` being called when the window is closed
  (as opposed to closing the tab). The settings check was missing.
* Session cleanup running depending on whether the feature is
  currently enabled as opposed to whether it was enabled on launch.

Closes #17206
Closes #17207

## Validation Steps Performed
* Create a bunch of leftover buffer_*.txt files by running
  the current Dev version off of main
* Build this branch, then open and close a window
* All buffer_*.txt are gone and state.json is cleaned up ✅

(cherry picked from commit dbac3a1)
Service-Card-Id: 92515454
Service-Version: 1.21
lhecker added a commit that referenced this pull request May 14, 2024
lhecker added a commit that referenced this pull request May 16, 2024
As the title says, this backports the changes in #17211 and #17268:
* `PersistState` being called when the window is closed
  (as opposed to closing the tab). The settings check was missing.
* Avoid persisting windows with 0 tabs (= last tab gets closed).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Rejected
4 participants