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

Fine grained kill signals on unix #263

Merged
merged 17 commits into from Dec 16, 2020

Conversation

LlinksRechts
Copy link
Contributor

Description

This PR adds a menu for selecting the signal to send to the selected process when pressing dd in unix.

image

The signals are aligned in a 8×8 grid, which can be navigated with either the arrow keys, hjkl, or the mouse (if enabled). In addition, number keys can be pressed to quickly jump to the respective signal. On enter, the selected signal is sent to the process and the popup is closed (or no signal is sent if 0: Cancel is selected).

The signal selected by default is 15 (SIGTERM), which is the default signal for killing processes on unix systems.

Issue

Closes: #251

Type of change

  • New feature (non-breaking change which adds functionality)

Test methodology

Tested on:

  • Windows
  • macOS
  • Linux

The feature was tested on Linux. In theory, the original functionality should still be in place for windows builds; however, I was not able to verify this as I could not get the app to run properly on windows in any version (including current master, release branch). I can verify that the build succeeds and the app starts on windows, but the output I get in the terminal (cmd, powershell, git bash) of my (virtual) machine (win7) is garbled, and I sadly don't have any other versions of windows to test with (according to the documentation of crossterm, it should work on win7...).

Checklist

  • Change has been tested to work, and does not cause new breakage unless intended
  • Code has been self-reviewed
  • Passes Travis tests (clippy check and cargo test check)
  • Areas your change affects have been linted using rustfmt (cargo fmt)
  • No merge conflicts arise from the change

@ClementTsang
Copy link
Owner

Firstly, thanks for the PR - I appreciate it a lot, especially since I'm kinda swamped with working on a lot of other stuff rn.

I'll take a closer look at the implementation parts and leave any suggestions if necessary in a bit, but one thing I may want to change is how the values are laid out. Since the strings themselves are quite long, it kinda looks a bit weird to me at least when I use it in a vertical terminal (which I normally use):
image

The nature of the layout also doesn't really handle window size changes too well either
image

Not sure how to exactly change it, but maybe going for a scrollable list would work better? I'm open to any ideas.

@ClementTsang
Copy link
Owner

Oh, right, and I'll test it on other platforms as well (macOS and Windows) later.

@LlinksRechts
Copy link
Contributor Author

the thing with the layout is a good point; i have to admit that i havent tried resizing...
i'll see if i can find a better solution, and of course i'm open to suggestions (since I usually have my terminal open on my 4k monitor, i probably have another perspective on the problem...)

@LlinksRechts
Copy link
Contributor Author

LlinksRechts commented Oct 3, 2020

I have replaced the grid with a scrollable list; what do you think? Imo it looks pretty usable... (and also scales better)

image

I'm not completely satisfied with the code yet, so I havent added it to the PR, but you can see it on the kill-list-scrollable in my fork

Also, I havent tried building on windows yet, so I might have broken smth

EDIT: it builds on windows, and I think it works (?) what I can tell from the broken layout...

src/app.rs Show resolved Hide resolved
@ClementTsang
Copy link
Owner

Also yeah feel free to take your time on this.

@LlinksRechts
Copy link
Contributor Author

fixed the number input thing, and cleaned up my scroll list and merged it into this branch

@ClementTsang
Copy link
Owner

ClementTsang commented Oct 13, 2020

Sorry, been busy for the past few days. Let me know whenever you're ready for me to take another look and I'll check it + test it (just not sure about the status of this PR and whether you're happy with it). If it's good I'll merge.

@LlinksRechts
Copy link
Contributor Author

No problem! I've fixed the things you suggested and just rebased to master, so imo the PR should be good to go whenever you're happy with it also :)

Copy link
Owner

@ClementTsang ClementTsang left a comment

Choose a reason for hiding this comment

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

Sorry for taking so long to do this. Work ramping up and a bad sleep schedule haven't been kind to me.


I prefer the list approach a lot more, and it generally looks better and feels more responsive to resizes, so that's good. However, a list approach brings in a lot of gotchas, mainly with how to achieve the list scrolling behaviour currently used in the application for other widgets. I've left some tips on how to fix it, but I know it's currently a bit of a confusing mess so let me know if you need some more help on it.

I tested running it on macOS, it looks fine, though you should double check which signals are valid on macOS, because some signals are not accepted that do work on Linux. Check htop and/or documentation for which signals are valid on macOS and Linux.

It looks fine on Windows as well other than the minor reversed movement bug, and you should make sure there aren't any warnings on compilation if you're using cfg to gate some of the imports:

image

I would also appreciate adding support for G and gg keybinds to jump to the end and start of the list respectively, but that's pretty easy for me to add later so no worries if it's not done in this PR.

src/canvas/dialogs/dd_dialog.rs Outdated Show resolved Hide resolved
src/app.rs Outdated Show resolved Hide resolved
src/canvas/dialogs/dd_dialog.rs Outdated Show resolved Hide resolved
src/canvas/dialogs/dd_dialog.rs Outdated Show resolved Hide resolved
src/app.rs Outdated Show resolved Hide resolved
@LlinksRechts
Copy link
Contributor Author

since I haven't answered in a while: I haven't forgotten about this, and will look into the things you noted, but have a lot of other stuff currently, so I'll see when I have the time :)

@ClementTsang
Copy link
Owner

No worries, I've also been a bit busy with IRL stuff too so I'm in no rush.

@LlinksRechts
Copy link
Contributor Author

Finally got around to looking into the things you commented; got some very good points there, thanks for the feedback!
I changed the things you suggested; youre right, the list flows much more naturally now, and I really didn't think about different signals on MacOS :p Also, I added support for gg/G.
If you could find some spare time to test this on windows once again that'd be great!

@ClementTsang
Copy link
Owner

Took a look on macOS and Windows, looks good there.

I'll take a look at the code and if it looks good I'll merge.

@ClementTsang
Copy link
Owner

@LlinksRechts LGTM. I'll merge this - thanks for the work you've done!

@all-contributors add @LlinksRechts for code

@allcontributors
Copy link
Contributor

@ClementTsang

I've put up a pull request to add @LlinksRechts! 🎉

@ClementTsang ClementTsang merged commit 120da2c into ClementTsang:master Dec 16, 2020
ClementTsang added a commit that referenced this pull request Dec 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: Sending arbitrary signals to processes
2 participants