Add config option to set presets #85

uttarayan21 · 2021-05-08T12:05:08Z

Fixes #83

macchina can now read from a config Linux ~/.config/macchina/macchina.toml check dirs-rs repo for windows and linux Will warn if config is invalid. Can use `--export-config` to export the configuration options from the commandline

grtcdr · 2021-05-08T12:12:51Z

We're missing color and separator_color

uttarayan21 · 2021-05-08T12:14:21Z

Oh I just removed a few of stuff from the config just to show that the config can work without all the config options already set.

grtcdr · 2021-05-08T12:15:41Z

Oh great!

This also made me realize that Xinitrc is setting itself in one of the environment variables that tell us which DE you're running, I'll add a new exception for Xinitrc ASAP.

uttarayan21 · 2021-05-08T12:17:53Z

Oh don't worry about that. I have a bit of a quirky setup which makes Xinitrc show up as an item in lightdm's DM selection menu.
So that's why macchina thinks Xinitrc is my dm.

This is the package if you are curious xinit-xsession

grtcdr · 2021-05-08T12:27:01Z

Oh don't worry about that. I have a bit of a quirky setup which makes Xinitrc show up as an item in lightdm's DM selection menu.
So that's why macchina thinks Xinitrc is my dm.

No worries, we still have to account for the quirkiness of some setups, if you look around libmacchina you'll notice certain operations and exceptions that seem weird at first, but they were added because I noticed weird output on some people's machines.

EDIT: I added the exception, haven't published the release yet.

grtcdr · 2021-05-08T12:35:16Z

I think the next step for the config file is to allow for custom theming, the way we can go about is to initialize a placeholder theme in theme.rs whose fields will take the values of say:

separator_glyph = "->"
bar_glyph = "o"
// The opening bar delimiter
bar_delimiter_open = "("
// The closing bar delimiter
bar_delimiter_close = ")"

And if we're really ambitious we can allow for custom abbreviations, here's a little example:

window_manager_string = "WM"
desktop_environment_string = "DE"
...

The key naming choice is up for debate, I'm not sure window_manager_string is that great of a name.

This will most likely break the preset theme that the user specified in their config, so we have to prioritize either the custom theme, or the preset theme.

uttarayan21 · 2021-05-08T16:04:53Z

Ah okay so I'll have to look into dividing the config into theme ~~sections~~ files.

Something like

[config]
palette = true
no_color = false
no_separator = false
no_bar_delimiter = false
no_title = true
no_ascii = true
no_box = true
bar = true
random_color = false
random_sep_color = false
short_uptime = false
short_shell = true
theme = 'Boron'
custom_ascii = '/home/fs0c131y/.config/ascii/archlinux.ascii'

and
Mytheme.toml

[theme]
name = 'MyTheme'
window_manager_string = 'WM'

Edit: Hmmm it seems that the toml spec cannot have multiple keys with same name so we will have to split the themes into seperate files.

grtcdr · 2021-05-08T16:58:36Z

Edit: Hmmm it seems that the toml spec cannot have multiple keys with same name so we will have to split the themes into seperate files.

Separate files is definitely a better idea.

grtcdr · 2021-05-08T19:37:23Z

SInce you're using dirs to retrieve the config directory, I'll remove the home dependency which is being used to get the home directory to count local packages for certain package managers and use dirs instead.

EDIT: This concerns libmacchina actually, forgot to mention that.

uttarayan21 · 2021-05-09T07:40:25Z

Okay so I was looking into serializing themes from a file and I have a few design related questions.

Should we keep the hydrogen, lithium and the other themes in the theme.rs or should we seperate them into Hydrogen.toml and similar files ?
IMO It should be able to perform just as a single binary so I'd keep at least a default theme in it.
Should we keep the theme.rs as is and make a separate module for parsing themes ?

grtcdr · 2021-05-09T08:41:49Z

Okay so I was looking into serializing themes from a file and I have a few design related questions.

* Should we keep the hydrogen, lithium and the other themes in the theme.rs or should we seperate them into Hydrogen.toml and similar files ?
  IMO It should be able to perform just as a single binary so I'd keep at least a default theme in it.

The single binary route is probably the best option, is it possible to split up themes into separate toml files and still be able to use them as if they were built-in? (most likely not)

* Should we keep the theme.rs as is and make a separate module for parsing themes ?

I guess so, the intent behind theme.rs is to provide a basic trait that different themes can implement to create new built-in themes, so a theme toml parser should be done outside of theme.rs as it feels more like an extension to the theming capabilities.

uttarayan21 · 2021-05-09T17:58:24Z

Okay so I'm going to implement it as such

Check theme name from config.
If theme matches inbuilt themes then display that.
else try to parse a file in dirs::data_dir()/macchina/themes/ folder.
If everything else fails, default to the hydrogen theme.

grtcdr · 2021-05-10T08:53:03Z

Sounds good!

EDIT: I'd like to mention that although this feature isn't breaking per se, it still is significant that it warrants a jump in the versioning of the program, so when bumping the version number, it should be v0.8.0.

uttarayan21 · 2021-05-10T15:57:25Z

Okay so I was implementing the custom themes and I think I have to restructure the themes module quite a bit to make both inbuilt and custom themes work at the same

Also since the theme argument used to be an enum I'll need to change it to a string to work with custom values.

Also this will break auto generated autocomplete for --themes argument, (since we don't know all the variants anymore) but a custom one can be written.

Edit:
Also a question about ReadoutKey.get_common_name(), should it also be theme-able ?

grtcdr · 2021-05-10T16:58:34Z

Also since the theme argument used to be an enum I'll need to change it to a string to work with custom values.

We can do this without removing the enum, when matching the --theme argument, if the argument isn't one of the default themes, then we can assume the argument is a path to a file.

Okay so I was implementing the custom themes and I think I have to restructure the themes module quite a bit to make both inbuilt and custom themes work at the same

How so? I feel like the only change that needs to happen is a placeholder theme that is filled with the values found in a given theme.toml, but then again, I may be missing something.

Also a question about ReadoutKey.get_common_name(), should it also be theme-able ?

Um, I think so?

Key names are considered an AbbreviationType and should go through get_common_name(), so get_common_name() should be responsible for custom key names too.

But I'm sure you can bypass this process if custom key names are provided in the config.

uttarayan21 · 2021-05-10T17:17:49Z

We can do this without removing the enum, when matching the --theme argument, if the argument isn't one of the default themes, then we can assume the argument is a path to a file.

    #[structopt(
    short = "t",
    long = "theme",
    default_value = "Hydrogen",
    possible_values = & theme::Themes::variants(),
    case_insensitive = true,
    help = "Specify the theme"
    )]
    pub theme: Option<theme::Themes>,

Theme argument directly take enum values only and AFAIK we can't make new enum variant's at runtime with custom theme names.
And matching against the enum variants is done automatically by structopt/clap so we can't add additional logic there.

How so? I feel like the only change that needs to happen is a placeholder theme that is filled with the values found in a given theme.toml, but then again, I may be missing something.

This is mostly because most of the getter setter functions are redundant since all of them are basically the same.
So I was thinking about a maybe default implementation and overwrite it when necessary.
Also the new fucntion needs to be removed from the trait to a standard impl since CustomTheme cannot have a new function since we need a name for the theme to be provided.

But I'm sure you can bypass this process if custom key names are provided in the config.

I'll look into it after I have completed the rest of the theming.

grtcdr · 2021-05-10T17:26:24Z

Theme argument directly take enum values only and AFAIK we can't make new enum variant's at runtime with custom theme names.
And matching against the enum variants is done automatically by structopt/clap so we can't add additional logic there.

That is true, I'm sort of getting rusty at writing Rust code since most of my time has been taken up by PHP, and so I keep forgetting these details.

This is mostly because most of the getter setter functions are redundant since all of them are basically the same.

I did notice this a while back, you're absolutely right.

So I was thinking about a maybe default implementation and overwrite it when necessary.

Sounds good :)

Also the new fucntion needs to be removed from the trait to a standard impl since CustomTheme cannot have a new function since we need a name for the theme to be provided.

So a custom theme is not a Path but is extracted from the entries of the macchina directory within dirs::data_dirs?

EDIT: Totally off-topic, but, I noticed you were using bat, it's awesome!!

uttarayan21 · 2021-05-10T17:32:57Z

Yes I was thinking something along the lines of

macchina --list-themes
Hydrogen ( Inbuilt )
Lithium ( Inbuilt )
Proton
Carbon

for listing themes and the themes themselves will simply be a toml file in dirs::data_dirs()/macchina/themes folder.

grtcdr · 2021-05-10T17:34:05Z

I'm all up for this!

It looks to me like the configuration system is tightly joined together and it's pretty dope!

uttarayan21 · 2021-05-10T17:37:30Z

Haha, It's probably gonna take a while to be honest, but I'll keep you posted.

uttarayan21 · 2021-05-10T19:00:49Z

Huh It seems that rust doesn't have fields in traits so we still need to do the getter/setter madness until the rfc is merged.

grtcdr · 2021-05-10T22:00:23Z

Oops, forgot to tell you about that lol.

I went through a similar situation yesterday, but I felt as if fields in a trait is not really the purpose of traits. Can you link me the RFC so I can get an understanding of the reasoning behind why Rust doesn't let you do this by default and why people want to see it get merged? (other than removing redundancy)

uttarayan21 · 2021-05-11T07:08:12Z

Sure,
Here you go.
https://github.com/nikomatsakis/fields-in-traits-rfc/blob/master/0000-fields-in-traits.md
rust-lang/rfcs#1546
https://internals.rust-lang.org/t/fields-in-traits/6933

grtcdr · 2021-05-11T10:13:45Z

Sure,
Here you go.
https://github.com/nikomatsakis/fields-in-traits-rfc/blob/master/0000-fields-in-traits.md
rust-lang/rfcs#1546
https://internals.rust-lang.org/t/fields-in-traits/6933

Thank you so much!

This is a basic POC implementation. Needs a lot of cleanup and fixing.

uttarayan21 · 2021-05-14T20:21:09Z

So I've done a basic implementation of the themes.
Had to restructure a lot of the stuff but custom themes are working now.

grtcdr · 2021-05-16T16:08:50Z

There's one thing we might want to address before merging, that is the conflict between hide and show_only within the configuration file.

Is patch_args the appropriate place to set a condition for their coexistance?

uttarayan21 · 2021-05-16T16:23:05Z

Is patch_args the appropriate place to set a condition for their coexistance?

I mean it would work but I think it should be done in a separate function like check_conflicts.

uttarayan21 · 2021-05-16T16:26:08Z

Or possibly directly inside of from_config and return an error if it has conflicts.

grtcdr · 2021-05-16T16:26:28Z

Is patch_args the appropriate place to set a condition for their coexistance?

I mean it would work but I think it should be done in a separate function like check_conflicts.

Alrighty, I could take care of it if you'd like.

uttarayan21 · 2021-05-16T16:27:31Z

Alrighty, I could take care of it if you'd like.

Sure ! I also have some more changes locally I need to commit.

grtcdr · 2021-05-16T16:29:00Z

Alrighty, I could take care of it if you'd like.

Sure ! I also have some more changes locally I need to commit.

Great :)

Fixed naming and paths. Added some docs.

uttarayan21 · 2021-05-16T16:40:09Z

I'll include a custom theme
Give me a second

grtcdr · 2021-05-16T16:46:13Z

I'll include a custom theme
Give me a second

Sure!

Added example theme in theme/Carbon.json Themes support true color but config file doesn't

uttarayan21 · 2021-05-16T16:49:21Z

A screenshot of theme I included.

uttarayan21 · 2021-05-16T16:54:26Z

A few notes.

theme field should have the exact filename for custom themes without extensions. i.e. for Carbon.json -> "Carbon"
Custom themes support truecolor (24-bit) and 256 (8-bit) colors in addition to the 4-bit color pallet.
Both theme and config options are case-sensitive.

grtcdr · 2021-05-16T17:19:50Z

A few notes.

* `theme` field should have the exact filename for custom themes without extensions. i.e. for `Carbon.json` -> `"Carbon"`

* Custom themes support truecolor (24-bit) and 256 (8-bit) colors in addition to the 4-bit color pallet.

* Both theme and config options are case-sensitive.

Thanks for the clarification, I'll make sure to include this when I update the READMEs.

grtcdr · 2021-05-17T13:07:46Z

I managed to make the config conflict check work and also report which keys are conflicting, check it out and tell me if anything needs to be changed.

uttarayan21 · 2021-05-17T13:13:41Z

Looks great but I think you forgot to push the commit :).

grtcdr · 2021-05-17T13:14:34Z

Looks great but I think you forgot to push the commit :).

Yeah, just cleaning up a bit.

uttarayan21

Looks good !

grtcdr · 2021-05-17T13:33:41Z

Looks good !

Terrific work by the way! 🚀

uttarayan21 · 2021-05-17T13:35:22Z

Looks good !

Terrific work by the way! 🚀

Haha, thanks ! :)

uttarayan21 added 2 commits May 8, 2021 17:21

Added reading from config

bf5ba38

macchina can now read from a config Linux ~/.config/macchina/macchina.toml check dirs-rs repo for windows and linux Will warn if config is invalid. Can use `--export-config` to export the configuration options from the commandline

Merge branch 'main' of gh:macchina-cli/macchina

