Skip to content
This repository has been archived by the owner on Feb 26, 2023. It is now read-only.

Annotation helpers for PreferenceActivity and PreferenceFragment #1162

Merged
merged 16 commits into from Apr 25, 2015

Conversation

WonderCsabo
Copy link
Member

This PR is a WIP, and related to #7.

I decided to use resource IDs instead of plain string preference keys in the annotations, since this is the suggested way (if you use a string resource as a key, you can use it both in java and xml). Also the implementation is easier, since we can rely on existing concepts.

The implementation is basically the same as View injection/listener. Currently there is some code duplication, but i am not sure we should remove that, because it would really obfuscate the intent of the code. But i gladly accept suggestions on that.

@yDelouis, @DayS, @dodgex i would appreciate your feedback.

public JInvocation findPreferenceByKey(JFieldRef idRef) {
JInvocation getString = invoke(_this(), "getString").arg(idRef);
JInvocation findPreferenceByKey = invoke(_this(), "findPreference");
findPreferenceByKey.arg(getString);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i would return on this line. not sure how to explain why, but it feels more clean for me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed.

@dodgex
Copy link
Member

dodgex commented Sep 24, 2014

from a quick read it looks ok so far.

@WonderCsabo WonderCsabo force-pushed the 7_preferenceScreen branch 3 times, most recently from 79fec41 to 11e8d24 Compare September 24, 2014 21:27
@WonderCsabo
Copy link
Member Author

Functionality is done. Tests and javadoc is for the next day...
Until that, please take a look at it, guys.


add(new IgnoredWhenDetachedHandler(processingEnvironment));
/* After injection methods must be after injections */
add(new AfterInjectHandler(processingEnvironment));
add(new AfterExtrasHandler(processingEnvironment));
add(new AfterViewsHandler(processingEnvironment));
add(new AfterPreferencesHandler(processingEnvironment));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a note that we keep in mind to update the wiki #1157

@WonderCsabo WonderCsabo force-pushed the 7_preferenceScreen branch 2 times, most recently from a57af5e to 7946a5a Compare September 25, 2014 13:09
@WonderCsabo
Copy link
Member Author

I updated PreferenceScreenHandler, because in case of Fragments addPreferencesFromResource cannot be called in init body. The reason for that is in that case, addPreferencesFromResource must be called after super.onCreate().

And another thing: @PreferenceHeaders automatically adds the headers, but from API 19 and up, another method also must be overriden. We could generate the method from the header xml, but i am not sure to do it, since this is a security feature.

if (variableElement.asType().toString().equals(CanonicalNameConstants.PREFERENCE)) {
call.arg(preferenceParam);
} else if (variableElement.asType().toString().equals(CanonicalNameConstants.OBJECT)) {
call.arg(newValueParam);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am just trying this PR in a real app. I think we could further help the user, if we not just allow Object here, but any type which can be a preference. So the client do not have to cast the parameter, we could do that instead of him.
For the built-in Android Preference classes, these types can be in the newValue parameter:

  • String (EditTextPreference, ListPreference, RingtonePreference)
  • Set (MultiSelectListPreference)
  • Boolean (CheckBoxPreference, RingtonePreference)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I implemented this.

@WonderCsabo WonderCsabo force-pushed the 7_preferenceScreen branch 4 times, most recently from 1996920 to 0ab7830 Compare September 25, 2014 20:52
@WonderCsabo
Copy link
Member Author

@yDelouis, if you can take a quick look at this, i will start tests and javadoc if you find this ok.

for (VariableElement variableElement : userParameters) {
if (variableElement.asType().toString().equals(CanonicalNameConstants.PREFERENCE)) {
call.arg(preferenceParam);
} else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't you use else if ? It would remove one level of indentation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@WonderCsabo
Copy link
Member Author

Rebased onto develop.

@WonderCsabo
Copy link
Member Author

I rebased onto develop again.

@yDelouis can you just review this quickly? If it is basically OK, i'll write tests and JavaDoc.

@dodgex
Copy link
Member

dodgex commented Nov 20, 2014

i had some time to read through the changes. i haven't found anything bad. but i had no chance yet to test it in an app. but you started this as you need it in your app, so i think that you are testing it that way. :P

@WonderCsabo
Copy link
Member Author

Thanks!
Yes, this is already working in my current project, this still needs a CR.
But i think i'll write the JavaDocs, because that may not change.

@dodgex
Copy link
Member

dodgex commented Nov 20, 2014

What does CR mean?

@WonderCsabo
Copy link
Member Author

Code review.

@WonderCsabo
Copy link
Member Author

I added JavaDocs and tests.

yDelouis added a commit that referenced this pull request Apr 25, 2015
Annotation helpers for PreferenceActivity and PreferenceFragment
@yDelouis yDelouis merged commit 128a851 into androidannotations:develop Apr 25, 2015
@yDelouis
Copy link
Contributor

It's okay.
Thanks for having applied the requested modifications.

@yDelouis yDelouis added this to the 3.3 milestone Apr 25, 2015
@WonderCsabo WonderCsabo deleted the 7_preferenceScreen branch April 28, 2015 21:31
@dodgex
Copy link
Member

dodgex commented Apr 30, 2015

duh. i just found that this PR got merged and wanted to update the pref branch in my app and while testing i found a little problem. while this might be working as intended, we may want to reconsider this.

case: i have some @PreferenceChange annotated methods. here i update e.g. the summary based on the new value. but now i realized that this annotation uses preference.setOnPreferenceChangeListener() to trigger the method what brings me to my problem, this callback is called on change, but BEFORE the change is persisted

Sets the callback to be invoked when this Preference is changed by the user (but before the internal state has been updated).

i think it would be nice to have both. a callback before and one after. before using these annotations i used getPreferenceScreen().getSharedPreferences().registerOnSharedPreferenceChangeListener(this); and implemented OnSharedPreferenceChangeListener.

i have another feature branch where i need the post change callback even more as i need to do some logic and fire some events.

@WonderCsabo
Copy link
Member Author

@dodgex i used this listener intentionally. This gives a chance to act before the value is persisted. The value will be persisted after it anyway. So why would you need OnSharedPreferenceChangeListener? If the preference is not registered after @PreferenceChange, you have bigger problems, and that should never happen actually.

@dodgex
Copy link
Member

dodgex commented Apr 30, 2015

in one of my listener i update the summary based on the new value and in another one i even update another preference and fire an event with the new pref value.

@WonderCsabo
Copy link
Member Author

Can you give more detail? I really do not understand why you cannot use current @PreferenceChange for that.

@dodgex
Copy link
Member

dodgex commented Apr 30, 2015

currently i do something like this

ListPreference pref = (ListPreference) findPreference(key);
if (pref != null) {
    pref.setSummary(pref.getEntry());
}

where findPreference(key) could be replaced by the Preference param of the annotated method. the problem is, that getEntry() returns the previous value not the new one. so if i e.g. have "world" selected and change the selection to "hello" i would still get "world" as summary. and then if i change it again (to something else) i would get "hello" as summary and not the new one.

@WonderCsabo
Copy link
Member Author

@PreferenceChange is passing you the new value.

@dodgex
Copy link
Member

dodgex commented Apr 30, 2015

Argh! Damn. :/ @dodgex RTFM!

@WonderCsabo Thank you. Nice Work! 👍

@WonderCsabo WonderCsabo restored the 7_preferenceScreen branch May 9, 2015 10:51
@WonderCsabo WonderCsabo deleted the 7_preferenceScreen branch May 9, 2015 10:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants