13
\$\begingroup\$

I wrote the following little shell script and published it on GitHub. In the repo there's also a README file that explains how to use it in more detail.

This is the first time I open-sourced/published something I wrote, so I'm interested whether you think that this code is ready for the public.

Some pointers, but feel free to add your own thoughts:

  1. Does it look professional? If not, what would need to be done?
  2. Are the comments too much? Was it ok to publish it, although I know that under some circumstances it doesn't work (I wrote about that in the README)?

#    random xkcd wallpaper: gets a random xkcd comic from xkcd.com and sets it as the desktop background.


#!/bin/bash



# /random/comic redirects to random comic. Wget gets the index.html of the comic.
wget http://dynamic.xkcd.com/random/comic/

echo $(pwd)

#Searches the line in the index.html file that points to the url where the actual comic is placed.
#The image urls are of the form: http://imgs.xkcd.com/comics/.'name of comic'.(png | jpg)
url=$(cat index.html | grep -o -m 1 http://imgs.xkcd.com/comics/.*\.png)

#Assuming picture format is .png. Gets the name of the image file by only matching what comes after the last forward slash.
name_pic=$(echo $url | grep -o [^/]*\.png)
is_png=1


#Sets url and name_pic in the case of the picture being in .jpg format.
if [ -z "$url" ]
then
    url=$(cat index.html | grep -o -m 1 http://imgs.xkcd.com/comics/.*\.jpg)
    name_pic=$(echo $url | grep -o [^/]*\.jpg)
    is_png=0   
fi     


#Downloads the image and saves it under its appropriate name.
wget --output-document="$name_pic"  "$url"

#Sets the desktop background
gconftool-2 --set --type=string /desktop/gnome/background/picture_filename $(pwd)/"$name_pic"

#Cleans up
rm index.html

#For some reason, if the image is moved to fast (e.g without a wait) the background does not get set.
sleep 1

#The current wallpaper can always be found under "current_xkcd_wallpaper.png" or "current_xkcd_wallpaper.jpg". Also prevents cluttering the directory with images. 
#If you want to keep all the pictures you downloaded, uncomment the following lines.

#Cleans up. Makes sure that there is only one current_xkcd_wallpaper image file.
#rm current_xkcd_wallpaper.*

rm current_xkcd_wallpaper.*

if [ $is_png = 1 ] ; then
    mv $(pwd)/"$name_pic" $(pwd)/current_xkcd_wallpaper.png
else
    mv $(pwd)/"$name_pic" $(pwd)/current_xkcd_wallpaper.jpg
fi
\$\endgroup\$
12
\$\begingroup\$

Sorry, this might sound harsh, but I really think GPLv3 is plain overkill for your code. And this even might sound harsher: What you want to archive can be done in a 3-liner, so I think Public Domain would fit far better.

I do not want to show you up as everybody starts as beginner, and basically it's your idea which counts, the implementation itself is mostly an exercise for the reader. So continue learning shell, it's tremendously powerful!

An elaborated example which basically does what your script does, it borrows some details from your script:

#!/bin/bash

cd "`dirname "$0"`" || exit

[ 0 = "$#" ] &&
set -- gconftool-2 --set --type=string /desktop/gnome/background/picture_filename

url="`curl -sL http://dynamic.xkcd.com/random/comic/ | grep -om1 'http://imgs.xkcd.com/comics/[^.]*\.[a-z]*'`"
img="$PWD/xkcd-wallpaper.${url##*.}"

curl -so "$img" --fail "$url" &&
"$@" "$img"

I wrapped the lines just for better readability. Note that every programmer has a set of favorite tools and ways, and one always can learn something from each other. Like I learned "grep -o" from your script today (I usually use sed, but grep -o is far more elegant in this context).

The nice thing on Unix is that you can chain things so easily and flexibly. There is no need to squeeze everything into one monolithic script. The better way would be to create two scripts as building blocks and then join them together.

One pulls the picture. Second installs the picture on gnome. Like this:

xkcd-pull.sh:

#!/bin/bash

url="`curl -sL http://dynamic.xkcd.com/random/comic/ | grep -om1 'http://imgs.xkcd.com/comics/[^.]*\.[a-z]*'`"

img="/tmp/xkcd-wallpaper.${url##*.}"
img="${1:-$img}"

curl -so "$img" --fail "$url" && echo "$img"

set-gnome-wp.sh

#!/bin/bash

exec gconftool-2 --set --type=string /desktop/gnome/background/picture_filename "$@"

And then join that together, for example on the commandline:

img="`./xkcd-pull.sh`" && ./set-gnome-wp.sh "$img"

Following this pattern you can easily extend it for KDE or perhaps even adapt it to Cygwin under Windows. Please note that these snippets even contain some extra bloat, which might come handy later on.

As you can see easily, doing it this way there is nothing left which you cannot look up in a manual. So all creativity left is your idea "XKCD -> Wallpaper" while the implementation is "trivial".

(I did not test the code snippets here. Maybe there still is a typo in it. My contributed code is Public Domain.)

\$\endgroup\$
  • \$\begingroup\$ Thanks for the input. As already mentioned, this is the first thing I open sourced, so I wanted to put a license on it :) But I see your point. I also appreciate the bash specific comments, was my first script, lots more to learn! \$\endgroup\$ – Basil Jan 30 '11 at 22:42
  • \$\begingroup\$ The kind of license you choose is orthogonal to the length of the code and the matureness of the developer. Having said that, I like to nitpick on the backticks, which are deprecated. $(...) is better because it can easily be nested, and is better readable, even in poor fonts. \$\endgroup\$ – user unknown Apr 11 '11 at 5:47
  • \$\begingroup\$ You are right! But typing 2 backticks involves 2 keypresses of just a lonely single key, while you usually have 5 keypresses of 4 different keys to enter $( and ). And moreover, we program because we are lazy, right? So using $(..) everywhere means wasting entropy. Which is bad as we know: Entropy never can drop. (SCNR as I am German <- triple irony) \$\endgroup\$ – Tino Feb 10 '14 at 8:44
14
\$\begingroup\$
#!/bin/bash

The shebang line has no effect unless it's on the very first line, so you should put it before the copyright notice.

echo $(pwd)

echo $(command) is almost always the same as just writing command (the exception being that echo adds a newline at the end if the command doesn't already produce one, but that's not the case here). So you can just change the above to pwd.

cat index.html | grep -o -m 1 http://imgs.xkcd.com/comics/.*\.png

That's useless use of cat. If you want to grep through a file, you can just pass the filename as the last argument - no need to cat and pipe. (The same goes for later places in the code where you do the same with .jpg).

gconftool-2 --set --type=string /desktop/gnome/background/picture_filename $(pwd)/"$name_pic"

I think it'd be a good idea to make the command to change the wallpaper configurable, so non-GNOME users can use your script too.

Also I'm not sure why you set the image to be the wallpaper before renaming it. I think it'd make more sense to do so afterwards (that might also solve your issue of having to sleep).


On a general note you might take care to gracefully handle already existing files. For example if the script is called in a directory where an index.html already exists (which is not that unlikely), your script will a) not work and b) delete that index.html, which its owner might not appreciate.

