[SES-2107] Optimise SharedPreferences#1554
[SES-2107] Optimise SharedPreferences#1554bemusementpark wants to merge 12 commits intooxen-io:devfrom
Conversation
bemusementpark
force-pushed
the
just-prefs
branch
from
d6eb959 to
a196a98
Compare
bemusementpark
changed the title
AL-Session
left a comment
AL-Session
left a comment
There was a problem hiding this comment.
Nice work! Maybe a couple of twiddles depending on if you agree w/ my comments or nah. Happy to approve once I've got that feedback.
| super.onResume(); | ||
| initializeScreenshotSecurity(true); | ||
| DynamicLanguageActivityHelper.recreateIfNotInCorrectLanguage(this, TextSecurePreferences.getLanguage(this)); | ||
| DynamicLanguageActivityHelper.recreateIfNotInCorrectLanguage(this, MessagingModuleConfiguration.getShared().getPrefs().getLanguage()); |
There was a problem hiding this comment.
Is it worth putting a... what's the word, um, convenience getter (? I can't think of it - but what I'm after is syntactic sugar!) on this so we can just call MessagingModuleConfiguration.getLanguage() or something?
Same for anything else that calls MessagingModuleConfiguration.getShared().getPrefs() before it's final "give me the thing".
It looks like you have this further down where you just call context.prefs.<something> - is that the same thing?
There was a problem hiding this comment.
Yeah, though ideally this is injected, I'll see if I can do that.
haha, yeah context.prefs should be the same thing, but again, if prefs is injected, that's more correct.
I'll look into it.
|
|
||
| // if (context.getString(R.string.note_to_self).toLowerCase().contains(constraint.toLowerCase()) && | ||
| // !numberList.contains(TextSecurePreferences.getLocalNumber(context))) | ||
| // !numberList.contains(MessagingModuleConfiguration.getShared().getPrefs().getLocalNumber(context))) |
There was a problem hiding this comment.
If we're not using this code maybe remove?
There was a problem hiding this comment.
eek, find and replace fixed the commented code 🤔
I'd be tempted to delete.
| long currentTime = System.currentTimeMillis(); | ||
| // 7 days | ||
| if (currentTime - TextSecurePreferences.getLastVacuumTime(context) > 604_800_000) { | ||
| if (currentTime - MessagingModuleConfiguration.getShared().getPrefs().getLastVacuumTime() > 604_800_000) { |
There was a problem hiding this comment.
Out of curiosity, why do you reckon it might be 604,800,000... that's oddly specific, isn't it?
Ah, it's 1 week in milliseconds. Can we use something like 1.weeks.inWholeMilliseconds instead?
There was a problem hiding this comment.
totally, that's 604_800_000 x thinking.
Although... I could add a courtesy Pref with an Converter to get/set Durations directly... 🤔
| toolbar.setOnFilterChangedListener(this); | ||
| toolbar.setOnLayoutChangedListener(this); | ||
| toolbar.setPersistence(GiphyActivityToolbarTextSecurePreferencesPersistence.fromContext(this)); | ||
| toolbar.setPersistence(new GiphyActivityToolbarTextSecurePreferencesPersistence()); |
There was a problem hiding this comment.
Should we make this a static class and just access it without the new?
There was a problem hiding this comment.
It's more correct to instantiate, as it needs prefs. I could make this thing get injected itself... and inject its members...
2 participants
Prefclass that encapsulates a preference key, its type and default value all in one place.Flowto observe changes to shared preferencesWhy add Pref? Pref is a place to put all of the important info about a preference and adds a degree of type-safety
name
default value
the type is implied by the default (or if the default is null)
Then when we get/set/observe it the type is enforced. The only way to mess it up is if the name of the pref is reused with another type, even in the past... maybe it'd be an idea to wipe all prefs we don't declare... 🤔 that'd be safer.
Why delete the static methods?
The static methods were not mockable
The static methods would create/find/fetch a new SharedPreferences for each call. slow? expensive? probably.
Why remove the interface?
The interface served no purpose as there was no alternative implementations,
the implementation was not in a separate module, it was in the same file.
Why add StateFlows to prefs?
StateFlow has a present value,
StateFlow emits changes