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

kill subscriber process when desktop is removed #60

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

amtoine
Copy link
Collaborator

@amtoine amtoine commented Jul 9, 2022

This is a duplicate of the stale #36 to apply the requested changed and merge the branch to master.

@amtoine amtoine added duplicate This issue or pull request already exists feature New feature or request labels Jul 9, 2022
@amtoine amtoine requested a review from phenax July 9, 2022 08:51
@amtoine amtoine self-assigned this Jul 9, 2022
@amtoine
Copy link
Collaborator Author

amtoine commented Jul 9, 2022

TODO:

@amtoine
Copy link
Collaborator Author

amtoine commented Aug 9, 2022

@phenax
would you like me to apply the changes above from the original PR, without a review?
or wrap them inside a review first?

no pressure if you do not have the time, that's just a friendly ping 😌

@phenax
Copy link
Owner

phenax commented Aug 9, 2022

@amtoine, I think it's better if this pr contains those changes before the review if that's what you mean

@amtoine
Copy link
Collaborator Author

amtoine commented Aug 9, 2022

@amtoine, I think it's better if this pr contains those changes before the review if that's what you mean

it was my question 😋

so i'll add the changes and ping you when i'm done 😉

Addresses the original review requested changes from phenax#36
Addresses requested changes from the original stale phenax#36
@amtoine
Copy link
Collaborator Author

amtoine commented Aug 12, 2022

@phenax
the first point should be addressed in b38e145c and the second in 3186fc79 😋
EDIT: and a bad typo below 👀

review can begin 💪

remove_listener() {
desktop=$1;
[[ -z "$desktop" ]] && desktop=$(get_focused_desktop);

kill_layout "$desktop";
local old_pid="$(get_desktop_options "$desktop" | valueof pid)";
Copy link
Owner

Choose a reason for hiding this comment

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

I don't get the point of this change. It was killing the process before resetting the state, now it does that after resetting the state but what does that accomplish?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

that is a change from the original PR, not sure what the goal was here to be honest 🤔

and, with the changes you originally requested in #36, kill_layout "$desktop" and

old_pid="$(get_desktop_options "$desktop" | valueof pid)"
kill_process "$old_pid"

should do exactly the same thing 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

change the way kill_layout gets called... desktop options in remove_listener have to be cleared before calling kill_layout,
otherwise if remove_listener is called by the subscriber process on desktop_remove event, it will kill itself before the options are cleared.

that is the original comment on the PR 👌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate This issue or pull request already exists feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants