Skip to content
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

inject SharedPreferences #4441

Merged
merged 8 commits into from
May 24, 2024
Merged

inject SharedPreferences #4441

merged 8 commits into from
May 24, 2024

Conversation

connyduck
Copy link
Collaborator

(this one is for @charlag)

Calling PreferenceManager.getDefaultSharedPreferences() will read the preference file from disk every time. This PR makes SharedPreferences a singleton so they will only be created once at appstart (with a few exceptions where it is hard to inject, e.g. in the openLink helper) which should help getting our ANRs down.

StrictMode policy violation; ~duration=285 ms: android.os.strictmode.DiskReadViolation
    at android.os.StrictMode$AndroidBlockGuardPolicy.onReadFromDisk(StrictMode.java:1666)
    at libcore.io.BlockGuardOs.access(BlockGuardOs.java:74)
    at libcore.io.ForwardingOs.access(ForwardingOs.java:128)
    at android.app.ActivityThread$AndroidOs.access(ActivityThread.java:8054)
    at java.io.UnixFileSystem.checkAccess(UnixFileSystem.java:313)
    at java.io.File.exists(File.java:813)
    at android.app.ContextImpl.ensurePrivateDirExists(ContextImpl.java:790)
    at android.app.ContextImpl.ensurePrivateDirExists(ContextImpl.java:781)
    at android.app.ContextImpl.getPreferencesDir(ContextImpl.java:737)
    at android.app.ContextImpl.getSharedPreferencesPath(ContextImpl.java:962)
    at android.app.ContextImpl.getSharedPreferences(ContextImpl.java:583)
    at android.content.ContextWrapper.getSharedPreferences(ContextWrapper.java:221)
    at android.content.ContextWrapper.getSharedPreferences(ContextWrapper.java:221)
    at androidx.preference.PreferenceManager.getDefaultSharedPreferences(PreferenceManager.java:119)
    at com.keylesspalace.tusky.BaseActivity.onCreate(BaseActivity.java:96)
   ...

@connyduck connyduck requested review from Tak, Lakoja and charlag May 11, 2024 19:43
preferences = getSharedPreferences(
getString(R.string.preferences_file_key),
Context.MODE_PRIVATE
)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure why the SharedPreferences were created differently than everywhere else here?

@@ -125,7 +127,8 @@ private boolean activityTransitionWasRequested() {

@Override
protected void attachBaseContext(Context newBase) {
SharedPreferences preferences = PreferenceManager.getDefaultSharedPreferences(newBase);
// injected preferences not yet available in this point of the lifecycle
SharedPreferences preferences = ((TuskyApplication)newBase.getApplicationContext()).sharedPreferences;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Too hacky?

@cbeyls
Copy link
Contributor

cbeyls commented May 12, 2024

While I agree that it's better to inject preferences everywhere for consistency and to make testing easier, it's not true that a new instance gets created every time.

The system keeps a static cache of SharedPreferences by name, and that cache includes the default SharedPreferences as well.
So it's not necessary to declare it as @Singleton in the dagger module because it's already a singleton.

@@ -125,7 +127,8 @@ private boolean activityTransitionWasRequested() {

@Override
protected void attachBaseContext(Context newBase) {
SharedPreferences preferences = PreferenceManager.getDefaultSharedPreferences(newBase);
// injected preferences not yet available at this point of the lifecycle
SharedPreferences preferences = ((TuskyApplication)newBase.getApplicationContext()).getPreferences();
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider using a Dagger EntryPoint instead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh that is nice, will do

@@ -99,11 +96,6 @@ class LoginActivity : BaseActivity() {
.into(binding.loginLogo)
}

preferences = getSharedPreferences(
Copy link
Contributor

@cbeyls cbeyls May 12, 2024

Choose a reason for hiding this comment

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

This separate SharedPreferences instance was used to store temporary data that should ideally be stored to saveInstanceState instead of polluting the global SharedPreferences.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

🧐

@connyduck
Copy link
Collaborator Author

connyduck commented May 12, 2024

it's not true that a new instance gets created every time.

Maybe it is not a new instance, but according to StrictMode it does the disk read every time and that is my main concern here

@cbeyls
Copy link
Contributor

cbeyls commented May 12, 2024

Maybe it is not a new instance, but according to StrictMode it does the disk read every time and that is my main concern here

I was surprised because I didn't notice that behavior in my apps before. It turns out that the cache is Context-related so the file is read once per Context. If we make sure to pass the Application Context for every retrieval like we do now, the file will only be read once and the SharedPreferences don't need to be declared as Singleton in the Dagger module.

@connyduck connyduck changed the title inject SharedPreferences instead of creating a new instance every time inject SharedPreferences May 13, 2024
@connyduck connyduck merged commit d554d71 into develop May 24, 2024
3 checks passed
@connyduck connyduck deleted the inject-shared-preferences branch May 24, 2024 06:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants