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.

Sign up for GitHub

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

Jump to bottom

Colored text for readability and transfer speed display #279

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Colored text for readability and transfer speed display #279

wants to merge 11 commits into from

Conversation

kernelPanic0x
Copy link
Contributor

@kernelPanic0x kernelPanic0x commented Dec 9, 2024 •
edited
Loading

Closes #276

Added some color to make wormhole-rs transfer information more readable! 🎨 When using it daily, I found myself missing transfer speeds the most and with that some colors would be nice to better see information right away.

Here's what I changed:

  • Now shows connection type (direct/relay) and peer IP so you know what's going on with your connection
  • Added colored transfer speed display to track progress better
  • Used colors to make important info pop:
    • 🟡 Yellow for timestamps and timing info
    • 🔵 Blue for file sizes
    • 🟢 Green for filenames (the stuff you care about most)
    • 🟣 Magenta for connection type (a little easter egg in wormhole theme color)
    • ⚡ Transfer speeds and IPs in cyan (using lightning bolt since there's no cyan emoji 😆)

Everything important has its own color now. Makes it way nicer to use, especially when sending big files and you want to keep an eye on the speed and connection type. What do you think? Is this a nice addition?

@codecov Codecov
Copy link

codecov bot commented Dec 9, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 0% with 52 lines in your changes missing coverage. Please review.

Project coverage is 36.96%. Comparing base (bee63ed) to head (0ae287c).

Files with missing lines Patch % Lines
cli/src/main.rs 0.00% 47 Missing ⚠️
src/transit.rs 0.00% 5 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #279      +/-   ##
==========================================
- Coverage   37.69%   36.96%   -0.73%     
==========================================
  Files          19       19              
  Lines        3221     3265      +44     
==========================================
- Hits         1214     1207       -7     
- Misses       2007     2058      +51

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

@kernelPanic0x
Copy link
Contributor Author

Can i rerun a failed test manually? It seems to have some network related issues.

@piegamesde
Copy link
Member

Can you maybe show some screenshots of the new colors?

@kernelPanic0x
Copy link
Contributor Author

When Transmitting:
tx

When Receiving:
rx

felinira
felinira requested changes Dec 12, 2024
View reviewed changes
Copy link
Collaborator

@felinira felinira left a comment

Choose a reason for hiding this comment

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

Looking fun 🟣 🟡 🔵

A few remarks:

  • This should respect https://no-color.org/ a little better: Right now the "Connecting" and "Override" text still uses color, it works fine with the progress bar.
  • We should default to no color when used in a script, or when redirecting stderr/stdout.

cli/src/main.rs Outdated Show resolved Hide resolved
cli/src/main.rs Outdated Show resolved Hide resolved
cli/src/main.rs Outdated Show resolved Hide resolved
cli/src/main.rs Outdated Show resolved Hide resolved
@felinira
Copy link
Collaborator

felinira commented Dec 12, 2024 •
edited
Loading

I also wonder whether we want a separate command line flag to disable colors as well, in the same spirit as --no-qr #277, but that can also be done as a follow-up, or when someone asks for it. Environment variables don't work very well for everyone.

felinira
felinira requested changes Dec 13, 2024
View reviewed changes
cli/src/main.rs Outdated Show resolved Hide resolved
cli/src/main.rs Outdated Show resolved Hide resolved
cli/tests/cmd/basic.stderr Outdated Show resolved Hide resolved
cli/tests/cmd/help.stdout Outdated Show resolved Hide resolved
@felinira
Copy link
Collaborator

I will probably want to leave this open for a few more days and gather some feedback on the color choices. While we can't exactly make everyone happy, we should at least try not to regress on readability.

@piegamesde
Copy link
Member

Haven't followed closely, but does this respect the CLICOLORS standard? https://bixense.com/clicolors/

@felinira
Copy link
Collaborator

Haven't followed closely, but does this respect the CLICOLORS standard? https://bixense.com/clicolors/

That spec is amazing. It's apparently based entirely on the python implementation of string to boolean coercion.

Anyway, I think we should be checking for != 0 and != "" for those too i guess. That's what I found in other places.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@piegamesde piegamesde piegamesde left review comments

@felinira felinira felinira requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEATURE REQUEST] Enhanced Connection Information Display
3 participants
@kernelPanic0x @piegamesde @felinira