ChrisTitusTech / linutil

Chris Titus Tech's Linux Toolbox - Linutil is a distro-agnostic toolbox designed to simplify everyday Linux tasks.
https://christitus.com
MIT License
1.75k stars 158 forks source link

Refactoring: Allow themes to scale with ease #151

Closed SoapyDev closed 4 weeks ago

SoapyDev commented 1 month ago

Pull Request

Title

Allow theme scaling without risk of indexing out of bounds

Type of Change

Description

This is following a suggestion that was made last week.

Converting the list to an enum to identify the themes, using the ValueEnum trait to allow clap to show the list in the terminal. This avoid any risk of indexing out of bounds while adding / removing themes making it extendable without risk of crashing the app. Also, the code in main will not need to be updated when adding / removing themes.

I added a next/prev function as unused to make sure this could be merged without risk of conflicting with any of the major refactoring I see coming up in the main or if its not desired, they can easily be deleted.

@ChrisTitusTech, if you wish to attempt to implement it yourself, go ahead. It requires to listen to a new key and call next or previous. In what context you are might matter depending on the key that you listen to.

For example, if you take t, you cannot listen to it in search mode as its a searchable character, but if you take F2 you could. Meaning, that if you want to be able to change theme in any mode, it needs to be a non searchable character else, it needs to be handled only when out of search mode or preview mode.

Testing

Manual testing

Impact

Nothing really, this is self contained and should not affect other features.

Checklist

lj3954 commented 1 month ago

ValueEnum has a derive macro, you don't need to write a manual implementation of it. I'm also not exactly sure what the point of all of these from implementations. With the type in question being a unit enum (which is the same as a usize), you should just derive Copy on it, since it has very little cost (sometimes less than passing references) and leads to cleaner code.

If we're going to do this next/previous theme feature, it's probably not the greatest idea, either, to keep transforming between these different types. Perhaps, instead, implement functions on the ThemeType directly.

i.e.

impl ThemeType {
    fn dir_color(&self) -> Color {
        match self {
            Self::Default | Self::Compatible => Color::Blue,
        }
    }
    fn cmd_color(&self) -> Color {
        match self {
            Self::Default => Color::Rgb(204, 224, 208),
            Self::Compatible => Color::LightGreen,
        }
    }

etc

SoapyDev commented 1 month ago

@lj3954 , thanks for the comment. I re-commited the code following your recommandation. Its indeed cleaner and very simple.

Thanks for your time.