fa0d321

grtcdr added the enhancement label May 8, 2021

Added theme configuration options (incomplete)

b995d6a

This is a basic POC implementation. Needs a lot of cleanup and fixing.

Merge branch 'main' of gh:macchina-cli/macchina

0505206

grtcdr and others added 6 commits May 16, 2021 16:47

Added a default config file and renamed some flags

397242f

Fixed abbreviation

d7de81b

Merge branch 'main' of gh:uttarayan21/macchina

888ae9a

Update default config file

d99be4b

Merge branch 'main' of https://github.com/uttarayan21/macchina

5b279e2

Bump version to 0.8.0

457234b

uttarayan21 added 2 commits May 16, 2021 22:04

Removed unused files

29647e9

Fixed naming and paths. Added some docs.

Merge branch 'main' of gh:uttarayan21/macchina

2502442

grtcdr approved these changes May 16, 2021

View reviewed changes

Added example theme to docs.

af8ba2f

Added example theme in theme/Carbon.json Themes support true color but config file doesn't

grtcdr approved these changes May 16, 2021

View reviewed changes

Check for conflicts after patching args

381d21f

uttarayan21 commented May 17, 2021

View reviewed changes

grtcdr merged commit fa48b81 into Macchina-CLI:main May 17, 2021

Add config option to set presets #85

Add config option to set presets #85

Conversation

uttarayan21 commented May 8, 2021 • edited Loading

grtcdr commented May 8, 2021

uttarayan21 commented May 8, 2021 • edited Loading

grtcdr commented May 8, 2021

uttarayan21 commented May 8, 2021 • edited Loading

grtcdr commented May 8, 2021 • edited Loading

grtcdr commented May 8, 2021 • edited Loading

uttarayan21 commented May 8, 2021 • edited Loading

grtcdr commented May 8, 2021 • edited Loading

grtcdr commented May 8, 2021 • edited Loading

uttarayan21 commented May 9, 2021

grtcdr commented May 9, 2021 • edited Loading

uttarayan21 commented May 9, 2021

grtcdr commented May 10, 2021 • edited Loading

uttarayan21 commented May 10, 2021 • edited Loading

grtcdr commented May 10, 2021 • edited Loading

uttarayan21 commented May 10, 2021

grtcdr commented May 10, 2021 • edited Loading

uttarayan21 commented May 10, 2021

grtcdr commented May 10, 2021 • edited Loading

uttarayan21 commented May 10, 2021

uttarayan21 commented May 10, 2021

grtcdr commented May 10, 2021 • edited Loading

uttarayan21 commented May 11, 2021

grtcdr commented May 11, 2021

uttarayan21 commented May 14, 2021 • edited Loading

grtcdr commented May 16, 2021 • edited Loading

uttarayan21 commented May 16, 2021

uttarayan21 commented May 16, 2021

grtcdr commented May 16, 2021

uttarayan21 commented May 16, 2021

grtcdr commented May 16, 2021

uttarayan21 commented May 16, 2021

grtcdr commented May 16, 2021

uttarayan21 commented May 16, 2021

uttarayan21 commented May 16, 2021

grtcdr commented May 16, 2021

grtcdr commented May 17, 2021 • edited Loading

uttarayan21 commented May 17, 2021

grtcdr commented May 17, 2021

uttarayan21 left a comment

Choose a reason for hiding this comment

grtcdr commented May 17, 2021

uttarayan21 commented May 17, 2021

uttarayan21 commented May 8, 2021 •

edited

Loading

uttarayan21 commented May 8, 2021 •

edited

Loading

uttarayan21 commented May 8, 2021 •

edited

Loading

grtcdr commented May 8, 2021 •

edited

Loading

grtcdr commented May 8, 2021 •

edited

Loading

uttarayan21 commented May 8, 2021 •

edited

Loading

grtcdr commented May 8, 2021 •

edited

Loading

grtcdr commented May 8, 2021 •

edited

Loading

grtcdr commented May 9, 2021 •

edited

Loading

grtcdr commented May 10, 2021 •

edited

Loading

uttarayan21 commented May 10, 2021 •

edited

Loading

grtcdr commented May 10, 2021 •

edited

Loading

grtcdr commented May 10, 2021 •

edited

Loading

grtcdr commented May 10, 2021 •

edited

Loading

grtcdr commented May 10, 2021 •

edited

Loading

uttarayan21 commented May 14, 2021 •

edited

Loading

grtcdr commented May 16, 2021 •

edited

Loading

grtcdr commented May 17, 2021 •

edited

Loading