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

Uses the system theme as default theme #3813

Merged
merged 3 commits into from
Aug 23, 2023

Conversation

SpaceFox
Copy link
Contributor

@SpaceFox SpaceFox commented Jul 7, 2023

A small polish update to set the "System Design" as default theme.

This to avoid unexpected behaviour on first application usage, and to force the user to change settings few seconds only after the first Tusky opening – which is a quite bad first impression.

@nikclayton
Copy link
Contributor

Just tried this, and it doesn't work as expected from a fresh install, because the default value, if the preference is missing, is 0, and that's less than the value it's being compared against.

So even with a fresh install you still get the dark theme.

Unfortunately, the two cases where the preference is missing are:

  • this is a fresh install of the app
  • the user is updating from a version before this schema version preference was introduced

There needs to be a mechanism to distinguish between those two cases.

It might be possible to do that by inspecting the database. I'll investigate tomorrow.

@nikclayton
Copy link
Contributor

@SpaceFox Yeah -- simplest approach is probably, in TuskyApplication.kt:

  1. Inject an AppDatabase instance
  2. From the database instance, get an AccountDao instance, and call loadAll() on that

That'll return a list of accounts. If it's empty then this is a fresh install[1]. If it's not empty then this is an upgrade.

So something like (untested) around line 79 of TuskyApplication.kt:

val upgrading = db.accountDao().loadAll().isNotEmpty()
if (upgrading) {
    val oldVersion = sharedPreferences.getInt(PrefKeys.SCHEMA_VERSION, 0)
    if (oldVersion != SCHEMA_VERSION) {
        upgradeSharedPreferences(oldVersion, SCHEMA_VERSION)
    }
}

[1] There's a chance the user has also logged out of all their accounts, and is now logging in from scratch. That's probably sufficiently unlikely that we don't need to worry about it...

@connyduck
Copy link
Collaborator

Please don't overengineer this D:
Why not check the oldVersion in the preferences migration? If it is 0 its a new install

@nikclayton
Copy link
Contributor

Please don't overengineer this D: Why not check the oldVersion in the preferences migration? If it is 0 its a new install

Ah, yeah, that'll do it :-)

@nikclayton
Copy link
Contributor

Hi @SpaceFox , are you able to make the suggested changes?

@SpaceFox
Copy link
Contributor Author

SpaceFox commented Aug 6, 2023

Hi,

I’m back from holidays, I pushed the suggested updates.

@nikclayton
Copy link
Contributor

nikclayton commented Aug 23, 2023

Thank you. Sorry it took longer than I'd like to check this out.

Just in case it's needed for future reference, I checked the following scenarios and it all behaves as expected:

Upgrade

  • Wipe and install a fresh develop
  • Don't change settings
  • Update with PR
  • Should still be dark theme

PASS

Upgrade with theme (black)

  • Wipe and install a fresh develop
  • Change theme to black
  • Update with PR
  • Should still be black theme

PASS

Upgrade with theme (dark)

  • Wipe and install a fresh develop
  • Explicitly set the theme to dark (e.g., set to black, exit prefs, then set back to dark)
  • Update with PR
  • Should still be dark theme

PASS

Fresh install, system theme is light

  • Wipe and install from PR
  • Should be light theme

PASS

Fresh install, system theme is dark

  • Wipe and install from PR
  • Should be dark theme

PASS

@nikclayton nikclayton merged commit 85888ac into tuskyapp:develop Aug 23, 2023
3 checks passed
@SpaceFox SpaceFox deleted the default_theme_system branch August 23, 2023 13:36
@charlag charlag added this to the Tusky 24 milestone Nov 18, 2023
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.

4 participants