Add colors to tar check menu

[intgr]

Using the same color scheme as review menu
in commit a800fb2 (PR #161)

[vn971]

Hey, here I'm somewhat unsure here. The install file itself could be dangerous, but showing the file is not a dangerous operation.

Should we change it to something like [I]=show [red]install file[/red] ? Or something else?

Alternatively, make all actions except installation to be slightly green or even neutral? As far as my preferences go, I think I'd consider making everything neutral except install file text and ok, proceed -- because those are the only things or operations that "could harm"

[intgr]

I'll give that a try.

[intgr]

Is this what you had in mind?

[intgr]

I didn't realize that the tar check menu already has colors. I see how adding color to other options might reduce visibility of the previously red options.

Making them yellow seems to make them stand out more:

[Morganamilo]

Just mentioning yellow can look quite bad on a light terminal.

[vn971]

Looking at the screenshots, I think I'm definitely aligning more to the "traffic lights" rule you mentioned before. In this case it'd be one potentially dangerous operation [O]=ok, proceed and highlighting the potentially dangerous parts of SUID, maybe install file as well. Technically all files should be reviewed, because a file placed in cron directory is just as "dangerous" as an install file

[intgr]

This is what darker yellow would look like (better for white background)

[vn971]

I was thinking of something like that:
AFK now, will be back a bit later today

Maybe showing install file / SUID files could be yellow (or a less intense red) color as well, I like some parts of your screenshot above. *I'll back later

[vn971]

I played some more with the idea, and no matter what I do, this kinda doesn't make sense for me. The only really dangerous action is to install the package, but the parts the user should be paying attention to are not those. I'm thinking of the following design

Any thoughts on the "dangerous action" / "potentially dangerous details" concept mismatch?

[intgr]

My initial reason for creating these pull requests was that I found it hard to tell, where one option ends and the next one starts. So I'd prefer to have something that more clearly separates the options. Color and bolding the keyboard shortcut made them quite distinct. But without color, the effect is less pronounced:

I agree that the colors (like in #163 (comment)) do seem excessive. On the other hand, it slightly bothers me that the colors in the menus work differently now (compared to #161). But I can live with that. :)

[vn971]

I've finally got back to the MR 😅 (sorry, it's been a really long time, I acknowledge). I'm trying the PR out at the moment, I have a few questions

[vn971]

I tried it out now, I think I actually like to keep the old if condition above. Without it, my terminal alarms me: "hey, you have both SUID and an installation file at your hands, be sure to review it!" (see screenshot)

However, after clicking on checking these warnings, you see Package packagename-1.0.0-x86_64.pkg.tar.zst has no SUID files. That's confusing to me, I like the old conditional display better

[vn971]

I'm willing to restore the if condition and try it out then, maybe that'll be good to keep everyone satisfied and improve the display :)

[vn971]

This gives a compiler warning:

error: literal with an empty format string
   --> src/tar_check.rs:114:35
    |
114 |         eprint!("{}{}, ", "[E]".bold(), "=list executable files");
    |                                         ^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: `-D clippy::print-literal` implied by `-D warnings`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#print_literal
help: try this
    |
114 -         eprint!("{}{}, ", "[E]".bold(), "=list executable files");
114 +         eprint!("{}=list executable files, ", "[E]".bold());
    | 

I'll revert to inlining the "static" text as the compiler suggests, but will keep the .bold() part you've introduced

[vn971]

Merged!

Before:

After:

[intgr]

Thanks! :) I'm fine with this, it makes the available choices clearer without excessive color.

"hey, you have both SUID and an installation file at your hands, be sure to review it!"

Yeah, I think I commented the if out just to test the output. Didn't intend to leave it like that.