imgview: handle arguments as strings

[nblock]

Exit make_thumbs early when invoked with an empty first argument "" to avoid a syntax error.

[jarun]

@luukvbaal @N-R-K can you please review?

[0xACE]

What syntax error are you referring to?

Seems like the previous authors specifically call the function with with a empty string, your [ -z "$1" ] && return would short-circuit the intended code...

[nblock]

The syntax errors displayed on invocation of imgview with imv installed are:

line 34: [: : integer expression expected
line 49: [: : integer expression expected

@0xACE you are right, this short-ciruits the code. Another possible fix is to call make_thumbs with an integer argument not handled inside the function instead of an empty string, such as make_thumbs 9.

[N-R-K]

Haven't looked too carefully, but does this work?

Suggested change

	if [ "$1" -eq 3 ]; then
	if [ -n "$1" ] && [ "$1" -eq 3 ]; then

[nblock]

Sure, as long as all other occurrences in the function are guarded, too. How about evaluating the argument as a string and using = or != for comparison?

[luukvbaal]

I must confess I don't quite remember my thinking when I rewrote this plugin when merging the image plugins. IIRC some of the image programs require thumbnails while others already generate those themselves for certain file types. Looking at this now the arguments passed to make_thumbs() indeed don't seem to make much sense. We are passing 0, 1 3 and "", seems to me we should pass something like 0, 1, 2 and maybe -1 instead of ""? I'm not quite sure how I never ran into that syntax error, I remember thoroughly testing all the options back then.

[nblock]

@luukvbaal thx for your suggestion - -1 makes the intend clear. I'll update the PR shortly. I only came across this because I switched to another image viewer and purged a few others from my system.

[N-R-K]

I think spelling out the option makes more sense than using magic numbers. It's shell after all :)

[nblock]

I decided to just handle arguments as strings and keep the behavior as is.

[luukvbaal]

Any reason not to follow @N-R-K's suggestion? Should not change the behavior either right? Could you check that the commit I pushed works as expected?

[N-R-K]

Could you check that the commit I pushed works as expected?

Untested, but the change looks good to me.

[jarun]

@luukvbaal please approve and merge when ready.

[luukvbaal]

Tested it myself, seems OK. Thanks for bringing up the issue @nblock.