-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Fix settings popup process alive #39790
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
base: main
Are you sure you want to change the base?
Conversation
…nto fix/settings-window-close
…nto fix/settings-window-close
Sorry, there is a little bit of a mess in commits here. I haven't used Windows in years. |
Thanks for the PR @ddoemonn - we'll check it out. |
Right now your PR closes the settings window when the workspace that originally opened it is closed. This doesn't cover the edge case where a user has multiple open workspaces. I think it would be better if the subscription you created did something like this let Some(app_state) = workspace::AppState::global(cx).upgrade() else {
return;
};
if app_state.workspace_store.read(cx).workspaces().is_empty() {
// Close settings ui window
} I'm happy to make the changes if you don't have the time too! Also, this way you won't need to add the subscription as a field on SettingsWindow, you could just detach it. |
Actually @Anthony-Eid if that works for you I wanna make the changes if you can give me little bit time. But of course I do not know it is urgent or not. |
@ddoemonn No worries! It's semi urgent because I want this to be apart of the stable settings ui release, so If you can make the changes by Monday that would be great. Otherwise, I can do it |
@Anthony-Eid Monday works for me. Thanks! I will try to be quick. |
Closes #39786
Release Notes: