-
Notifications
You must be signed in to change notification settings - Fork 368
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
feat: Merge threads in live view #589
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Marta Gomez Macias <[email protected]>
Signed-off-by: Marta Gomez Macias <[email protected]>
Signed-off-by: Marta Gomez Macias <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #589 +/- ##
==========================================
+ Coverage 92.55% 92.93% +0.38%
==========================================
Files 91 92 +1
Lines 11304 11307 +3
Branches 1581 2076 +495
==========================================
+ Hits 10462 10508 +46
+ Misses 837 799 -38
+ Partials 5 0 -5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Marta Gomez Macias <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a great start, but I'm spotting lots of little issues as I try to use it.
For one, if the process is still running and output isn't paused, then threads seem to become un-merged again as soon as the next update happens.
For another, this disables the <
and >
keys, but not the left and right arrow keys, which do the same thing (as long as the grid isn't wide enough to scroll - if it is, they scroll the grid instead).
Also, if after the tracked process dies you use the >
key to select a specific thread and then enable thread merging, the table doesn't get redrawn to include allocations from all threads, even though the header gets redrawn to say that allocations from all threads are being shown together.
Can you spend some time tracking down and fixing these issues?
Hello @godlygeek, I plan working on this by the end of the week. Thank you for your review! :) |
Signed-off-by: Marta Gomez Macias <[email protected]>
Signed-off-by: Marta Gomez Macias <[email protected]>
Without this code, you are able to keep moving among all threads while in merged mode. Once you go back to the regular mode, you won't go back to where you left but where you've been moving while in merged mode. This is a confusing behaviour and this commit blocks it. Signed-off-by: Marta Gomez Macias <[email protected]>
The merge threads attribute was not watched by the allocation table widget, so changing it made no effect. When the app was connected, the table was updated because a new snapshot was available, not because the merge threads attribute change was detected either. This commit fixes that behaviour Signed-off-by: Marta Gomez Macias <[email protected]>
Hey @godlygeek, So when I said I would work on this by the end of the week, I meant by the end of the day 😅 😂 I fixed the issues you pointed out. Can you please review it again? Thanks! |
Signed-off-by: Marta Gomez Macias <[email protected]>
Signed-off-by: Marta Gomez Macias <[email protected]>
Hello @godlygeek, Thank you for your review again, I just pushed a commit fixing that problem. Can you take another look? Thanks! Cheers! |
Signed-off-by: Marta Gomez Macias <[email protected]>
Signed-off-by: Marta Gomez Macias <[email protected]>
Signed-off-by: Marta Gomez Macias <[email protected]>
Hello @godlygeek, I have fixed some conflicts that appeared in 2f50fa7. Can you take a look again please? 🥺 Thanks! |
Signed-off-by: Marta Gomez Macias <[email protected]>
Signed-off-by: Marta Gomez Macias <[email protected]>
Issue number of the reported bug or feature request: #587
Describe your changes
This PR adds a new option in the live view called "merge threads" that summarizes all allocations in a single view. It also hides the buttons ">" and "<" because you can't move between threads in that moment.
In addition, I added some missing docs to the readme.
Testing performed
Manual testing + unit tests included in this PR