search

adi1090x / battery-wallpaper

Simple bash script to set wallpaper according to battery percentage with charging animations.
GNU General Public License v3.0
391 stars 22 forks source link

Refactored Code #6

Closed shubhamoli closed 4 years ago

shubhamoli commented 4 years ago

@adi1090x Please review PR

and also please test in before merging as I haven't tested it on linux

shubhamoli commented 4 years ago

@adi1090x

Also please help me in understanding - why some effect require division by 25 whereas some require division by 20?

Can't we fix this to a single number -- either 25 or 20.

Doing this will ease writing generic function for all styles.

adi1090x commented 4 years ago

Hi, you're code looks good but it does not work. can't merge. also, as you asked why some options needs to be divided by 25 and not by 20 is because some options have 4 battery wallpapers and some have 5. So, the option which has 4, needs to be divided by 25 and not by 20 to work correctly.

rhysperry111 commented 4 years ago

Would it not be a easier to have some themes take slightly longer/shorter than others? EDIT: I misunderstood what the number is for

shubhamoli commented 4 years ago

Hi, you're code looks good but it does not work. can't merge. also, as you asked why some options needs to be divided by 25 and not by 20 is because some options have 4 battery wallpapers and some have 5. So, the option which has 4, needs to be divided by 25 and not by 20 to work correctly.

Yeah! true, I had already read the code........That's why I asked - can't we consent upon setting a fix number like either 4 or 5 iterations. Without it, we have to introduce another variable unnecessarily for writing a single function for all styles.

And, if we write a single function to set wallpaper for all styles, then it'll become easier to add/remove more styles in future.

Although, nothing is wrong in your current code but how many if...else if conditions are we going to add more? to support more styles in future

Regarding code not working.......I'm helpless as I've mac and it is working fine on it but I can't test it on a real linux machine (VMs don't have battery configurations)

adi1090x commented 4 years ago

i'll start working on themes soon, i'll make every theme have a fixed number (5 maybe) of wallpapers. i just need to think which wallpaper should be shown for a range of battery percentage in themes which has 4 wallpapers. maybe you guys can do it too.

adi1090x commented 4 years ago

Also, i'm now using acpi to get battery info in linux & i've pushed that update.

rhysperry111 commented 4 years ago

You could implement it in such a way that instead of a fixed number, the theme passes the amount of iterations to the function which would calculate the percentage ranges automatically. This would allow for more advanced (10-20 steps) themes in the future