search

pesos / grofer

A system and resource monitoring tool written in Golang!
Apache License 2.0
352 stars 52 forks source link

Allow sending various signals to processes #112

Closed anihm136 closed 3 years ago

anihm136 commented 3 years ago

Description

This PR aims to integrate sending of various signals to processes from the grofer proc subcommand. Currently, the command only allows killing of a process. The scope of this PR include -

Fixes #79, #80

Type of change

Please delete options that are not relevant.

Checklist:

anihm136 commented 3 years ago

On second thoughts, I should have probably made this a draft PR. Just to clarify, this is still WIP and not ready for review yet. Will update when I'm done

anihm136 commented 3 years ago

Do we currently have a way to display a panel/window on one side of the main panel? I'm thinking of a UI similar to htop for this. We could always go for a new overlay window (like the help window), but I feel it would be slightly obstructive to block the process list when the user selects a signal to send. @MadhavJivrajani @Gituser143 let me know your thoughts on this

Gituser143 commented 3 years ago

We currently have one being used for errors, in src/display/misc. It can be extended to take input. As for the obstruction, we can just render it according to relative terminal dimensions. @anihm136 do you think that could work?

anihm136 commented 3 years ago

Yep, that seems like what I was looking for!

anihm136 commented 3 years ago

Okay @Gituser143 I think I did not understand your point about relative terminal dimensions correctly. It appears that the error page is also rendered fullscreen, like the help window. What I'm thinking of is something like this - image Is this possible with the currently available stuff?

Gituser143 commented 3 years ago

@anihm136 Yes. So what you can do is, each drawable widget has a setRect() method which can be used to set relative terminal dimensions. For example in your case, you could do:

// Get terminal dimensions 
w, h := ui.TerminalDimensions()

// Set process grid coordinates
page.Grid.SetRect(2*w/3, 0, w, h)

// Set signal table coordinates
page.SigTable.SetRect(0, 0, w/3, h)

EDIT: You could take a look at the updateUI() function in src/display/general/overallGraphs.go for a reference, the case when numcores is not more than 8 should show you how it's being done.

EDIT 2: The values to setRect() are x1, y1, x2, y2. I'm not fully sure of the coordinate system. You can find that in the docs of termUI I guess.

anihm136 commented 3 years ago

Got it, thanks!

anihm136 commented 3 years ago

Okay, the basic signalList is now working. You can open a list of signals, navigate through it and select a signal to send to the process. Some more details are in the commit messages, and I'll start a discussion to decide on a few other aspects of behavior and keybindings

Gituser143 commented 3 years ago

@anihm136 Instead of inheriting a List widget for the signal list, I feel using the custom table (/src/utils/table.go) would be a better choice. This is because it would fit well with the already existing process table and for the reason that it resizes better than a list (If we were to implement more columns in the table, say with the integer codes, list would get messy). What do you think?

MadhavJivrajani commented 3 years ago

@anihm136 checking in, how's it going?

anihm136 commented 3 years ago

Sorry, been a little busy for the last few days. Should be able to make some progress today

anihm136 commented 3 years ago

@Gituser143 I'll look into using tables instead of a list today

anihm136 commented 3 years ago

Okay, we now have a table to display the signals (which actually looks a lot nicer and fits the rest of the UI well). I've also added numeric navigation, so you can type in the numeric ID of a signal in the table (as displayed beside it) to select the signal. I've also taken the liberty of adding a couple of utility functions that I needed for this, as I felt it would be useful elsewhere

anihm136 commented 3 years ago

I'm going to remove WIP now, because all functionality is ready. There are some potential improvements that we can discuss, and I will work on adding docs to this. The code is ready for review

Gituser143 commented 3 years ago

@anihm136 @MadhavJivrajani do you think we should add sorting functionality to the signal table? Considering it already has the option to be navigated purely with the the numbers, I'm not entirely sure. What do y'all think?

anihm136 commented 3 years ago

@MadhavJivrajani yes it does make sense to reword the help message. I'll do it as part of documentation update

MadhavJivrajani commented 3 years ago

@anihm136 @MadhavJivrajani do you think we should add sorting functionality to the signal table? Considering it already has the option to be navigated purely with the the numbers, I'm not entirely sure. What do y'all think?

Not sure how functional it would be considering it can be navigated with numbers

anihm136 commented 3 years ago

@Gituser143 I don't think sorting makes sense in this context, because generally you aren't looking for signals in any particular order. Two things that would help are -

  1. Indexing by signal number. The challenge with this is that different platforms have slightly different numbering for each signal, so I wanted to consider that before implementing
  2. Searching. I think this would be a great feature in general, and here in particular it would be helpful because you know either the name or the number of the signal. Between these two you would be able to quickly get to the signal you want
Gituser143 commented 3 years ago

Isn't it already sorted on signal integers though? We don't have to explicitly do it right?

anihm136 commented 3 years ago

Isn't it already sorted on signal integers though? We don't have to explicitly do it right?

They aren't sorted on signal integers now. Indexing is arbitrary (based on order of definition in the syscall package)

Samyak2 commented 3 years ago

I noticed another issue that can be reproduced with the following steps:

  1. Run grofer proc
  2. Run a different process (for example, cat) in another terminal such that it is the last process in the list
  3. Kill it with KK
  4. Notice no process is currently highlighted
  5. Press K again to open the signal list
  6. Try to navigate the signal list with numbers or jk, the navigation happens in the process list instead. <Escape> also does not close the signal list. The signal list is stuck there.

I'm not sure what's causing this yet.

anihm136 commented 3 years ago

@Samyak2 I am able to reproduce the second issue. Two things to note -

  1. The disappearing highlight seems to be a more general issue. When the last process in the list is highlighted and it dies (for any reason), no process is highlighted. Using the nav keys brings back the highlight to a process in the table
  2. After step 5, navigating the process list and pressing K on a selected process behaves as expected (does not open a new menu, navigation shifts to the existing menu and selecting a signal sends it to the process and closes the menu)

I think the first issue should be worked on as a separate issue. Meanwhile, I can fix it temporarily by making sure a process is highlighted before opening the menu

anihm136 commented 3 years ago

Can you verify if the issue is solved now?

Samyak2 commented 3 years ago

Yes, the issues are resolved now :tada:

Opening a separate issue for last process highlight sounds good. I think I first noticed it in #54 where it gave an out of range panic. Would you be able to open the issue for this?

anihm136 commented 3 years ago

Will do

anihm136 commented 3 years ago

Okay I think I'm done. Can I have another review to make sure everything's in order?

Samyak2 commented 3 years ago

@Gituser143 could you review the PR again? (or @MadhavJivrajani)

MadhavJivrajani commented 3 years ago

WOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOT