gregkorossy / Android-Support-Preference-V7-Fix

Android androidx.preference support library has some issues, this lib tries to fix them.
https://discord.gg/87NVsSK
Apache License 2.0
497 stars 49 forks source link

Request for additional preference types #87

Open gregkorossy opened 6 years ago

gregkorossy commented 6 years ago

New preference types are added occasionally to a separate artifact:

compile 'com.takisoft.fix:preference-v7-extras:26.1.0.0'

Current and planned extra preference types:

If you have any requests about new preference types, add them here

zhanghai commented 6 years ago

SeekBarPreference: You can base it upon my impl.

gregkorossy commented 6 years ago

@DreaminginCodeZH SeekBarPreference is already available both in the official lib and the fixed lib as well.

zhanghai commented 6 years ago

Maybe a SimpleMenuPreference here? It correctly implemented the Material Design spec, but is based on the framework Preference and compilation hacks.

gregkorossy commented 6 years ago

Isn't it the DropDownPreference? Because it looks like that.

zhanghai commented 6 years ago

@Gericop

No:

Simple Menu - Material Design Guidelines

It's runtime effect is just as in the spec. I've seen the author using it in one of his apps.

gregkorossy commented 6 years ago

Well, the DatePickerPreference and TimePickerPreference are almost done, I had to port the entire framework version of the picker dialogs to show material themed pickers (I know about the separate datetimepicker project of AOSP, but that uses the first (=old) version of the material pickers, and had to solve some problems related to date and time formatting on older APIs (mainly 17 and under). It's only in a Github repo for the time being, but soon it's gonna be released to jCenter, too.

gregkorossy commented 6 years ago

The 3 new preference types are now available on jCenter.

RingtonePreference:

compile 'com.takisoft.fix:preference-v7-ringtone:26.0.2.0'

DatePickerPreference and TimePickerPreference:

compile 'com.takisoft.fix:preference-v7-datetimepicker:26.0.2.0'
zhanghai commented 6 years ago

@Gericop Is the SimpleMenuPreference going to land in this library? If not, I'm also considering creating a separate library for it.

gregkorossy commented 6 years ago

@DreaminginCodeZH I'll look into it. I'm finishing the ColorPickerPreference now, then check out what's needed for the simple menus.

gregkorossy commented 6 years ago

The funny thing is that DropDownPreference has everything basically to implement this (so Google could have / should have done it), but the most important part (the item measuring) is package private, so it means that either a hack is needed (placing the custom preference to a package that has the same name as the official one), or the whole class must be duplicated to override that functionality. Seriously, sometimes I just don't understand why would they choose to make some methods like this (package) private...

opus1269 commented 6 years ago

Would it be possible to support any custom preference that extends from DialogPreference?

I have a need for a NumberPicker preference. I could do it with a list, but the NumberPicker is cleaner IMO.

gregkorossy commented 6 years ago

@opus1269 Yeah, it is currently undocumented, but you can add custom DialogPreference implementations by extending DialogPreference for the preference and PreferenceDialogFragmentCompat for the dialog, and adding the class references using this:

public class YourCustomPreference extends DialogPreference {
    static {
        PreferenceFragmentCompat.addDialogPreference(YourCustomPreference.class, YourCustomPreferenceDialogFragmentCompat.class);
    }
    // ...
}

The lib's PreferenceFragmentCompat implementation will handle and display the given dialog fragment if the preference is touched by the user.

NumberPickerPreference is certainly not a bad idea, but you need a material picker because the internal widget does not comply with the MD guidelines on pre-Lollipop devices. The DateTimePicker lib contains an implementation (the official one from the Android 8.0 source) since the collapsed view shows a spinner (a NumberPicker) instead of a calendar view, however, it is currently not available separately.

zhanghai commented 6 years ago

Can you make the addDialogPreference method accept any Fragment? It would make things easier for displaying an AppCompatDialogFragment without the need for a key on the preference (or else the support lib tries to findPreference which will throw exception in onCreate of PreferenceDialogFragmentCompat).

gregkorossy commented 6 years ago

Is there any particular reason why you don't want to extend PreferenceDialogFragmentCompat instead of AppCompatDialogFragment? PreferenceDialogFragmentCompat is meant to be used for DialogPreference's since it provides some helpful things such as getPreference() which can be used to query and update the preference itself.

zhanghai commented 6 years ago

without the need for a key on the preference (or else the support lib tries to findPreference which will throw exception in onCreate of PreferenceDialogFragmentCompat).

A use case of this is wrapping the LicensesDialog library, where the dialog is informative and dialog content is independent of parent preference attributes.

gregkorossy commented 6 years ago

I know that but why wouldn't you want a key for a preference? What's the use case?

zhanghai commented 6 years ago

Please see edit on my last comment.

gregkorossy commented 6 years ago

I see. Well, it can be done for sure. I'll probably release a new version later today with this, the colliding attribute names fixed, and some other fixes.

gregkorossy commented 6 years ago

Updating now the README.md so it can be released.

gregkorossy commented 6 years ago

Released v26.1.0.3.

zhanghai commented 6 years ago

Hi, I just checked out 26.1.0.3 and found that the second argument is still requiring a DialogFragment, but in fact one can choose to use a fragment to receive onActivityResult(). This technique can be used in for example a RingtonePreference that launches the stock ringtone chooser activity. So can you just relax the constraint to be a Fragment to make it more flexible?

gregkorossy commented 6 years ago

That wouldn't make sense semantically since a DialogPreference should display a dialog (I know that the ringtone picker Activity looks like a dialog, but it's an external component to the app). Also, there's no way of knowing when the given Fragment should be removed from the FragmentManager meaning the developer should handle it which can lead to bugs, so for both of these reasons I will not relax the constraint. However, you can do a little hack and use the DialogFragment just like if it was a normal Fragment:

  1. Create the fragment where you override onCreateDialog(...) so that it returns null (this is allowed by DialogFragment)
  2. Launch whatever Activity you want from your fragment
  3. In onActivityResult(...) after processing the results, call dismiss().

This will add the DialogFragment to the FragmentManager listening to whatever result you expect, then the dismiss() call removes it from the FragmentManager.


P.S.: There's a RingtonePreference so do not reinvent the wheel if you just want to pick a ringtone.

zhanghai commented 6 years ago

I think API design should not prevent developer from accomplish certain task even if it introduces likelyhood of error - error-prone developers won't try to understand why the limitation is there and will work around it in some other ugly way, while people with true need of an API will be forced to hack. Intead, this should be the work of documentation.

As for semantics - My RingtonePreference isn't a DialogPreference, and the support library did not require a DialogPreference in onDisplayPreferenceDialog(). Simply any Preference that want to display a "dialog" can do that, and there's no need to prevent people from doing what they see fit in their own situation.

Or at least, please consider adding a @Deprecated overload that takes a Fragment and explain the reason there - it's painful to add another wrapper over PreferenceFragmentCompat and duplicate the logic in every app just because of a type constraint that could have been relaxed. (The show-and-dismiss dialog hack is too ugly and semantically-incorrect that I don't wish to consider.)


P.S: I think the stock ringtone picker is more reliable and may present user additions or modifications from the ROM, so I wrote the implementation based on Activity.

gregkorossy commented 6 years ago

You can do what I suggested in the 3-steps thingy. It's essentially the same what you want but prevents people from accidentally adding any kind of Fragment without understanding the problem it introduces. Still, this whole "use a proxy Fragment to open an Activity to get some results back to the preference" seems odd to me. I'd rather look for a better solution.

I chose not to use the ringtone picker Activity because on pre-Lollipop it doesn't have the material theme, on Lollipop and up it has the material theme but the color scheme doesn't match the app's one, so the UX is bad, and also it doesn't have things like "add ringtone" on older APIs.

zhanghai commented 6 years ago

onCreateDialog() is annotated with @NonNull in support-v4 DialogFragment, although current behavior guarded against null, I don't think it's guaranteed and prefer not doing so. So the 3-steps thing is not a correct workaround for this, not to mention it is far too much incorrect both semantically and in implementation.

Using a headless Fragment is not an odd thing, but a practice that has been around for a while. In contrast onCreateView() is marked with @Nullable, and it is a common technique to use a Fragment without UI to retain state of network call during configuration changes.

And just to stay consistent with support library, it didn't put restriction on the developer to extend DialogPreference to show any form of a dialog (see PreferenceFragmentCompat#onDisplayPreferenceDialog()). You cannot prevent developers from overriding their own onDisplayPreferenceDialog(), so they can always do insane things. And this is also a hidden API which dumb developers won't find. Why don't just be a bit more open about API and believe developers? After all, this is only a (super-)convenient wrapper around onDisplayPreferenceDialog(Preference preference).

