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

Port the tree reporter to textual #499

Merged
merged 1 commit into from
Nov 30, 2023
Merged

Conversation

pablogsal
Copy link
Member

Now that we are using Textual for the live mode, we can port the tree
reporter to be a live Textual App. This has plenty of advantages over
the static version as it offers interactive exploration of the tree, as
well as the possibility of using different screens for showing detailed
information about allocations such as the source and metadata.

Signed-off-by: Pablo Galindo [email protected]

Issue number of the reported bug or feature request: #

Describe your changes
A clear and concise description of the changes you have made.

Testing performed
Describe the testing you have performed to ensure that the bug has been addressed, or that the new feature works as planned.

Additional context
Add any other context about your contribution here.

@godlygeek
Copy link
Contributor

Some notes:

  • I love this!

  • We should probably have a toggle for "uninteresting" frames, like we do for the flame graph. Currently every import shows 6 <frozen importlib...> descendents

  • I'd like to see the function names in a different color than the percentages (as it is, I think the function names don't stand out enough)

  • I'd like to reuse the same module name shortening that we did for the live TUI. I think it helps to make the screen a lot less cluttered when there's not a ton of redundant paths

  • I'm a bit confused by the top line here. It seems to have two widgets on it - clicking the leftmost one (looks like a circle) pops up the command palette - OK, fair enough, but the command palette isn't that useful, especially if we haven't added anything custom to it. One thing it can be used for is toggling between light and dark mode - but when I toggle it to light mode, I get colors with fairly poor contrast...
    image
    That cyan is not great, but granted that might be partially my terminal settings...

  • The top line has another interactable widget, with the text "TreeApp" on it. When I click that, it just... adds 2 blank lines, one above "TreeApp" and one below? I have absolutely no idea what that is 😆

  • Anyway, if we can get rid of that top line entirely, I think that would be an improvement

  • The popup window is very cool, but when I leave my terminal emulator at its normal size, I wind up with a teeny tiny popup in just the corner, rendered with a scrollbar if the source code is wider than that little window:
    image

  • I think I'd rather this be shown in the center than the top left, and I'd rather the window use more of the available width (a maximum is fine, but that maximum should probably leave space for at least ~120 characters - that's a pretty common width I've seen people use for their source code in the wild)

  • The scrollbar is particularly annoying in that you can't interact with it using the keyboard - click and drag works, but pressing the left or right arrow key closes the popup instead of scrolling, which isn't the most user friendly

  • Speaking of clicking, you can click and drag to highlight code in the text box, and you can click to select individual messages on the bottom. That's probably not desirable...
    image

Overall, I think this is excellent, and a huge improvement over the existing non-interactive tree reporter!

@godlygeek
Copy link
Contributor

Ya know... an alternative to the popup might be to split the window into two panes, both always shown - one would be focusable and let you navigate through the tree, and the other would always display info about the highlighted node. We could even support both - an always-on display for big terminals, and a popup when there's few enough rows/columns that we want to prioritize space efficiency over usability...

@pablogsal
Copy link
Member Author

@willmcgugan do you know if there is an easy way to have a TextArea widget that's frozen and allows highlighting of lines? If not, what would be the best way to get something similar?

@willmcgugan
Copy link

@pablogsal Can't think of an straightforward way to do that, but it shouldn't be difficult to add to the TextArea. If you add a feature request to the repo, I think we could get it in the next version.

@pablogsal pablogsal force-pushed the tree_textual branch 8 times, most recently from 39aeb42 to 7dc2156 Compare November 22, 2023 18:12
@codecov-commenter
Copy link

codecov-commenter commented Nov 22, 2023

Codecov Report

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

Comparison is base (3f0447b) 92.21% compared to head (793cfe4) 92.30%.

Files Patch % Lines
src/memray/reporters/tree.py 97.14% 5 Missing ⚠️
tests/utils.py 71.42% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #499      +/-   ##
==========================================
+ Coverage   92.21%   92.30%   +0.09%     
==========================================
  Files          91       91              
  Lines       11030    11212     +182     
  Branches     1526     1553      +27     
==========================================
+ Hits        10171    10349     +178     
- Misses        854      858       +4     
  Partials        5        5              
Flag Coverage Δ
cpp 85.66% <ø> (+0.02%) ⬆️
python_and_cython 95.52% <98.37%> (+0.04%) ⬆️

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

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

Copy link
Contributor

@sarahmonod sarahmonod left a comment

Choose a reason for hiding this comment

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

small nitpick: can we get a slightly bigger screenshot? It looks like the previous one was about 5000px wide, and the new one is only around 1800px wide. This makes the text harder to read.

@pablogsal
Copy link
Member Author

small nitpick: can we get a slightly bigger screenshot? It looks like the previous one was about 5000px wide, and the new one is only around 1800px wide. This makes the text harder to read.

Done

Copy link
Contributor

@godlygeek godlygeek left a comment

Choose a reason for hiding this comment

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

I've pushed some more fixup commits that get this to a point where I'm happy to merge it. One last outstanding thing: the screenshot will need to be regenerated with those changes. I made some slight tweaks to the UI: the right column is a bit bigger (40% of the screen width, up to a maximum of 100 characters), and function names show up in bold white rather than in red/yellow/green to help them stand out better. Also, I fixed an issue where a node would get a folder icon even though it didn't have children (that doesn't seem to have affected the screenshot, but did affect some of the test snapshots).

When you regenerate the screenshot, please capture a node that includes text in the text box, so it's not just a weird empty box 😄

@godlygeek godlygeek force-pushed the tree_textual branch 2 times, most recently from d623ef7 to 35d2000 Compare November 28, 2023 23:00
Now that we are using Textual for the live mode, we can port the tree
reporter to be a live Textual App. This has plenty of advantages over
the static version as it offers interactive exploration of the tree, as
well as the possibility of using different screens for showing detailed
information about allocations such as the source and metadata.

Signed-off-by: Pablo Galindo <[email protected]>
@pablogsal pablogsal enabled auto-merge (rebase) November 30, 2023 14:39
@pablogsal pablogsal merged commit 22bc40e into bloomberg:main Nov 30, 2023
34 checks passed
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.

5 participants