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

Add parent detection #300

Merged
merged 5 commits into from
Sep 8, 2024
Merged

Add parent detection #300

merged 5 commits into from
Sep 8, 2024

Conversation

jojo2357
Copy link
Contributor

Fixes #299

before & after. Proton mail has its children accounted for.
image

I've done three things:

  • include parent pid in processdata
  • iff the cgroup is org.gnome.Shell, lets try to lookup our parent before invariably being resolved to org.gnome.Shell
  • Enabled short-circuiting of the last some for performance

I don't expect this to be the final revision, so opening as draft to encourage discussion

@nokyan
Copy link
Owner

nokyan commented Jul 23, 2024

Hi, thanks a lot for the PR!
So far I haven't found anything wrong with it, I've had no false detections when testing around. If you believe it's ready, we can merge it I think. :)

@jojo2357
Copy link
Contributor Author

I worry only that the hardcoded "org.gnome.Shell" will not be useful on other desktop managers.

@jojo2357
Copy link
Contributor Author

Interestingly, my use case appears to have disappeared as Proton Mail is using the correct cgroups now...

@nokyan
Copy link
Owner

nokyan commented Jul 27, 2024

Interestingly, my use case appears to have disappeared as Proton Mail is using the correct cgroups now...

That's interesting indeed. It's still a useful feature though, surely Proton Mail isn't the only app affected by this.

I worry only that the hardcoded "org.gnome.Shell" will not be useful on other desktop managers.

I think that's okay for now as it doesn't harm Resources on other DEs. I'd export "org.gnome.Shell" into an array to make it easier to extend later. I've got a commit ready, I'd push it to your branch if that's okay with you.

@jojo2357
Copy link
Contributor Author

Go ahead, Im waiting for #305 anyway

@jojo2357
Copy link
Contributor Author

Side-by-side, current flatpak vers is identical to this PR. the other app, dolphin-emu seems to have figured itself out (when I move the .desktop to a location resources can read)

@nokyan
Copy link
Owner

nokyan commented Jul 27, 2024

Do you think this is ready for merging?

@jojo2357
Copy link
Contributor Author

Im not confident to merge this until I can produce a solid example of this working.

I think I will have to design my own app to do it.

@jojo2357
Copy link
Contributor Author

I found a reason, this time with libreoffice

1.6:
image
This branch:
image

@jojo2357
Copy link
Contributor Author

@nokyan do you see any other examples of this PR fixing any apps? if so, and youre fine with the changes, then im fine undrafting and merging.

@nokyan
Copy link
Owner

nokyan commented Aug 27, 2024

@nokyan do you see any other examples of this PR fixing any apps? if so, and youre fine with the changes, then im fine undrafting and merging.

I haven't yet found other apps that might benefit from this. Though I am fine with the changes and a tree view for Processes is on the roadmap at some point as well, so keeping track of a process' parent already is useful as well.

@jojo2357
Copy link
Contributor Author

Ok, ill resolve the merge conflict when i get the chance and then unmark draft

@jojo2357
Copy link
Contributor Author

Currently rr is committing crimes, and i made sure again that it works as advertised.

image

@jojo2357 jojo2357 marked this pull request as ready for review August 27, 2024 23:09
@nokyan
Copy link
Owner

nokyan commented Sep 8, 2024

Sorry, I kinda forgot about this PR 😅
Merged. :)

@nokyan nokyan merged commit d80aa35 into nokyan:main Sep 8, 2024
1 check passed
@jojo2357 jojo2357 deleted the parent-app-inheritence branch October 21, 2024 04:41
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.

App Children Not Accounted For
2 participants