search

nbeaver / timetunnel-logos

Downloads the logos for the timetunnel screensaver that aren't installed by default.
MIT License
1 stars 0 forks source link

Simplification (for end users and of the code) #1

Closed hackerb9 closed 6 years ago

hackerb9 commented 6 years ago

I like this project, but there seems to be a lot of extra stuff going on that may make this harder to use than necessary. This could be trimmed down to a single, short file and then people could run just that.

For example, a lot of things are treated as variables when they don't need to be (e.g., CONFIG_FILE, INSTALL). Another thing, the scripts go to extra work to fail if a command line argument isn't provided. But then the error message prints out what the sane default would have been.

Ideally, I think this would be a two line script: 1 wget and 2 patch.

① Wget can already skip downloading of files that already exist (--no-clobber), so you could run wget like this and replace the entire download-logos.sh file:

URL=http://www.zettix.com/Graphics/timetunnel/xscreensaver-4.22/hacks/images/
wget -nc -r -np -A xpm -nd -P ~/.local/share/icons/xscreensaver/timetunnel $URL

② You already have patch working in patch-xscreensaver-config.sh. You'd just need to change CONFIG_FILE to have a sane default when not specified and then add in the wget command from #1 and you'd have a single script that does everything.

nbeaver commented 6 years ago

This could be trimmed down to a single, short file and then people could run just that.

The installation instructions were not so good. They should be better now. The single command to run is make install.

Does that help?

hackerb9 commented 6 years ago

Thanks. I do like Makefiles, but isn't it a bit overkill?

My suggestion was to maybe, if you feel like it, simplify and shorten the code. It seems perhaps unnecessary to have users download several files to get this to work. The Makefile, the download script, and the patch script could all be one file.

Again, this is just a minor stylistic suggestion. I think the one line wget example I gave is easier to understand because it's shorter, but perhaps you feel your download script is better because it's more verbose. It's your code, so do as you feel best.

nbeaver commented 6 years ago

I do like the simpler invocation of wget using the directory instead of the archive, so I've switched download-logos.sh to that and added a standalone installer script here:

https://github.com/nbeaver/timetunnel-logos/blob/master/install.sh

I still need a Makefile for things like generating local documentation (so I can check the markup), testing when things break, generating patches, running Shellcheck, uninstalling, etc.

Anything else for this issue?

hackerb9 commented 6 years ago

Nope. Looks good!