Skip to content

Bug 2021407 - Adds shared prefs implementation of summarization settings#98

Closed
boek wants to merge 1 commit into
mozilla-firefox:autolandfrom
boek:bug_2021407_shared_prefs_s2s_settings
Closed

Bug 2021407 - Adds shared prefs implementation of summarization settings#98
boek wants to merge 1 commit into
mozilla-firefox:autolandfrom
boek:bug_2021407_shared_prefs_s2s_settings

Conversation

@boek

@boek boek commented Mar 5, 2026

Copy link
Copy Markdown
Contributor

try

@github-actions

github-actions Bot commented Mar 5, 2026

Copy link
Copy Markdown
Contributor

View this pull request in Lando to land it once approved.

@boek

boek commented Mar 5, 2026

Copy link
Copy Markdown
Contributor Author

try

@segunfamisa segunfamisa left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The changes look good to me, thanks for the PR. All my comments are nonblocking.

Do we have a try for this? seen it!

}

/**
* Creates am implementation of [SummarizationSettings] that is backed by [SharedPreferences].

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Creates am implementation of [SummarizationSettings] that is backed by [SharedPreferences].
* Creates an implementation of [SummarizationSettings] that is backed by [SharedPreferences].

internal class SharedPrefsBackedSummarizationSettings(
private val prefs: SharedPreferences,
) : SummarizationSettings {
override suspend fun getHasConsentedToShake(): Boolean {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I wonder if we can remove the "get" and make it less verbose

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🫡

fun sharedPrefs(
context: Context,
): SummarizationSettings {
val prefs = context.getSharedPreferences("summarization_prefs", MODE_PRIVATE)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I like (lowkey prefer) this, but i got the feeling that we wanted to stick to putting prefs in settings.kt and then have our settings backed version.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I agree, I'm going to land this for now and we can revisit next week as we wire everything else up.

@boek boek force-pushed the bug_2021407_shared_prefs_s2s_settings branch from ad2d5f7 to 2f9a4fd Compare March 5, 2026 21:24
@lando-worker

lando-worker Bot commented Mar 6, 2026

Copy link
Copy Markdown

Pull request closed by commit 0bbf19a

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants