Skip to content

Conversation

@gepbird
Copy link

@gepbird gepbird commented Aug 16, 2025

Description

When you have a single GPU, you don't need to see it's full name, "GPU" is fine.

image

Issue

In a smaller terminal window the legend for the memory widget was not visible, because the GPU name tried to take up too much space.

image

When resizing the terminal window or the widget, it's better. Reading it is still a bit harder, as the TODO comment suggests in the code:

image

Testing

If relevant, please state how this was tested. All changes must be tested to work:

If this is a code change, please also indicate which platforms were tested:

  • Windows
  • macOS
  • Linux

Checklist

If relevant, ensure the following have been met:

  • Areas your change affects have been linted using rustfmt (cargo fmt)
  • The change has been tested and doesn't appear to cause any unintended breakage
  • Documentation has been added/updated if needed (README.md, help menu, doc pages, etc.)
  • The pull request passes the provided CI pipeline
  • There are no merge conflicts
  • If relevant, new tests were added (don't worry too much about coverage)

@gepbird gepbird marked this pull request as ready for review August 16, 2025 15:08
@ClementTsang
Copy link
Owner

ClementTsang commented Aug 16, 2025

Hmm. Not 100% sure about this one at first glance, might be fine with this being the default setting with the old setting being an option. Let me think about it for a bit and get back to you.

Slightly related, I was also going to add a fix to limit the length of the GPU name in the legend based on width + align it better, but I do agree the default behaviour being just "GPU" for a single GPU is probably fine.

@jamartin9
Copy link
Contributor

jamartin9 commented Aug 16, 2025

Slightly related, I was also going to add a fix to limit the length of the GPU name in the legend based on width + align it better

We used to use a 'short name' that was derived from the last two parts of the string ie '1050 Ti'. While not really conforming to the 3 letter convention it was less harsh on the legend sizing with multiple gpus.
Edit: amd names may need changed to fit conventions too.

@codecov
Copy link

codecov bot commented Aug 26, 2025

Codecov Report

❌ Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 42.08%. Comparing base (d55e1a6) to head (5437c1b).

Files with missing lines Patch % Lines
src/canvas/widgets/mem_graph.rs 0.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1791      +/-   ##
==========================================
- Coverage   42.09%   42.08%   -0.01%     
==========================================
  Files         115      115              
  Lines       16208    16211       +3     
==========================================
  Hits         6823     6823              
- Misses       9385     9388       +3     
Flag Coverage Δ
macos-14 37.34% <0.00%> (-0.01%) ⬇️
ubuntu-latest 43.64% <0.00%> (-0.01%) ⬇️
windows-2022 37.70% <0.00%> (-0.01%) ⬇️

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.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@gepbird
Copy link
Author

gepbird commented Sep 7, 2025

Hmm. Not 100% sure about this one at first glance, might be fine with this being the default setting with the old setting being an option. Let me think about it for a bit and get back to you.

What do you think, is the current approach fine? Personally I like that it aligns well with just 3 characters, but I understand having a special case for single GPUs is not the best.

We used to use a 'short name' that was derived from the last two parts of the string ie '1050 Ti'. While not really conforming to the 3 letter convention it was less harsh on the legend sizing with multiple gpus.
Edit: amd names may need changed to fit conventions too.

By the way, why was that removed? I'm happy to bring it back for the multiple GPUs case.

@ClementTsang
Copy link
Owner

Sorry for the delay - just to answer the questions:

What do you think, is the current approach fine? Personally I like that it aligns well with just 3 characters, but I understand having a special case for single GPUs is not the best.

To be honest I think it's fine to have a special case for single GPUs - but it might just make more sense to make it so there's an option to choose either short simple GPU names (GPU0, GPU1) or the longer ones. I would be fine with making short GPU names the default as well.

In a future PR I can also make it so the longer GPU names don't clash as badly even if used.

By the way, why was that removed? I'm happy to bring it back for the multiple GPUs case.

Don't think it was an active choice as far as I'm aware?

@ClementTsang ClementTsang self-assigned this Oct 8, 2025
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.

3 participants