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

Color changed listener #21

Closed MFlisar closed 8 years ago

MFlisar commented 8 years ago

I think, this would be nice... Sadly, the observable color is private and has no getter nor does the ColorPickerView have a function to forward an custom color observable...

martin-stone commented 8 years ago

I'd happily consider a pull request.

MFlisar commented 8 years ago

Done

martin-stone commented 8 years ago

Thanks for the quick work. I'll have a proper look later. One issue: the removeColorObserver function doesn't do what it says. In practice maybe we could live without that function anyway.

Also, what's the rationale for putting IColorObserver into it's own package? It creates a need for explicit imports everywhere for no real benefit that I can see.

MFlisar commented 8 years ago

I'll remove the removeColorObserver, I personally don't need it. Just wanted it to be there for the case...

And I'll make the listener an internal class again...

I personally prefer code like:

.addListener(new IColorChangedListener(){...})

Over colde like:

.addListener(new BaseClass.IColorChangedListener(){...})

It's cleaner IMHO... I personally use internal listeners only if they are private or protected...

martin-stone commented 8 years ago

I meant the com.rarepebble.colorpicker.interfaces package (as opposed to just putting it in com.rarepebble.colorpicker). Putting it in a separate module seems reasonable.

MFlisar commented 8 years ago

I see... Misunderstood you.

It's just because I'm used to it... To group and organize my classes...

In a little library like this it's probably not necessary...

MFlisar commented 8 years ago

I corrected the code...

martin-stone commented 8 years ago

All merged now, and built and published to jcenter as version 1.7.0.

Thanks!