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 pause button to live view #418

Merged
merged 7 commits into from
Jul 24, 2023
Merged

Conversation

bilbofroggins
Copy link
Contributor

@bilbofroggins bilbofroggins commented Jul 12, 2023

*Issue number of the reported bug or feature request: #404 *

Describe your changes
P key to pause, and when paused, U key to unpause.
Couldn't see anything built in to rich live that will automatically do this, so I just split the TUI class into a layout class (TUI) and a data class (TUIData) so that we can snapshot the data when the user pauses and show the snapshot while continuing to write to the live version of data in the background (in update_snapshot)

Previously used to put the whole _snapshot (Iterable[AllocationRecord]) within the TUI class and then extract what we cared about in the render stage (get_body) but there's a bunch of extra stuff in there it seems like is not needed. Instead, extracting the data we need in update_snapshot and then get_body can just pull from TUIData.

Testing performed
Tested an application that has a couple threads - made sure you can still arrow through them.
Sorting still works and carries over paused/unpaused states.
Ran pytest locally.

Closes: #418

@pablogsal
Copy link
Member

pablogsal commented Jul 13, 2023

I have not reviewed the code yet, but I noticed that when running memray run --live -m test test_asyncio and pausing, hitting U or P to unpause doesn't continue the process and the table does not keep updating for me.

@pablogsal
Copy link
Member

pablogsal commented Jul 13, 2023

Update: seems that it only updates when I keep pressing U

@codecov-commenter
Copy link

Codecov Report

Patch coverage has no change and project coverage change: +0.13 🎉

Comparison is base (3ca2337) 84.85% compared to head (37bc373) 84.99%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #418      +/-   ##
==========================================
+ Coverage   84.85%   84.99%   +0.13%     
==========================================
  Files          29       29              
  Lines        3631     3631              
==========================================
+ Hits         3081     3086       +5     
+ Misses        550      545       -5     
Flag Coverage Δ
cpp 84.99% <ø> (+0.13%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@bilbofroggins
Copy link
Contributor Author

Update: seems that it only updates when I keep pressing U

Oh interesting, I'm not seeing that on my side but I just put in a flag to prevent data from being copied a bunch while holding the keys down - and pushed that up. Possibly related?

@pablogsal
Copy link
Member

pablogsal commented Jul 13, 2023

Update: seems that it only updates when I keep pressing U

Oh interesting, I'm not seeing that on my side but I just put in a flag to prevent data from being copied a bunch while holding the keys down - and pushed that up. Possibly related?

Unfortunately still happens on my end after the latest commit Hummm, I think my terminal was busted or something because after retrying it works as expected! Sorry for the confusion @bilbofroggins

@pablogsal
Copy link
Member

Thanks a lot @bilbofroggins this looks fantastic. Very good job! 🚀

Can you maybe add a small test that checks the pause() and unpause() functions simulating an update in the middle?

Signed-off-by: Patrick Cunniff <pjcfifa@gmail.com>
Signed-off-by: Patrick Cunniff <pjcfifa@gmail.com>
Signed-off-by: Patrick Cunniff <pjcfifa@gmail.com>
Signed-off-by: Patrick Cunniff <pjcfifa@gmail.com>
@bilbofroggins
Copy link
Contributor Author

Thanks a lot @bilbofroggins this looks fantastic. Very good job! 🚀

Can you maybe add a small test that checks the pause() and unpause() functions simulating an update in the middle?

👍 Added

pablogsal and others added 2 commits July 18, 2023 09:01
Signed-off-by: Patrick Cunniff <pjcfifa@gmail.com>
Signed-off-by: Patrick Cunniff <pjcfifa@gmail.com>
@pablogsal
Copy link
Member

Will make another pass tomorrow! Thanks for the ping

Copy link
Member

@pablogsal pablogsal left a comment

Choose a reason for hiding this comment

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

LGTM! Excellent work @bilbofroggins

Thanks a lot for contributing to memray :)

@pablogsal pablogsal enabled auto-merge (squash) July 24, 2023 10:32
@pablogsal pablogsal merged commit ae12d50 into bloomberg:main Jul 24, 2023
32 of 33 checks passed
pablogsal pushed a commit to godlygeek/memray that referenced this pull request Jul 24, 2023
Add a key binding for pausing refreshes, and another key binding for resuming the display. Ensure that records continue to be consumed from the socket, and only the refreshes of the TUIs data are suspended.

Signed-off-by: Pablo Galindo <pablogsal@gmail.com>
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.

None yet

3 participants