search

martin-stone / hsv-alpha-color-picker-android

A color picker and a color preference for use in Android applications.
Apache License 2.0
290 stars 60 forks source link

Make fields protected instead of private #25

Closed jamorham closed 7 years ago

jamorham commented 8 years ago

Hi Martin,

I couldn't extend ColorPreference properly without making these fields accessible by changing them from private to protected

Another user also mentioned this restriction in https://github.com/martin-stone/hsv-alpha-color-picker-android/issues/16

I also made defaultColor non-final so it is possible to change it programatically.

Would you consider integrating these changes in to your excellent library?

Thanks Jon

martin-stone commented 8 years ago

Glad you're finding the library useful and thanks for the pull request. However I haven't changed the opinion I expressed in #16 (for all the standard reasons). As users of the library can modify the source in any way they choose, it seems unnecessary to violate these principles for the sake of extensibility.

jamorham commented 8 years ago

It is extremely useful for users to be able to simply include the dependency for the library in the build.gradle file rather than maintain/publish a fork of the software.

I completely appreciate the programming principles that you are following. Would a better alternative to my PR be to instead include a public method which can write to the defaultColor private field?

By being able to write to this it allows for extending the class so that it can use the standard android defaultValue XML attribute instead of the ColorPicker_colorpicker_defaultColor style attribute and then utilise the preference values transparently even when they have not yet been set.

martin-stone commented 8 years ago

Yes, I think a public method would be the way to go, although if the standard defaultValue can be used then maybe that should be built-in anyway. I dimly recall having a problem with that, related to not being able to give a color value for it in the XML, hence the custom attribute. If your requirements are sufficiently general, and you can get it working in a general way, I'd be happy to incorporate it.

jamorham commented 8 years ago

Yes, handling defaultValue seamlessly is not trivial, the reason I believe is that only the raw attribute is accessible unlike the styled attribute where lookups for example @string/ are already done for you.

It needs something with obtainStyledAttributes but I haven't gone that far with my extended class because hex values suitable for parseColor are sufficient for my needs and I couldn't find sufficient documentation on how to achieve it given limited time.

The only modification needed in the library is the ability to write to defaultColor (by a method seems preferred) - Then the end user can use an alternate attribute mechanism as required.

Should I update my PR to use a method instead of protected variables? Would you be willing to accept that?

martin-stone commented 8 years ago

Yes, I'd accept get/setDefaultColor functions instead.

martin-stone commented 7 years ago

Proper handling of defaultValue was added in version 2.0, so I think the underlying motivation for this PR has been resolved. Closing.