lyft / scissors

✂ Android image cropping library
https://eng.lyft.com/scissors-an-image-cropping-library-for-android-a56369154a19
Apache License 2.0
1.84k stars 235 forks source link

Crop View only configurable via XML #26

Closed rharter closed 2 years ago

rharter commented 8 years ago

The current CropViewConfig mechanism really cleans up attribute parsing, but also greatly limits the view since it can only be configured via xml. Want to programmatically create a view with non-default options? No, thank you.

I think we need standard getters/setters on the CropView object so that we can not only programmatically configure the view, but also alter the view at runtime (like allowing the user to choose from standard aspect ratios.

eveliotc commented 8 years ago

We could offer a config API to return a builder of CropViewConfig such as:

cropView.config()
  .withViewportHeightRatio(Ratio.16_9)
  .withViewportColor(Color.BLACK)
  .apply()

Apply would create a new CropViewConfig based off current configuration and set the view and manager to new configuration

rharter commented 8 years ago

What's the benefit of doing it this non-standard way? It seems like it's more trouble than it's worth, to me.

As an example of the kind of problem this introduces, it means that we can't use standard animations on the view. For instance, in my current production use case, I have buttons that allow the user to choose from 5 standard aspect ratios, 1:1, 5:4, native, 3:2 and 16:9. When the user taps a button, it animates the viewport to the desired ratio. Adding this config api makes that quite difficult to do.

I think the best approach would be to use standard properties on the view and get rid of the config object all together.

eveliotc commented 8 years ago

Benefit would be having a immutable configuration and a similar API to extensions, it could be easily implemented at this point as it is how configuration from XML works, animations could be run on config changes, but I do agree that's not a common approach vs. simply having setters

rharter commented 8 years ago

What do you mean by "animations could be run on config changes"? I'm not sure how the fits the provided use case. The problem is that the configuration is, by nature, not immutable. I see no reason the developer can't change things like the viewport ratio at runtime, for instance, when the user selects a different aspect ratio from a button in the UI.

rharter commented 8 years ago

Here's an example of the type of UX I'm talking about that an immutable config object doesn't support.

ezgif com-resize

dschaller commented 2 years ago

Thank you for you contribution to this repository.

Closing this contribution as this repository is being archived.