[API design] copy_from returns bool

[lovasoa]

Hello, and thank you for this very useful crate!
I stumbled upon GenericImage::copy_from, that returns false when the operation fails. This is not very idiomatic in rust, and makes error handling more difficult. It would be nice if the method could return a Result<()>. This would prevent crate users from inadvertently ignoring an error.

[HeroicKatora]

I'd like to resole this one way or another for the next breaking release–0.22–but there are two things to consider:

• The x, y position is maybe better expressed by copying into a sub_image and just removing the other arguments.
• Instead of returning a bool, the method could just panic, like [T]::copy_from_slice. If not, then I agree that Result<()> is the best alternative.

[lovasoa]

I don't think introducing a panic would be a good idea. It would just make the library less safe to use...

[HeroicKatora]

Less safe? Not quite sure what you mean. If the copy operation is necessary to avoid leaking data of other clients through a buffer reuse then the panic made it safer. Modelling the function after the standard library would also ensure that expectations by developers are met, and the copy_from_slice method indeed does panic on size mismatch. And not to mention that the bug found in #908 would not have appeared, as stricter preconditions are typically easier to check–and potentially allow more performant implementations.

Note: I'm not saying to change the method into something that panics, without offering any alternative.

The trait should, imo, be a (minimal) set of accessor functions on which other abstractions can be built. copy_from exists because we had hoped it could be implemented with a performance gain over the default provided implementation. Note that the method is eerily similar to imageops::overlay. Where are the differences? Would one be able to guess them? I think the answer is:

• For completely in-bounds coordinates, overlay and copy_from_slice are the same.
• If the overlayed image is bigger than the underlying one, then overlay is strictly more useful.
• overlay gives no indication of success, because its operation is well defined for all cases. Whereas copy_from_image seems to fit no single description.
• overlay should make use of copy_from in some way, to also forward the potential performance characteristics.

The current copy_from can also be implemented, without any panics, through the use of sub_image and the panicking copy_from. Wouldn't that triple of (copy_from_panicking, copy_from_old, overlay) be strictly superior to the current situation? If so, then I think only the first should be a method of the trait while the other two operations can either be free function as overlay currently or alternatively on an Ext trait.

I also find an Error misleading. First of all, the image::Error is far to broad if the only possible error variant is ::DimensionError (e.g. the Io variant could be extremely confusing to be potentially handled by the caller). Rather, a dedicated error with impl Into<image::Error> would be more appropriate. But even then, an error is more appropriate if the method checks some distinct inner condition, inherent to that algorithm. Image decoding does this, some externally defined operation do it, but a copy? And it fails if you provide it with too much data? Just seems confusing atm, and I'd like to resolve this.

[fintelia]

All of these functions are extremely simple, they basically just loop over each pixel in some rectangle and copy over the value at that location to another another image. This basically amounts to 3 lines of code. Convenience functions are nice, but there is no need to handle every possible case since users can easily just implement there own versions if necessary and if a user wants a non-panicking version of one of the current functions, they don't even need to do that: it is enough to just validate the preconditions before making the call.

[HeroicKatora]

I thought I'd create a PR for direct comparison (#905, #963). The alternative would panic! instead of allowing the operation on any dimension mismatch and encourage the use of sub_image. This gives the image implementation less options to customize the ininitialization since any other position is initialized by the code of SubImage. But at the same time I believe that the style of overlay and replace is the right one anyways: Validate the external preconditions by construction instead of relying on each GenericImage to do the same.

An third alternative: combine both. Keep the x, y arguments for potential (still unproven but I'm hopeful) future initialization speed improvements but panic on dimension mismatch.

[lovasoa]

This was closed by error because of a wrong manipulation I made. Can someone reopen this issue ?

[lovasoa]

Since no consensus seems to be reached here, maybe we should at least add a #[must_use] annotation to the function ?
Who is supposed to take the final decision ?

[fintelia]

Sorry this has been dragging on so long! Could you fix the CI issue on #1004 and then I'll go ahead and merge it?