-
Notifications
You must be signed in to change notification settings - Fork 133
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Proposal] Introduce kvs-schema #311
Conversation
@konifar I sent just a proposal PR. I would like to hear your opinion. |
This is what I thought! |
binding.notificationSwitchRow.init(PrefUtil.KEY_NOTIFICATION_SETTING, true); | ||
binding.localTimeSwitchRow.init(PrefUtil.KEY_SHOW_LOCAL_TIME, false); | ||
|
||
boolean shouldNotify = DefaultPrefsSchema.get(getContext()).getNotificationFlag(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder using getContext causes memory leaks cuz it just returns the Context this fragment is currently associated with. Should we use application context?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we don't have to use application context. Generally speaking, memory leak could occur when given context is used in different lifetime. Meanwhile in kvs-schema, DefaultPrefsSchema.get
runs synchronoushly and this library doesn't hold a reference of context. CMIIW 🙇
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok!
Almost awesome! This app will be a showcase of Japanese cool libraries. |
😍 |
👀 |
Thanks for your cool solution and detailed description! |
[Proposal] Introduce kvs-schema
Overview
Problem & Solution
Currently, this app retrieves shared preferences through
PrefUtil
. It's a typical way but it has some problems.Problem 1: Difficult to guess value types
PrefUtil
has keys below.It looks that
notification_setting
could take json andshow_local_time
could take date. At first glance, I didn't understand what kind of data are stored in this app 😕Problem 2: Need to declare similar accessors repeatedly
In this way, we need to declare new accessor when we add new value types.
Solution
I introduced kvs-schema that is a library to manage shared preferences. kvs-schema generates methods below statically using annotation processor.
get*
put*
remove*
has*
set*
(for Kotlin)By this change, we can just focus on key-value management.
How to introduce kvs-schema
For the case of existing app
Step 1: Add dependencies
Add dependencies to your
build.gradle
.Step 2: Declare schema class.
In this case,
PrefUtil
had keys below.Hence, the schema class becomes like below.
Notes 📝
io.github.droidkaigi.confsched_preferences.xml
(package name +"_preferences"
by default). So I declared the table name as"io.github.droidkaigi.confsched_preferences"
.flag
suffix for boolean field likeshowLocalTimeFlag
. kvs-schema generates method from field name. If you useis*
for boolean, the getter method name becomesgetIs*
. It doesn't look cool 😕DefaultPrefsSchema.get
as a static synchronized method. This method may not be called from many threads at the same time. I guess that the synchronization cost is not so high.How to add new key
Just add
@Key("key_name") Type field
to DefaultPrefsSchema.