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

windows destruction dispatchers #8962

Merged
merged 5 commits into from
Jan 8, 2025
Merged

Conversation

littleblack111
Copy link
Contributor

@littleblack111 littleblack111 commented Jan 5, 2025

Describe your PR, what does it fix/add?

atm kill(active,window) don't actually kill it but close it instead, this fixes that
also adds real kill(active,window) which actually kills it

Is there anything you want to mention? (unchecked code, possible bugs, found problems, breaking compatibility, etc.)

should probably be in breaking changes

Is it ready for merging, or does it need work?

ready

@izmyname
Copy link
Contributor

izmyname commented Jan 5, 2025

Isn't the whole point of killactive is to gracefully close the client, akin to 'x' button on stacking compositors?

Also, what about daemon processes, like easyeffects? For example, won't 'killing' easyeffects client window also kill ---gapplication-service process?

@littleblack111
Copy link
Contributor Author

Isn't the whole point of killactive is to gracefully close the client, akin to 'x' button on stacking compositors?

Also, what about daemon processes, like easyeffects? For example, won't 'killing' easyeffects client window also kill ---gapplication-service process?

imo kill doesn't sound that graceful, but close is. you can still use the killactive the same, just now renamed to closeactive

Copy link
Member

@vaxerski vaxerski left a comment

Choose a reason for hiding this comment

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

silent config break, I don't like those.

Is this really needed?

@izmyname
Copy link
Contributor

izmyname commented Jan 5, 2025

It's not about how it sounds, but what it does.

you can still use the killactive the same, just now renamed to closeactive

Honestly, I don't see the point. Besides, this is a breaking change and I can predict many 'help when I close the window - my daemon processes die too' issues.

UPD: also, what is the point in killing a client, instead of just closing? They don't get unresponsive that often - and, if they do - there's htop/btop/whatever task manager to get rid of them

@littleblack111
Copy link
Contributor Author

littleblack111 commented Jan 5, 2025

Just an feature I used to have but now have to replace via scripting, and I do use it pretty often(especially to applications that are poorly coded and mainly focus on windows just hang for 1-2min after giving killactive). I guess the naming of “kill”active didn’t rly make sense in the first place if it’s not really killing it but closing the window gracefully. Also, since we have that dialog that shows after every update, should be fine right

edit: if you have any better ways of doing this without a silent config break. I’d be happy to do

help when I close the window - my daemon processes die too' issues.

don’t see why would it kill the daemon? It sends a kill to the pid, not a killall to the binary(name)

Edit2: I think to avoid config breaks, not only silent ones, but config break in general, we shall perhaps add a post install script that automatically settles those stuff for us? Since I've seen quite a lot of Reddit post regarding the shadow configuration change when it was made.

@izmyname
Copy link
Contributor

izmyname commented Jan 5, 2025

I guess the naming of “kill”active didn’t rly make sense in the first place

It's a common name for this function - you can find this in sway, i3 and other compositors/xorg wms. Keeping things standardized is a good thing.

Also, since we have that dialog that shows after every update, should be fine right

That dialog pops up after a version bump - not git.

edit: if you have any better ways of doing this without a silent config break. I’d be happy to do

Not changing kill, killactive behavior.

don’t see why would it kill the daemon? It sends a kill to the pid

Okay, but what about the apps that aren't meant to be killed upon closing a window? The ones with a systray, for example?

Overall, I still don't see the point. To me - it looks like you want to break the most common must-have dispatcher, everyone uses, just because you, personally, prefer killing clients, instead of closing them.

@izmyname
Copy link
Contributor

izmyname commented Jan 5, 2025

don’t see why would it kill the daemon? It sends a kill to the pid, not a killall to the binary(name)

Also, I've just checked: easyeffects flatpak (and non-flatpak, I guess) has just one pid for both gapplication-service and an opened client window. I assume, the same is true for all GTK apps with daemon functionality. So, my point still stands.

@TAforever
Copy link

TAforever commented Jan 5, 2025

Using a real kill by default instead of close is a bad idea, but having a separate dispatcher for kill would be nice. bspwm has this option

bspc node -c #close
bspc node -k #kill

Currently in hyprland I use
bind = $mainMod SHIFT, Q, exec, kill -9 $(hyprctl activewindow | grep pid | tail -1 | awk '{print $2}')

Also it would close this issue #8212

@izmyname
Copy link
Contributor

izmyname commented Jan 5, 2025

You could leave kill(active), as it already is, and name the new dispatcher something like pid_kill(active) or kill(active)_proc. Or pkill(active).

UPD: hard_kill, destroy, terminate. I, personally, like the latter as it is both self-explanatory and different from already existing kill.

@leiserfg
Copy link
Contributor

leiserfg commented Jan 5, 2025

I agree with @izmyname, repurposing kill will break old configs (even when IMHO the name was confusing from the beginning).
Maybe an option to consider could be signal allowing to pass https://www.gnu.org/software/libc/manual/html_node/Termination-Signals.html

So @TAforever 's script :
bind = $mainMod SHIFT, Q, exec, kill -9 $(hyprctl activewindow | grep pid | tail -1 | awk '{print $2}')

could be replaced by:
bind = $mainMod SHIFT, Q, signal, 9
or:
bind = $mainMod SHIFT, Q, signal, 15

depending on how the user wants to deal with the processes

@littleblack111
Copy link
Contributor Author

littleblack111 commented Jan 5, 2025

Alright, will rename to forcekill(active) then?

And sure, I'll add a signal dispatcher too

don’t see why would it kill the daemon? It sends a kill to the pid, not a killall to the binary(name)

Also, I've just checked: easyeffects flatpak (and non-flatpak, I guess) has just one pid for both gapplication-service and an opened client window. I assume, the same is true for all GTK apps with daemon functionality. So, my point still stands.

I see, didn't realize that, thanks. Shouldn't be a issue anyway if you don't use the new dispatcher(since we r gonna keep the old dispatchers name)

@littleblack111 littleblack111 changed the title kill(active,window) fixes windows destruction dispatchers Jan 5, 2025
@littleblack111
Copy link
Contributor Author

I agree with @izmyname, repurposing kill will break old configs (even when IMHO the name was confusing from the beginning). Maybe an option to consider could be signal allowing to pass https://www.gnu.org/software/libc/manual/html_node/Termination-Signals.html

So @TAforever 's script : bind = $mainMod SHIFT, Q, exec, kill -9 $(hyprctl activewindow | grep pid | tail -1 | awk '{print $2}')

could be replaced by: bind = $mainMod SHIFT, Q, signal, 9 or: bind = $mainMod SHIFT, Q, signal, 15

depending on how the user wants to deal with the processes

alright, here you go.

you can do forcekillactive
or signal 9

signalwindow is also added

Copy link
Member

@vaxerski vaxerski left a comment

Choose a reason for hiding this comment

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

wiki mr pls

src/managers/KeybindManager.cpp Show resolved Hide resolved
src/managers/KeybindManager.cpp Outdated Show resolved Hide resolved
Copy link
Member

@vaxerski vaxerski left a comment

Choose a reason for hiding this comment

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

thanks!

@vaxerski vaxerski merged commit c9822b0 into hyprwm:main Jan 8, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants