Reimplement the live TUI using textual

[ayshiff]

Closes #238

Describe your changes
This patch ports TUI using rich to textual.

Testing performed

Here is a small sketch representing the different components of the live TUI and their interactions.

Additional context

This is still a work in progress since I might need some help to:

• Add proper test support (I took a look in tests/unit/test_tui_reporter.py but couldn't find the correct way to make it work with my work).
• Prevent the default left-right arrow behavior of the textual Table to interfere with the switching threads behavior.
• Remove the horizontal data table scroll.
• Check that the dynamic behavior of the TUI is correct (it seems that some variables are wrongly updated some of the time).
• Check that the styling match with the rich version (e.g. the data table).
• Some refactoring to make everything nice.

Thanks for your help! :)

[godlygeek]

Thanks a lot for the work on this! This is quite a large PR, so I haven't yet made time to review this, but I definitely appreciate the work you've put in here!

[godlygeek]

Sorry, @ayshiff - we've owed you a response on this for far too long, and I just haven't had the bandwidth to sit down and give this a proper review.

First of all: holy cow, this is really impressive! Being able to use the mouse wheel to scroll the TUI is very, very cool. And it's much, much faster than the existing Rich TUI. So much faster that we probably need to put in some sort of a delay: top updates only once every 3 seconds by default, perhaps we ought to do something similar here...

I notice that right when the Textual TUI first appears, before the grid is populated for the first time, the live memory graph in the top right starts off narrow, and then when we start getting samples it abruptly expands to the appropriate width. That seems like a minor bug.

There's a small omission, as well: on the Rich TUI, there's a dotted line in between the "Current heap size:" line and the top of the location grid. That line isn't just a separator, it's also a "progress bar", let's say. It shows what percentage the current heap size is, compared to the maximum heap size we ever saw. I think we want to keep that, though we could pick a different way to draw it that feels more natural for Textual.

This handles different terminal sizes pretty well, but not perfectly. I notice that if there's not enough horizontal room for the Memory graph in the top right, it seems to line wrap (you'll notice this if you try making the terminal window too skinny). Also, I notice that if I fullscreen my terminal (63 lines x 239 columns, it seems), there are a few unnecessary blank lines above and below the "Current heap size" line.

To address your points:

Add proper test support (I took a look in tests/unit/test_tui_reporter.py but couldn't find the correct way to make it work with my work).

Let's hold off on worrying about tests for now. Let's get the interface as polished as we can and working just how we want it to, and then we'll figure out what automated testing we can do. We have a lot of flexibility here, and we can always choose to throw the existing tests away and start from scratch if that turns out to be what makes the most sense. I've never worked with Textual before, but the docs have a app.run_test() asynchronous context manager that seems to be used for driving the tests in Textual's own test suite, and which it looks like we ought to be able to use to drive ours as well, at least at first glance. We'll probably want to add pytest-asyncio as a test dependency if we go that route.

Prevent the default left-right arrows behavior of the textual Table to interfere with the switching threads behavior.

That's interesting - sometimes the arrows switch threads, sometimes they scroll the table! We may just need to pick different keys for these things. Perhaps < and > for switching threads instead of left and right arrow, and perhaps even a clickable button for users with mouse support enabled.

I noticed something else funny with the scroll bar as well - if I scroll the table all the way to the bottom, then switch threads, and then switch back to the first thread, I see that the scroll bar looks as though I'm at the bottom, when actually the rows being shown are the ones at the top. It looks like we may need to re-sync the scroll bar with the grid position, or re-scroll the grid to whatever position the scroll bar thinks we should be at. Maybe remember the position when we switch away from a thread, and try to scroll back to that same position when we return to that thread...

I'm not sure what the limit of what we can do for ourselves there is, and at what point we might need a feature request to the Textual devs.

Remove the horizontal data table scroll.

I wouldn't! If someone makes their terminal too narrow to be able to see all the columns, being able to scroll to see the others is a nice feature.

Check that the dynamic behavior of the TUI is correct (it seems that some variables are wrongly updated some of the time).

Can you give any specifics for that? I don't think I managed to reproduce that, but I'm not sure what I should have been looking for.

Check that the styling match with the rich version (e.g the data table).

I'm not too worried about the style matching. If we find a nicer or prettier way to display anything, I don't want to be held back because of backwards compatibility with how the old grid table looked. I'm happy to take this as an opportunity to modernize the look and feel, so I think we should make choices based on what looks and feels right, not just on what the Rich version looked like.

---

I realize that it's been a while since you sent this PR - is this still something that you're interested in pushing forward? If not, we can try to pick this up ourselves and polish it and finish it off sometime in the next month or so, but if you're interested we'd still be very glad to have your help, and I'll try to be more responsive to any future requests for comment.

I really can't stress enough: this really looks awesome already, and I'm thrilled with everything you've done so far. This is very, very cool.

[godlygeek]

I realize that it's been a while since you sent this PR - is this still something that you're interested in pushing forward? If not, we can try to pick this up ourselves and polish it and finish it off sometime in the next month or so, but if you're interested we'd still be very glad to have your help, and I'll try to be more responsive to any future requests for comment.

Ping - are you still interested in pushing this forward, @ayshiff?

[ayshiff]

I realize that it's been a while since you sent this PR - is this still something that you're interested in pushing forward? If not, we can try to pick this up ourselves and polish it and finish it off sometime in the next month or so, but if you're interested we'd still be very glad to have your help, and I'll try to be more responsive to any future requests for comment.

Ping - are you still interested in pushing this forward, @ayshiff?

Yes definitely! Sorry for the delay, I've been a little busy lately.

[godlygeek]

@ayshiff We've decided to take this PR over. Holy cow, this was a lot more work than I expected, so thanks so much for getting the ball rolling for us!

I've got one last thing I need to ask you for, though. Would you mind editing the commit message for your commit to add a DCO signoff?

If you're using the github CLI, you should be able to do that using something like:

gh pr checkout -f 274

to check out the latest commits that I've pushed, then

git rebase -i c5672788e1472cb3a6eda4a65c400bd6ca8764a7~1

then change the first line from "pick" to "edit", and save and quit. From there you can:

git commit --amend -C HEAD -s

to add the signoff to the commit message, and finally

git rebase --continue

to finish rebasing and

git push --force-with-lease

to push the updated commits.

[ayshiff]

@godlygeek Thanks a lot for taking over my work! And sorry for having been a bit busy lately.
Can't wait to see the new TUI live using textual! :)

[codecov-commenter]

Codecov Report

Attention: 19 lines in your changes are missing coverage. Please review.

Comparison is base (2defa44) 92.23% compared to head (5196081) 92.24%.
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #274      +/-   ##
==========================================
+ Coverage   92.23%   92.24%   +0.01%     
==========================================
  Files          91       91              
  Lines       10872    11029     +157     
  Branches     1499     1520      +21     
==========================================
+ Hits        10028    10174     +146     
- Misses        841      851      +10     
- Partials        3        4       +1     

Flag	Coverage Δ
cpp	85.72% <ø> (-0.20%)	⬇️
python_and_cython	95.49% <97.30%> (+0.04%)	⬆️

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

Files	Coverage Δ
src/memray/commands/live.py	94.44% <100.00%> (+20.53%)	⬆️
src/memray/commands/run.py	89.10% <100.00%> (ø)
src/memray/reporters/summary.py	100.00% <100.00%> (ø)
tests/integration/test_main.py	93.03% <ø> (+0.41%)	⬆️
tests/integration/test_tracing.py	99.81% <100.00%> (+<0.01%)	⬆️
tests/unit/test_cli.py	100.00% <100.00%> (ø)
tests/unit/test_tui_reporter.py	98.81% <98.52%> (-1.19%)	⬇️
src/memray/reporters/tui.py	96.36% <96.00%> (-2.50%)	⬇️

... and 5 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[pablogsal]

I don't think this is used anywhere

[godlygeek]

Indeed, will remove

[pablogsal]

This seems is also not used anywhere

[godlygeek]

Indeed, will remove

[pablogsal]

Is this used? Seems that we copied it to summary.py

[godlygeek]

Used only for type annotations.

[godlygeek]

oh, wait, I misread - I thought you were asking about the highlighted line in the diff, not the last line in it. Yep, that's unused now.

[pablogsal]

is this needed for something?

[godlygeek]

Not anymore! Will remove.

[pablogsal]

Appart from the small nits to delete some unneeded things: LGTM.

Fantastic work @godlygeek! 🚀 I know how difficult was to do the refactor. You did a fantastic work ✨ Outstanding!

And thanks a lot for all the work and design in the initial version @ayshiff! This was certainly NOT a small issue, so this was super useful and you helped the project a lot ❤️