-
-
Notifications
You must be signed in to change notification settings - Fork 298
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
fix(ui): upgrade tui to latest version and fix breaking changes #190
Conversation
Hi @imsnif ! I'm a tad busy over the next couple of days, but I should have the time later this week or early next week to give it a look! I've also got an outstanding PR for adding a nicer interface to building |
Hey @imsnif ! Sorry for the delay! I think I'll take a look at this PR tomorrow! I got my changes merged into I'll give this a proper review tomorrow, but from a cursory glance, it looks quite nice :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not too much to say! Only two small comments. It looks stellar and is nice to have bandwhich on a newer TUI version!
Cargo.toml
Outdated
@@ -21,7 +21,7 @@ exclude = ["src/tests/*", "demo.gif"] | |||
[dependencies] | |||
pnet = "0.26.0" | |||
ipnetwork = "0.16.0" | |||
tui = { version = "0.6", default-features = false, features = ["crossterm"]} | |||
tui = { version = "0.10", default-features = false, features = ["crossterm"]} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be able to change this to 0.12 without breaking anything. I've given it a test on my machine and no breaking API changes have been made since 0.10 I don't think.
src/display/components/help_text.rs
Outdated
.render(frame, rect); | ||
|
||
let text = Span::styled( | ||
format!("{}{}{}", pause_content, tab_text, dns_content), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sticking strings together in Rust always gives me a bit of grief, but I think this also works here:
[pause_content, tab_text, dns_content].concat(),
To me it seems a tiny bit cleaner and is, apparently, about 3x faster because it can pre-compute the size of the final string I think. We are talking nanoseconds though, so it really doesn't matter and is a matter of style I think :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aha, good to know - thanks! I made the change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, @TheLostLambda - I sent you an email on a different subject. Mentioning it here because I remember there were some spam-filtering issues between us in the past. :)
Recently, as part of adding Windows support, we switched our TUI backend to Crossterm.
This broke the build for FreeBSD here: #188. In short, TUI was depending on an old version of Crossterm that had a bug when building for FreeBSD.
To fix this, I issued a hot fix here: #189. This upgraded TUI from 0.5 to 0.6.2, which included the FreeBSD fix, but did not have all the breaking API changes from the latest published TUI version (0.10.0).
While this did solve the problem, it did break the UI. When running locally, I was unable to see the "Total Utilization" line at the top, as well as other small glitches.
This PR solves this issue by upgrading tui to the latest version (0.10.0), accounting for all the API changes.
For ease of review, I'm going to sum the relevant parts here:
Text
TUI widget, we're now usingSpan
Frame::render_widget
Paragraph
, we can provide theSpan
s directly instead of wrapping them in something we can iterate over. (MUCH better IMO :) ).Table
, we now have to provide the widths as tuiConstraint
instances (not so nice for our use case, but the extra allocations are very minimal I think).Crossterm
backend with the system stdout. In our case this is not very elegant, but we should still get cross-platform functionality from the ruststd::io
itself, and it doesn't interfere with our tests - so I think it's fine.Table
columns in a 2-column table-layout, because otherwise one of the titles (Remote Addresses) got cut off.hide_cursor
/show_cursor
before everyflush
. I updated these in the tests - I don't think there is any user facing effect.@TheLostLambda - if you have time to review this (can wait a few days or even a week if you don't have the time) that would be great. If not I'll get a different pair of eyes on this.