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

Feature, add PID column to processes table #165

Closed
wants to merge 15 commits into from

Conversation

olehs0
Copy link
Contributor

@olehs0 olehs0 commented Apr 12, 2020

Fixes #159.
Grab and show process identifier in a particular column to the processes table

@olehs0 olehs0 changed the title Feature/Add PID column to processes table Feature, add PID column to processes table Apr 12, 2020
@olehs0
Copy link
Contributor Author

olehs0 commented Apr 12, 2020

Hello @imsnif! I don't know how to resolve conflicts in this file src/display/ui_state.rs, but i can try to do it. It seems there is some additional code which conflicted with my changes.

@TheLostLambda
Copy link
Collaborator

This is likely due to the recent update to Bandwhich adding cumulative usage tracking. There were changes to the table.rs, ui.rs, ui_state.rs, tests, and perhaps a few others. Unfortunately, there is no super easy solution. I would try to pull the latest version of the master branch on your machine and try a git rebase.

Most of the conflicts should be pretty easy to resolve, but it's possible that you'll need to do some more manual intervention. Let me know if you have any questions!

@olehs0
Copy link
Contributor Author

olehs0 commented Apr 17, 2020

sorry, guys @TheLostLambda @imsnif , there is an issue with macos build and i cannot reproduce it on my local machine (arch linux). I tried rustup target add x86_64-apple-darwin and then build it via cargo build --target=x86_64-apple-darwin but getting errors(see a screenshot). so, could you please provide some suggestions/helps in this situation?
image
image

@TheLostLambda
Copy link
Collaborator

Hi @olehs0! I've done some cross-compilation in the past from Arch Linux an it's a little finicky. Have you installed apple-darwin-osxcross? That's needed to link binaries for Mac OS.

Let me know if installing that helps any!

@olehs0
Copy link
Contributor Author

olehs0 commented Apr 17, 2020

Hi @olehs0! I've done some cross-compilation in the past from Arch Linux an it's a little finicky. Have you installed apple-darwin-osxcross? That's needed to link binaries for Mac OS.

Let me know if installing that helps any!

@olehs0 olehs0 closed this Apr 17, 2020
@olehs0 olehs0 reopened this Apr 17, 2020
@olehs0
Copy link
Contributor Author

olehs0 commented Apr 17, 2020

Hi @olehs0! I've done some cross-compilation in the past from Arch Linux an it's a little finicky. Have you installed apple-darwin-osxcross? That's needed to link binaries for Mac OS.

Let me know if installing that helps any!

thank you! i cannot build apple-darwin-osxcross because i have missed libc++ dependencies, so i'm going to resolve it. i'll ask you if i have any doubt.

@imsnif
Copy link
Owner

imsnif commented Apr 26, 2020

Hey @olehs0 - just a note: I will admit I could also never get cross-compiling working for macos. :)

The good news is that the macos version works on linux (it gets its info from lsof instead of the /proc filesystem which does not exist on mac). When I want to test stuff on mac, I usually just switch the "cfgs" around. So that everything that was "linux" becomes "macos" and the other way around. Hope that helps!

@olehs0
Copy link
Contributor Author

olehs0 commented Apr 27, 2020

Hey @imsnif , ok, i see, thank you, i'll try to use it. anyway, i fixed that issues and now the PR is valid and has no conflicts.

@olehs0
Copy link
Contributor Author

olehs0 commented Apr 28, 2020

Hi @imsnif ! Is everything fine there, or should I improve something else ?

@imsnif
Copy link
Owner

imsnif commented Apr 28, 2020

Hey @olehs0 - great work!
I will write more comments about the code, but before that - there is a behavior here I'd like changed.
Do you think we could change the "Processes" table, so that when it shrinks in size, we will always see the "rate" column? Meaning that the first column to disappear would be PID, then Connections? Or the other way around if you think it's better?

@olehs0
Copy link
Contributor Author

olehs0 commented Apr 28, 2020

Hey @olehs0 - great work!
I will write more comments about the code, but before that - there is a behavior here I'd like changed.
Do you think we could change the "Processes" table, so that when it shrinks in size, we will always see the "rate" column? Meaning that the first column to disappear would be PID, then Connections? Or the other way around if you think it's better?

Sure, i think i can swap the rate up\down column with pid, is this will be fine ?

@imsnif
Copy link
Owner

imsnif commented Apr 28, 2020

Sure, i think i can swap the rate up\down column with pid, is this will be fine ?

Do you think we can keep them where they are and only change which ones disappear when we resize?

@olehs0
Copy link
Contributor Author

olehs0 commented Apr 28, 2020

Probably it's not clear for me, how exactly user can resize the process table or all utilization tool? i guess i just swap rate with pid columns
image

@imsnif
Copy link
Owner

imsnif commented Apr 28, 2020

Don't swap the columns. "Rate" should be on the right.
In full size, we have:
Process, PID, Connections, Rate Up / Down

When resizing the window (making it smaller), we should have:
Process, PID, Rate Up / Down

IF you make it even smaller, you'll have only:
Process, Rate / Up Down

Does that make sense?

@olehs0
Copy link
Contributor Author

olehs0 commented Apr 30, 2020

@imsnif yes, thank you for your explanation, let me try to rework it

@olehs0
Copy link
Contributor Author

olehs0 commented Jun 3, 2020

sorry guys, i don't know how to implement it properly, could someone suggest something to me about the resizing ?

@imsnif
Copy link
Owner

imsnif commented Jun 6, 2020

Hey @olehs0, no worries - it's definitely a little tricky with the way things are written now.

What I like doing is starting simple, even if it means duplicating code. Then once everything works as I want it to, I try to refactor out the duplications.

I think a good direction here would be to move the ColumnCount (

pub enum ColumnCount {
) into the Table struct, and then have an if (or a match) in the render method that would deal with it.

Does that make sense? Or did you have issues in a different place? Please don't hesitate to ask. :)

@olehs0
Copy link
Contributor Author

olehs0 commented Jun 10, 2020

Morning! Yeah I see, i got the idea. Thank you for your suggestion, let my work around it

@imsnif imsnif closed this Sep 13, 2020
@imsnif
Copy link
Owner

imsnif commented Sep 13, 2020

Hey, this was closed by mistake and I don't seem to be able to re-open it. Please feel free to re-open or make another PR with this if you wish. My apologies!

cyqsimon added a commit that referenced this pull request Mar 18, 2024
* feat: add `PID` column to `Process` table

* fix(tests): populate fake data with the correct `ProcessInfo` type

* test: update snapshots

* refactor: use more idiomatic rust

* refactor: rename function from `get_proc_name` to `get_proc_info`

* refactor: only display PID when width available is highest

ref: #165 (comment)

* tests: update snapshots

* chore: update CHANGELOG

* fix: clippy warnings

* Revert "fix: clippy warnings"

This reverts commit e5f06cb.
We will do this separately for the sake of keeping a clean history

* refactor: use `u32` for PID

* refactor: more idiomatic rust

---------

Co-authored-by: cyqsimon <[email protected]>
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.

Add PID column to processes table
3 participants