\$\endgroup\$
  • \$\begingroup\$ Those are all valid points which I will incorporate. I especially appreciate the remark about deleting files. Thats really important :-) I set the image before renaming it, because gconftool does not change the background if the name of the image stays the same, even though the file changed. Seems to be a bug. \$\endgroup\$ – Basil Jan 29 '11 at 22:45
  • 1
    \$\begingroup\$ @Basil: This mailing list post suggests unsetting and then resetting the key. That seems cleaner than using the old filename and then sleeping. \$\endgroup\$ – sepp2k Jan 29 '11 at 22:59
  • \$\begingroup\$ I tried this when I wrote the first version of this script. I didn't work for some reason, thats why I download it to a different file name each time. It just loads the same image again otherwise. But your right, this would be nicer if it worked. \$\endgroup\$ – Basil Jan 30 '11 at 23:02
7
\$\begingroup\$

As well as the other problems already outlined, your code gets a file 'index.html', which could clobber any file of the same name in the current directory. Maybe it would be a good idea to create a new directory so that you don't mess things up?

It would also be a good idea to make sure that the debris is removed even if the script is interrupted. You do that with 'trap'. I recommend trapping HUP, INT, QUIT, PIPE and TERM signals:

trap "rm -f index.html; exit 1" 0 1 2 3 13 15

...other actions...

rm -f index.html
trap 0

The trap 0 at the end ensures that your script can exit successfully - very important. You can make your trap actions as complex as necessary - and should aim to keep them as simple as possible. One advantage of creating a directory for intermediate files is that you can simply remove that directory to clean up.

\$\endgroup\$
  • 1
    \$\begingroup\$ +1 for creating a new folder. It is good practice to create a hidden folder named after your app in the user's home folder for this purpose, e.g. ~/.yourappname \$\endgroup\$ – Eric Bréchemier Jan 30 '11 at 14:22
  • 1
    \$\begingroup\$ Even better: avoid writing index.html to disk at all. Just pipe it into your code for processing. \$\endgroup\$ – 200_success Feb 2 '14 at 3:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.