budlabs / mondo

A theme manager and generator written in bash
MIT License
36 stars 2 forks source link

Fixed: force option #6

Closed ibokuri closed 5 years ago

ibokuri commented 5 years ago

The force -f option was not working with either the theme generation option -g nor the generator updating option -u. There were 2 reasons:

1) The condition ((__force!=1)) on lines 260 and 306 were missing $ in front of the __force. So the value of the variable __force was not being compared, meaning whether or not -f was specified, ERX or ERR would always be called and the theme files would not be updated.

2) The value of global variable __force was never being changed, despite an assignment in the case statement on line 46. After a long ass time wondering why, I found out that having the assignment __force=1 on the same line as the expression, for some reason, doesn't change the value of __force.

After fixing these issues, the force option works with both -g and -u!

budRich commented 5 years ago

Great. I hadn't noticed this issue myself, and made that refactoring (including the __ naming convention for global vars) a bit rushed.

@bl0nd , i am very grateful for this PR, i think it's awesome when people just try my stuff, even better when i get an issue, but a pull request! Amazing. I will test this quickly and if everything works (which i know it will), i will merge and create a new release today!

Since you have been digging in the code, is there anything you think should be changed, improved, added removed or such?

budRich commented 5 years ago

also strange errors.. ((force==1)) should be valid (maybe this underscore business is breaking stuff, interesting), and the case thing, i wonder if it can't be written force=1 && shift ;; ..

budRich commented 5 years ago

i took a look at the option parsing part and fixed the force issue, and made the whole thing a bit more easy to manage (can be nice if the list of options grows). #7

according to my tests: ((var==1)) is valid, and the (($var==1)) fixes is not needed. This is one of the benefits with arithmetic tests, even better when there are arrays, we can write ((array[key]==1)) or ((${array[key]}==1)).

ibokuri commented 5 years ago

Thanks a lot for fixing the issue! Bash isn't my primary language so I'm definitely gonna learn about those arithmetic tests, they seem really useful!

And I absolutely love all of your stuff man! I've learned a ton about i3 and bash from your videos. In fact, I was watching a video of yours a while back about better window switching or something and I saw a Todo program on the side and since you never talked about it I went ahead and wrote a copy-cat just cause it looked so cool.

Anyways, right now, the only thing I want to figure out is how to simplify the templates. Currently, every time I update a config file (say polybar's), I also have to update polybar's _mondo-template, otherwise when the former gets overwritten when I apply a theme, it'll be outdated by an edit. This updating gets a little tedious, especially when you got 4 or 5 other generators.

I thought it'd be a good idea to maybe have __mondo-template contain only lines from the original config that require substitution, so like for polybar:

~/.config/mondo/generators/polybar/_mondo-template

background = %%activebg%%
background-alt = %%inactivebg%%
foreground = %%activefg%%
foreground-alt = %%inactivefg%%
primary = %%blue%%
secondary = %%yellow%%
alert = %%red%%

That way, whenever we update the original polybar config, we don't need to update the polybar _mondo-template. And to apply the theme, we'd just need to update the original config with the generated theme files instead of overwriting the entire original config. The only problem is I don't know how to do that just yet. I think diff could help but the real problem is how to replace the appropriate lines in the original config file.

I saw that your mondo-contrib repo was structured like this so I have no idea if this is already a feature? If it is, it hasn't worked for me so far (though maybe I'm just being dumb and messing up) so thought I'd just bring it up.

budRich commented 5 years ago

the todo thing, is a Sublime package called "PlainNotes" there is also "PlainTasks" by the same author that is more focused on todos. I haven't talked about it in the videos, since it is sublime, but now with my new sublime playlist i might make a short video about it, its great stuff.

Let's continue the discussion regarding "template injection" in #8. i have thought a bit about something similar, and it would be a cool feature, and i don't think it's hard to implement, but it might have undesired side effects..