tui: allow theming selected rows in hops table

[PerchunPak]

Previously, it used hops-table-row-active-text-color for background (applied it to foreground, then for some reason applied inversion filter (switch foreground and background places)). This resulted in using bg-color as foreground.

Now, there is just a new option for that, and there is no filter. I have changed the default value for hops-table-row-active-text-color (because it was technically a setting for background color, not foreground) to black (default value for bg-color).

Tested with #1717, but did not include it in this PR.

Closes #1735

[fujiapple852]

@PerchunPak did you try running this? When I do, I see all rows in the table as selected/highlighted which is not what we want as there is no way to tell which row is currently selected.

This is because the call to row_highlight_style was removed.

I like the idea of making the background of the selected row configurable, but it needs to be done such that the default styling remains as it is today and that the currently selected row is visible.

[PerchunPak]

The reason why this PR didn't work, is because is_in_round was always true (#1717 attempted to fix it), but this line

This is because the call to row_highlight_style was removed.

made me realize that I can change the modifier in that row_highlight_style. So this PR doesn't change anything old (unlike the previous version), just adds two new options for theming. Applying the reverse filter is just theming-unfriendly (you can't really make the colors work)

[fujiapple852]

@PerchunPak I adjust the defaults to match the current style, so now:

hops_table_row_selected_bg: Gray
hops_table_row_selected_text: Black

[fujiapple852]

@PerchunPak the remaing issue is selected inactive rows. See hop 12 in this screenshot, it should be dimmed like hop 11
as both are inactive, however as hop 12 is selected it shows incorrectly now that we do not use the REVERSED style.

I think we'll need:

• hops_table_row_active_selected_bg (renamed from hops_table_row_selected_bg)
• hops_table_row_active_selected_text (renamed from hops_table_row_selected_text)
• hops_table_row_inactive_selected_bg (new)
• hops_table_row_inactive_selected_text (new)

[fujiapple852]

@PerchunPak pushed changes to add the 4x needed styles. I'm not yet convinced if the complexity of having 4x styles for selected row (vs INVERSE) is justified. What do you think?

[PerchunPak]

What do you think?

I still don't really know what option to use to get inactive hops, just using --udp doesn't work for me

trip --udp -4 google.com

I'm not yet convinced if the complexity of having 4x styles for selected row (vs INVERSE) is justified

Yes, I didn't expect this to add so much code complexity, but I would still like to customize the color. It is not a big deal though

vs

[fujiapple852]

@PerchunPak

I still don't really know what option to use to get inactive hops

It happens when each round of tracing follows a different path through the network with a different length (number of hops). This is typically due to ECMP, which normally only impacts UDP and TCP traffic.

Imagine a trace from a Src to a Dest such that the path followed* in rounds 1 and 2 is as follows:

• Round 1: Src -> A (ttl=1) -> B (ttl=2) -> C (ttl=3) -> Dest (ttl=4)
• Round 2: Src -> A (ttl=1) -> B (ttl=2) -> Dest (ttl=3)

If we combine all round by ttl (as Trippy does) we get the following table:

Ttl	Hosts	Round 1	Round 2
1	A	Active	Active
2	B	Active	Active
3	C, Dest	Active	Active
4	Dest	Active	Inactive

The columns round 1 and round 2 shows the active/inactive status as observed during that round. Here we can see that during round two, no probe responded for ttl=4 as the path through the network was shorter. Trippy will report ttl=4 as a previous round did respond for ttl=4, but it will be marked as inactive for the current round.

It is of course entirely possible that in rounds 3..N the probe with ttl=4 will respond and therefore the ttl will be shown as active once more.

To stress, a ttl not reporting in a given round does not indicate a problem with that host, it only indicates that the probe followed a different path through the network which took fewer hops than the previous path.

Observing this will depend entirely on where the Src and Dest hosts are and how traffic is routed between them. Usually it can be seen with some trial end error over different Dest hosts when using UDP or TCP probes. Networks with heavy use of ECMP (such as the big cloud providers) tend to show this much more frequently.

*In reality, each probe within a round is an independent trial, so we cannot say anything about the path a probe follows to a given ttl within a round. Trippy has some tricks to make this much more likely however.

[fujiapple852]

but I would still like to customize the color

@PerchunPak as would I. I believe the vast majority of the Trippy TUI can have a custom bg/text colour, though there are a handful of exceptions such as this and it would be good to close the gaps.

Moreover, I'd like Trippy to have first class support for named themes, perhaps included by default. I wrote a little about it here. Let me know if you're interested in working on that.

[PerchunPak]

Thanks for the explanation. I still couldn't get the skipped hops, so instead I just patched that function

diff --git a/crates/trippy-core/src/state.rs b/crates/trippy-core/src/state.rs
index 7d9da0a..4db5379 100644
--- a/crates/trippy-core/src/state.rs
+++ b/crates/trippy-core/src/state.rs
@@ -512,7 +512,7 @@ impl FlowState {
     }
 
     const fn is_in_round(&self, hop: &Hop) -> bool {
-        hop.ttl <= self.highest_ttl_for_round
+        hop.ttl <= (self.highest_ttl_for_round - 3)
     }
 
     fn target_hop(&self) -> &Hop {

I like the current solution, much better than what I had staged right before you committed. Personally, I wouldn't dim the background of skipped hops, but that's just my taste.

Moreover, I'd like Trippy to have first class support for named themes, perhaps included by default. I wrote a little about it #411 (comment). Let me know if you're interested in working on that.

I already have four catppuccin themes ready in https://github.com/PerchunPak/ctp-trippy/tree/main/themes, but they are not yet updated to the latest changes here. I can make other popular themes as well

I would imagine the --tui-theme flag being kind of an override for the default values, so settings would be resolved in this order: Defaults -> Theme -> User config, instead of just Defaults -> User config like it is now. Defining a custom pallet would be great, so the theme wouldn't just have rgb values (btw, you should strip # when parsing colors, so the user could write #123456 instead of 123456. Just a convenience thing).