So can you be so kind to at least add a deprecated overload, so that a stupid artifact just adding one method call won't ever be needed on maven? It can be like this:

    /**
     * @deprecated Most users should add a {@link DialogPreference} with a
     * {@link PreferenceDialogFragmentCompat}. If you are using this overload, make sure you remove
     * your fragment once work is done.
     */
    public static void addDialogPreference(Class<? extends Preference> preferenceClass,
                                           Class<? extends Fragment> fragmentClass) {
        ...

I acknowledge that the custom ringtone picker approach has its advantages, but its disadvantages concerns me and I'd like to have the freedom for an alternative.

gregkorossy commented 6 years ago

Technically yes, there's no restriction on DialogPreference, but semantically this is odd and should use only DialogPreference because, well, you want to display a dialog. The DialogPreference's onClick() calls the PreferenceManager's showDialog(...) which then calls onDisplayPreferenceDialog(...). The original implementation of onDisplayPreferenceDialog(...) expects the returned fragment to be a DialogFragment so you can see that the devs obviously expect a dialog to be displayed there despite the generic Preference approach (I know that in the overriding class I use Fragment but still, the originally intended use is pretty clear here).

I know headless fragments and I wouldn't call this a headless but a dangling one because it just does not belong anywhere. It's basically used for hacking the system and starting an Activity.

As I said, I'm not changing this right now because I feel like this is a bad approach (beside the fact that this is clearly meant to be for internal use only, at least for the time being) and will investigate what better options would be available. Be patient...

gregkorossy commented 6 years ago

Here's a possible approach:

  1. The custom preference implements an interface that has two methods: one for receiving the PreferenceFragmentCompat instance so it can start the Activity itself, and one for receiving the results (same signature as onActivityResult(...))
  2. The fragment's onActivityResult(...) iterates over all of the preferences and calls onActivityResult(...) until the appropriate one is found

This removes the burden from the dev having to create a separate fragment and ensures that no dangling fragments are left behind upon returning from the target Activity. A possible drawback could be the case when the request codes equal in two different preferences but the request codes should also be user settable since they can also use the same request code elsewhere which could lead to problems (hence there's a setCustomRingtoneRequestCode(...) method in the RingtonePreference).

zhanghai commented 6 years ago

It's not a hack but a representation of a dialog that is displayed by an external activity. The second proposed approach is also what I have considered before, and I think it burdens developers by taking away one specific request code from them and forcing them to write the callback logic in every preference fragment they use the preference. So I took the Fragment approach.

On the contrary, using a fragment to represent the existence of a dialog activity and track its state is semantically correct, and leverages support library's implementation of Fragment.onActivitiyResult() which won't be delivered to any other Fragment and is self-contained which won't pollute developer's preference fragment code. It is not dangling because it spawns with the extern dialog activity and is removed when the activity finishes, that is, it is a representation of the existence of an external dialog.

Neither PreferenceFragmentCompat.onDisplayPreferenceDialog() nor PreferenceManager.showDialog() required a DialogPreference instead of a general Preference. So I believe when considering the design of a fixing library no extra constraints should be imposed on developers. Any Preference should be able to call showDialog() easily, and any Fragment (or anything) that can present a dialog to user in any form should still be allowed in a wrapper for onDisplayPreferenceDialog. In a word I think when designing a library it should tend to be more flexible instead of more restrictive, because there can always be some possible truly valid use cases that become prevented, and bad developers can always do bad things in other bad ways.

gregkorossy commented 6 years ago

The second approach does not take anything away. Since the preference receives a Fragment instance, it is more than welcome to do whatever it wants (add fragments to it for example), but it also makes it easier for those who just want to launch an Activity and get the results back without implementing extra fragments and whatnot. Another good thing about this approach is that the devs can control how (commit(), commitNow(), etc.) they want to add the fragment (if any).

This lib is technically not restricting anything. Yes, it (still an internal use only method, for the time being) requires a DialogPreference because it is meant to be used with those type of preferences (since they call the dialog showing logic on click by default). If I went back to the official way, the onDisplayPreferenceDialog() would only accept DialogFragment classes when instantiating them. Sure I can make the addDialogPreference(...) to accept a generic Preference just to satisfy the no-more-restriction need, but the second parameter would remain DialogFragment because the intended use is to display a dialog fragment for the preference unless the logic is changed there too.

If you still insist, sure, I can make it happen to add a Fragment via addDialogPreference(...) but I would prefer the second approach (the interface one) because it is more robust and leaves the originally intended, easy-to-use implementation of the preference dialog handling alone.

zhanghai commented 6 years ago

Yes, I agree the approach you proposed is totally viable and do has its advantages, but still I prefer a more flexible addDialogPreference(Preference, Fragment) based on the reasons I mentioned (or registerPreferenceDialogFragment semantically speaking). And since one can always show a dialog in a plain Fragment (e.g. A regular fragment that does something and wraps a dialog or a dialog fragment in it for some reason), requiring the fragment used in this mechanism to be a subclass of DialogFragment is kind of rigid.

So maybe can you just make it happen? (Accepting Preference and Fragment, being deprecated is ok)

gregkorossy commented 6 years ago

@DreaminginCodeZH Released v27.0.0.0. Use PreferenceFragmentCompat.registerPreferenceFragment(...) to register a preference of which the fragment should be added to the fragment manager when the preference is clicked.

zhanghai commented 6 years ago

Thanks!

zhanghai commented 6 years ago

Sorry to bother again, but can you call setTargetFragment() for a Fragment class just as what happens for a DialogFragment class? It is crucial in getting the PreferenceFragment and then the Preference, and this maintains the same behavior as DialogFragment without breaking anything.

And since I'm bothering again, the naming of registerPreferenceFragment() did not reflect the fact that it is for showDialog() and onDisplayPreferenceDialog(). Maybe registerPreferenceDialogFragment() is a better name?

gregkorossy commented 6 years ago

Yeah, setTargetFragment(...) can be added.

I think registerPreferenceDialogFragment(...) would be a good name if the second parameter was a DialogFragment class (which is not the case anymore). Maybe a registerPreferenceFragmentForDialog(...) or something, but I don't think the name matters too much as it's rarely used for custom preferences, most devs will use only the built-in ones to provide a settings screen quickly and easily. Also, the javadoc speaks for itself.