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

Support Library PreferenceFragmentCompat version #35

Closed kustra closed 5 years ago

kustra commented 7 years ago

I've created a colorpicker_compat module for using the library in the PreferenceFragmentCompat of the Support Library. It depends on the original colorpicker module and adds the ColorPreferenceCompat class.

Also modified the demo_app for showcasing its usage, and the README.md for documenting it. Please note that the colorpicker-compat artifact that the README.md refers to does not exist yet. I integrated it into my project using Jitpack.

An independent, small cleanup was included in the Support Library version: I've found a way to hide the keyboard when showing the dialog - without adding android:windowSoftInputMode="stateHidden" to AndroidManifest.xml for the Activity, as using that may not be possible for single-Activity applications.

martin-stone commented 7 years ago

Hi. You've obviously put a lot of effort into this, but there's more code duplication than I'm prepared to accept into master. Having to maintain two distributions is pretty unpleasant too.

What features are missing from the standard android.preference.* classes that make this necessary?

kustra commented 7 years ago

I don't mind working on it a bit more to reduce code duplication. Since the base classes are provided by the Android API and cannot be modified, the (common) code will have to go into a POJO and called from there.

It's not so much the missing features that make me prefer the support library preferences but the fact that I'm developing a single activity application that uses support library fragments and fragment manager everywhere. I'm unwilling to mix a regular (preferences) fragment into this architecture. I also don't think that such an app architecture would be uncommon, so others should have the same issue. Surprisingly, hardly any color picker libraries support the Support Lib Preferences API, which is why I opted for modifying this one.

I can also create and distribute a separate lib with the support library classes that depends on yours, but I'd prefer a solution with less complexity for developers trying to use this lib.

What do you think?

martin-stone commented 7 years ago

I'd need it to be tidier in order to be convinced -- As the maintainer, I try to keep things as simple as possible, so I can still understand it all when I return to it after a long absence. :-)

Why is the ColorPreferenceHelper necessary? This seems ugly for both library and user code.

kustra commented 7 years ago

The Support Library API changed, compared to the built-in one. One of the notable changes around DialogPreference is that dialog fragment's code needs to be in a separate class (ColorPreferenceDialogFragmentCompat for us) and the DialogPreference subclass (ColorPreferenceCompat for us) is only responsible for storing the preference value and the preference fragment's list item display.

These two classes need to know of each other, and the connecting code has been moved from the DialogPreference subclass to the PreferenceFragmentCompat. The developer needs to execute some code in their preferences fragment implementation for the color picker dialog to work, otherwise they'll get a runtime exception when trying to edit the preference. I could think of two possible solutions for this: our own PreferencesFragmentCompat subclass that we force the developer to use as their preferences fragment base class, or a helper class, much like the google or Facebook api clients. I chose the latter.