Don't return error if action succeeded

[heyimalex]

I use IM primarily to convert tiffs. Libtiff (which imagemagick uses under the hood) will call its warning handler whenever it encounters a tag it doesn't recognize. Because tiff tags are just a free-for-all key-value store where every piece of software that's ever touched the file has dumped some info, that means it calls its warning handler pretty much every time.

That isn't a problem because it just skips those tags, finishes reading the file, everything is a-ok. But right now that means when using this lib I get an error during ReadFile...

So two issues.

First, we need a way for end users to figure out what the ExceptionType of an error is. We can make the user do type assertion from error to MagickWandException to get it, but it needs to be accessible. That's as easy as changing MagickWandException#kind to be public and documenting.

Second, anything that wraps a c api method that returns MagickBooleanType shouldn't return an error unless the api call returns false.

[justinfx]

Yes, it should be easy enough just to make the kind a public member. The fixing up of all the functions that deal with MagickBooleanType return values would need some auditing. Did you want to work on this and submit a merge request? Or did you want to wait for someone to get around to it?

[heyimalex]

I'll work on it

[rogierlommers]

Any update on this? I'm trying to process TIF files as well and I'm getting panics :(.

[heyimalex]

It looked like a lot of work and I was ultimately using imagemagick just for creating thumbnails, so I ended up just using cgo directly and then totally forgot about this issue. If you're not opposed to writing c and you're needing this now, that's a pretty straightforward option.

Did say I'd work on it though... so I'll look into it again when I get to work! Think it's a pretty simple change, it just needs to be done in a ton of places.

[rogierlommers]

I need it :). Can you please pass me the changes you've already made?

[heyimalex]

I don't have any changes to give, but here's all that needs to be done:

Create a function like this:

func (mw *MagickWand) getLastErrorIfFailed(ok C.MagickBooleanType) error {
    if C.int(ok) == 0 {
        return mw.GetLastError()
    } else {
        return nil
    }
}

And then, the tedious part, change every function that currently returns mw.GetLastError() in some form or fashion to 1. store the return value of whatever MagickWand api call was made, and then 2. pass that return value to getLastErrorIfFailed and use that in place of GetLastError.

So, for example, the first function in magick_wand_image.go would change this:

func (mw *MagickWand) AdaptiveBlurImage(radius, sigma float64) error {
    C.MagickAdaptiveBlurImage(mw.mw, C.double(radius), C.double(sigma))
    return mw.GetLastError()
}

to this

func (mw *MagickWand) AdaptiveBlurImage(radius, sigma float64) error {
    ok := C.MagickAdaptiveBlurImage(mw.mw, C.double(radius), C.double(sigma))
    return mw.getLastErrorIfFailed(ok)
}

Most of them are that simple. According to grep -r 'mw.GetLastError()' . | wc -l there are about 256 spots that need to be changed. Maybe I will do it tonight but... tbh it doesn't sound like a lot of fun :)

[heyimalex]

Actually, with some regexes it wasn't bad. I've got a branch here

[justinfx]

That looks pretty straightforward to me. Want to put up a merge request, after @rogierlommers confirms that it fixes his bug?

[heyimalex]

For sure. Test looks like it's working, but like I said most of the work was done with regexes.

A couple of functions in magick_wand_image.go didn't take well to the change:

MagickEvaluateImages returns a MagickWand instance, but EvaluateImages only returns an error. Not sure what I should do here? Change the signature to return the wand, getting the error if it's null? Is this a memory leak right now?

I can't even find the documentation for MagickFxImage, but it looks kind of similar. It also raises another issue; a bunch of functions return *MagickWand and no error value. Should we be checking whether the pointer returned from the underlying API call is null and then returning GetLastError if it is? I don't know enough about the MagickWand api to say.

[justinfx]

It actually might be a leak. The ImageMagick docs aren't super clear all the time about ownership. But I see an allocation happening for the cloned wand that gets returned: http://www.imagemagick.org/api/MagickWand/magick-image_8c_source.html#l00087

Looking at the bindings in iMagick, I am not sure how this has worked properly in all cases. It would seem that the modifications being done the wand are done in the cloned wand?

[justinfx]

If it was leaking already before you did any work, we can address this in a separate ticket. But if its very coupled with your fixes, then I guess it has to be addressed. I'm not sure how anyone managed to use these particular functions if the C-API returned a new wand, yet the Go bindings never returned that wand to the caller. I haven't tested these functions, but it would seem the desired effects would be in the returned wand. So on top of leaking, it doesn't seem to be useful/complete in its current state.

Maybe you can do the most minimal change, by not changing the signatures, and just doing the proper check and returning the error as needed.

[heyimalex]

Yeah, I figure it's just not used very often because the non-generated documentation is wrong as well and, according to blame in the im gitlab, has been for the past six years haha.

I'm about to head out but I'll clean up my branch a little and submit a PR tonight or tomorrow.

[justinfx]

Fixed by #66

[justinfx]

Thanks for the work!

[rogierlommers]

Thank you both for your work!