-
Notifications
You must be signed in to change notification settings - Fork 363
On launch, go to the last visited account #1784
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
Merged
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
44c0ce3
db test [nfc]: Factor out BoolGlobalSettings tests in its own group
sm-sayedi ed75f19
settings: Avoid redundant updates in setBool
sm-sayedi 331ed03
settings: Make general int global settings, so as to add without migr…
sm-sayedi f1a238c
notif test [nfc]: Remove a param that's effectively never used
chrisbobbe f1badd4
notif test: Decompose takeStartingRoutes
chrisbobbe c569596
settings: Add IntGlobalSetting.lastVisitedAccountId, and keep updated
sm-sayedi a26a469
test: Add eg.thirdAccount
sm-sayedi a9137f2
app: On launch, go to the last visited account
sm-sayedi File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -35,7 +35,8 @@ class GlobalSettings extends Table { | |||||||||
| .nullable()(); | ||||||||||
|
|
||||||||||
| // If adding a new column to this table, consider whether [BoolGlobalSettings] | ||||||||||
| // can do the job instead (by adding a value to the [BoolGlobalSetting] enum). | ||||||||||
| // or [IntGlobalSettings] can do the job instead (by adding a value to the | ||||||||||
| // [BoolGlobalSetting] or [IntGlobalSetting] enum). | ||||||||||
| // That way is more convenient, when it works, because | ||||||||||
| // it avoids a migration and therefore several added copies of our schema | ||||||||||
| // in the Drift generated files. | ||||||||||
|
|
@@ -50,6 +51,9 @@ class GlobalSettings extends Table { | |||||||||
| /// referring to a possible setting from [BoolGlobalSetting]. | ||||||||||
| /// For settings in [BoolGlobalSetting] without a row in this table, | ||||||||||
| /// the setting's value is that of [BoolGlobalSetting.default_]. | ||||||||||
| /// | ||||||||||
| /// See also: | ||||||||||
| /// - [IntGlobalSettings], the int-valued counterpart of this table. | ||||||||||
| @DataClassName('BoolGlobalSettingRow') | ||||||||||
| class BoolGlobalSettings extends Table { | ||||||||||
| /// The setting's name, a possible name from [BoolGlobalSetting]. | ||||||||||
|
|
@@ -73,6 +77,37 @@ class BoolGlobalSettings extends Table { | |||||||||
| Set<Column<Object>>? get primaryKey => {name}; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| /// The table of the user's int-valued, account-independent settings. | ||||||||||
| /// | ||||||||||
| /// These apply across all the user's accounts on this client | ||||||||||
| /// (i.e. on this install of the app on this device). | ||||||||||
| /// | ||||||||||
| /// Each row is a [IntGlobalSettingRow], | ||||||||||
| /// referring to a possible setting from [IntGlobalSetting]. | ||||||||||
| /// For settings in [IntGlobalSetting] without a row in this table, | ||||||||||
| /// the setting's value is `null`. | ||||||||||
| /// | ||||||||||
| /// See also: | ||||||||||
| /// - [BoolGlobalSettings], the bool-valued counterpart of this table. | ||||||||||
| @DataClassName('IntGlobalSettingRow') | ||||||||||
| class IntGlobalSettings extends Table { | ||||||||||
| /// The setting's name, a possible name from [IntGlobalSetting]. | ||||||||||
| /// | ||||||||||
| /// The table may have rows where [name] is not the name of any | ||||||||||
| /// enum value in [IntGlobalSetting]. | ||||||||||
| /// This happens if the app has previously run at a future or modified | ||||||||||
| /// version which had additional values in that enum, | ||||||||||
| /// and the user set one of those additional settings. | ||||||||||
| /// The app ignores any such unknown rows. | ||||||||||
| TextColumn get name => text()(); | ||||||||||
|
|
||||||||||
| /// The user's chosen value for the setting. | ||||||||||
| IntColumn get value => integer()(); | ||||||||||
|
|
||||||||||
| @override | ||||||||||
| Set<Column<Object>>? get primaryKey => {name}; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| /// The table of [Account] records in the app's database. | ||||||||||
| class Accounts extends Table { | ||||||||||
| /// The ID of this account in the app's local database. | ||||||||||
|
|
@@ -116,7 +151,7 @@ class UriConverter extends TypeConverter<Uri, String> { | |||||||||
| @override Uri fromSql(String fromDb) => Uri.parse(fromDb); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| @DriftDatabase(tables: [GlobalSettings, BoolGlobalSettings, Accounts]) | ||||||||||
| @DriftDatabase(tables: [GlobalSettings, BoolGlobalSettings, IntGlobalSettings, Accounts]) | ||||||||||
| class AppDatabase extends _$AppDatabase { | ||||||||||
| AppDatabase(super.e); | ||||||||||
|
|
||||||||||
|
|
@@ -129,7 +164,7 @@ class AppDatabase extends _$AppDatabase { | |||||||||
| // information on using the build_runner. | ||||||||||
| // * Write a migration in `_migrationSteps` below. | ||||||||||
| // * Write tests. | ||||||||||
| static const int latestSchemaVersion = 9; // See note. | ||||||||||
| static const int latestSchemaVersion = 11; // See note. | ||||||||||
|
|
||||||||||
| @override | ||||||||||
| int get schemaVersion => latestSchemaVersion; | ||||||||||
|
|
@@ -200,7 +235,30 @@ class AppDatabase extends _$AppDatabase { | |||||||||
| // assume there wasn't also the legacy app before that. | ||||||||||
| await m.database.update(schema.globalSettings).write( | ||||||||||
| RawValuesInsertable({'legacy_upgrade_state': Constant('noLegacy')})); | ||||||||||
| } | ||||||||||
| }, | ||||||||||
| from9To10: (m, schema) async { | ||||||||||
| await m.createTable(schema.intGlobalSettings); | ||||||||||
| }, | ||||||||||
| from10To11: (Migrator m, Schema11 schema) async { | ||||||||||
| // To provide a smooth experience for users when they first install a new | ||||||||||
| // version of the app with support for the "last visited account" feature, | ||||||||||
| // we set the first available account as the last visited one. This way, | ||||||||||
| // the user is still taken straight to the first account, just as they | ||||||||||
| // were used to before, instead of being shown the "choose account" page. | ||||||||||
| final firstAccountId = await (m.database.selectOnly(schema.accounts) | ||||||||||
| ..addColumns([schema.accounts.id]) | ||||||||||
| ..limit(1) | ||||||||||
| ).map((row) => row.read(schema.accounts.id)).getSingleOrNull(); | ||||||||||
| if (firstAccountId == null) return; | ||||||||||
|
|
||||||||||
| // Like `globalStore.setLastVisitedAccount(firstAccountId)`, | ||||||||||
| // as of the schema at the time of this migration. | ||||||||||
| await m.database.into(schema.intGlobalSettings).insert( | ||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, though, let's have a higher-level comment instead:
Suggested change
Then I think the one below isn't needed. |
||||||||||
| RawValuesInsertable({ | ||||||||||
| 'name': Variable('lastVisitedAccountId'), | ||||||||||
| 'value': Variable(firstAccountId), | ||||||||||
| })); | ||||||||||
| }, | ||||||||||
| ); | ||||||||||
|
|
||||||||||
| Future<void> _createLatestSchema(Migrator m) async { | ||||||||||
|
|
@@ -256,6 +314,14 @@ class AppDatabase extends _$AppDatabase { | |||||||||
| return result; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| Future<Map<IntGlobalSetting, int>> getIntGlobalSettings() async { | ||||||||||
| return { | ||||||||||
| for (final row in await select(intGlobalSettings).get()) | ||||||||||
| if (IntGlobalSetting.byName(row.name) case final setting?) | ||||||||||
| setting: row.value | ||||||||||
| }; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| Future<int> createAccount(AccountsCompanion values) async { | ||||||||||
| try { | ||||||||||
| return await into(accounts).insert(values); | ||||||||||
|
|
||||||||||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
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.
The commit 5cfd4ae, which added
BoolGlobalSettings, included a lot of useful material that I think should be easy to propagate to this new thing:(EDIT: and I see some of these have actually been addressed in the latest revision; I'll strike out those parts. 🙂)
GlobalSettingssaying "consider whether [etc.] can do the job instead", which helps us get the most usefulness from the thing. Let's change that comment so that it also mentions the int global settingsDartdocs on(Let's also add bidirectional "See also:" notes in that class's dartdoc and inBoolGlobalSettingsand its fields.IntGlobalSettings's.)BoolGlobalSettingenum, and it also got this helpful-looking implementation comment:placeholderIgnoremember, though; the pseudo-setting we're adding here isn't an "experimental feature" setting and we don't plan to remove it.)Tests