Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parameters for extract are ordered unintuitively #276

Closed
papandreou opened this issue Sep 28, 2015 · 5 comments
Closed

Parameters for extract are ordered unintuitively #276

papandreou opened this issue Sep 28, 2015 · 5 comments

Comments

@papandreou
Copy link
Contributor

The other day I spent a long time wondering why I couldn't get extract to do the right thing, only to find out that it wants top, left, width, height, whereas I intuitively put left, top, width, height. It works according to the documentation and all, but somehow I just assumed it would be X-Y-X-Y, and certainly not Y-X-X-Y.

I realize it'd be a hard thing to change, but I just wanted to check whether it's intentional? And if it's not, I move that extract is deprecated in favor of a differently named method that takes the parameters in the "correct" order ;). Mostly to save new users from the gotcha.

@lovell
Copy link
Owner

lovell commented Sep 28, 2015

Hej Andreas,

The underlying vips_extract_area method accepts (left, top, width, height) so you are definitely not alone in thinking that, intuitively, the parameters should be in that order.

Perhaps the extract method could be modified to (also) accept a single Object parameter with left, top, width, height as keys?

@brandonaaron originally implemented the much-requested extract feature in 83b72a1 so also may have relevant comments or suggestions.

@papandreou
Copy link
Contributor Author

Perhaps the extract method could be modified to (also) accept a single Object parameter with left, top, width, height as keys?

That could be a fair compromise, except that none of the methods with similar purposes take options objects (resize, rotate, crop etc.). That could be fixed, of course.

I'm mostly concerned with the long time effects of having the weirdly ordered extract around in any shape or form. As long as it's there and documented, I predict that users to waste time figuring it out. I still think deprecating it, undocumenting it, and introducing a new method (cut, shave, zoom, ...) would be the best long term solution. Or maybe crop can be overloaded to also support 4 numbers, if that's not in conflict with any future plans for that method.

@brandonaaron
Copy link
Contributor

It seems really obvious after reading this that it should definitely be in that order. At the time I didn't think much of it. I just followed the example api laid out in this comment without really thinking about it.

Another potential name for a replacement is isolate. However, I wonder if a new method would be more confusing than just making the method take an object. I think I'd vote for just documenting the oddity of argument order and have it take an object.

@lovell
Copy link
Owner

lovell commented Sep 29, 2015

Thank you both and sorry for not spotting this anomaly with extract sooner; API design is tricky.

In the current API, methods refer to both the what and the how. Switching to fewer methods (the what) that accept a related Object of parameters (the how) will more closely match libvips and should hopefully help expose more of its features. I realise this might make things inconsistent, at least in the short term.

Breaking API changes are also under discussion at #241 - please do get involved there too.

@papandreou
Copy link
Contributor Author

Added support for the options object over at #308 as that seems to be the pragmatic solution. Closing this issue.

papandreou added a commit to papandreou/express-processimage that referenced this issue Nov 18, 2015
teohhanhui added a commit to teohhanhui/image-resizer that referenced this issue Jun 14, 2016
teohhanhui added a commit to tripviss/image-resizer that referenced this issue Jun 14, 2016
teohhanhui added a commit to teohhanhui/sharp that referenced this issue Jun 14, 2016
@lovell: "it was entirely my fault!"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants