Skip to content

GlobalSettingsStore, and other refactors #1416

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 19 commits into from
Mar 18, 2025
Merged
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
6e4feea
test [nfc]: Inline eg.globalSettings
gnprice Mar 14, 2025
17ad29c
db test: Test updating GlobalSettings
gnprice Mar 15, 2025
154e059
settings [nfc]: Drop return value from updateGlobalSettings
gnprice Mar 14, 2025
df4e145
settings [nfc]: Write docs on extension members
gnprice Mar 15, 2025
ea20bd4
settings [nfc]: Cut "todo" comment calling for settings in store
gnprice Mar 14, 2025
3fc1314
db [nfc]: Extract _migrationSteps as a static
gnprice Mar 13, 2025
6c041b1
db [nfc]: Make _dropAndCreateAll static too
gnprice Mar 13, 2025
2365bb3
db: Create GlobalSettings row as part of schema setup
gnprice Mar 13, 2025
bda2351
db [nfc]: Simplify name and tests of getGlobalSettings
gnprice Mar 14, 2025
70824b4
store [nfc]: Drop no-op should-notify override in _GlobalStoreInherit…
gnprice Mar 14, 2025
59e78c8
store test: Add test for GlobalStoreWidget.of
gnprice Mar 15, 2025
0566369
settings [nfc]: Split out a GlobalSettingsStore, and GlobalStoreWidge…
gnprice Mar 14, 2025
f177fe0
settings [nfc]: Move the settings data out to GlobalSettingsStore
gnprice Mar 14, 2025
1614322
settings [nfc]: Have inherited widget use GlobalSettingsStore directly
gnprice Mar 14, 2025
6483514
settings [nfc]: Have settingsOf return whole store, not just data
gnprice Mar 14, 2025
c107fe8
store [nfc]: Simplify name GlobalStore.settings, from globalSettings
gnprice Mar 14, 2025
793dbc7
store [nfc]: Separate "backend" class from store itself
gnprice Mar 14, 2025
1cf31a4
settings [nfc]: Use `update` method directly on GlobalSettingsStore
gnprice Mar 14, 2025
9201ae4
settings [nfc]: Give settings specific setter methods
gnprice Mar 14, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 44 additions & 35 deletions lib/model/database.dart
Original file line number Diff line number Diff line change
@@ -9,11 +9,13 @@ import 'settings.dart';

part 'database.g.dart';

/// The table of the user's chosen settings independent of account, on this
/// client.
/// The table of one [GlobalSettingsData] record, the user's chosen settings
/// on this client that are independent of account.
///
/// These apply across all the user's accounts on this client (i.e. on this
/// install of the app on this device).
///
/// This table should always have exactly one row (it's created by a migration).
@DataClassName('GlobalSettingsData')
class GlobalSettings extends Table {
Column<String> get themeSetting => textEnum<ThemeSetting>()
@@ -78,6 +80,8 @@ VersionedSchema _getSchema({
return Schema3(database: database);
case 4:
return Schema4(database: database);
case 5:
return Schema5(database: database);
default:
throw Exception('unknown schema version: $schemaVersion');
}
@@ -95,12 +99,12 @@ class AppDatabase extends _$AppDatabase {
// See ../../README.md#generated-files for more
// information on using the build_runner.
// * Update [_getSchema] to handle the new schemaVersion.
// * Write a migration in `onUpgrade` below.
// * Write a migration in `_migrationSteps` below.
// * Write tests.
@override
int get schemaVersion => 4; // See note.
int get schemaVersion => 5; // See note.

Future<void> _dropAndCreateAll(Migrator m, {
static Future<void> _dropAndCreateAll(Migrator m, {
required int schemaVersion,
}) async {
await m.database.transaction(() async {
@@ -125,11 +129,42 @@ class AppDatabase extends _$AppDatabase {
});
}

static final MigrationStepWithVersion _migrationSteps = migrationSteps(
from1To2: (m, schema) async {
await m.addColumn(schema.accounts, schema.accounts.ackedPushToken);
},
from2To3: (m, schema) async {
await m.createTable(schema.globalSettings);
},
from3To4: (m, schema) async {
await m.addColumn(
schema.globalSettings, schema.globalSettings.browserPreference);
},
from4To5: (m, schema) async {
// Corresponds to the `into(globalSettings).insert` in `onCreate`.
// This migration ensures there is a row in GlobalSettings.
// (If the app already ran at schema 3 or 4, there will be;
// if not, there won't be before this point.)
await m.database.transaction(() async {
final rows = await m.database.select(schema.globalSettings).get();
if (rows.isEmpty) {
await m.database.into(schema.globalSettings).insert(
// No field values; just use the defaults for both fields.
// (This is like `GlobalSettingsCompanion.insert()`, but
// without dependence on the current schema.)
RawValuesInsertable({}));
}
});
},
);

@override
MigrationStrategy get migration {
return MigrationStrategy(
onCreate: (Migrator m) async {
await m.createAll();
// Corresponds to `from4to5` above.
await into(globalSettings).insert(GlobalSettingsCompanion());
},
onUpgrade: (Migrator m, int from, int to) async {
if (from > to) {
@@ -142,39 +177,13 @@ class AppDatabase extends _$AppDatabase {
}
assert(1 <= from && from <= to && to <= schemaVersion);

await m.runMigrationSteps(from: from, to: to,
steps: migrationSteps(
from1To2: (m, schema) async {
await m.addColumn(schema.accounts, schema.accounts.ackedPushToken);
},
from2To3: (m, schema) async {
await m.createTable(schema.globalSettings);
},
from3To4: (m, schema) async {
await m.addColumn(
schema.globalSettings, schema.globalSettings.browserPreference);
},
));
await m.runMigrationSteps(from: from, to: to, steps: _migrationSteps);
});
}

Future<GlobalSettingsData> ensureGlobalSettings() async {
final settings = await select(globalSettings).get();
// TODO(db): Enforce the singleton constraint more robustly.
if (settings.isNotEmpty) {
if (settings.length > 1) {
assert(debugLog('Expected one globalSettings, got multiple: $settings'));
}
return settings.first;
}

final rowsAffected = await into(globalSettings).insert(GlobalSettingsCompanion.insert());
assert(rowsAffected == 1);
final result = await select(globalSettings).get();
if (result.length > 1) {
assert(debugLog('Expected one globalSettings, got multiple: $result'));
}
return result.first;
Future<GlobalSettingsData> getGlobalSettings() async {
// The migrations ensure there is a row.
return await (select(globalSettings)..limit(1)).getSingle();
}

Future<int> createAccount(AccountsCompanion values) async {
53 changes: 53 additions & 0 deletions lib/model/schema_versions.g.dart
Original file line number Diff line number Diff line change
@@ -241,10 +241,56 @@ i1.GeneratedColumn<String> _column_10(String aliasedName) =>
true,
type: i1.DriftSqlType.string,
);

final class Schema5 extends i0.VersionedSchema {
Schema5({required super.database}) : super(version: 5);
@override
late final List<i1.DatabaseSchemaEntity> entities = [
globalSettings,
accounts,
];
late final Shape2 globalSettings = Shape2(
source: i0.VersionedTable(
entityName: 'global_settings',
withoutRowId: false,
isStrict: false,
tableConstraints: [],
columns: [_column_9, _column_10],
attachedDatabase: database,
),
alias: null,
);
late final Shape0 accounts = Shape0(
source: i0.VersionedTable(
entityName: 'accounts',
withoutRowId: false,
isStrict: false,
tableConstraints: [
'UNIQUE(realm_url, user_id)',
'UNIQUE(realm_url, email)',
],
columns: [
_column_0,
_column_1,
_column_2,
_column_3,
_column_4,
_column_5,
_column_6,
_column_7,
_column_8,
],
attachedDatabase: database,
),
alias: null,
);
}

i0.MigrationStepWithVersion migrationSteps({
required Future<void> Function(i1.Migrator m, Schema2 schema) from1To2,
required Future<void> Function(i1.Migrator m, Schema3 schema) from2To3,
required Future<void> Function(i1.Migrator m, Schema4 schema) from3To4,
required Future<void> Function(i1.Migrator m, Schema5 schema) from4To5,
}) {
return (currentVersion, database) async {
switch (currentVersion) {
@@ -263,6 +309,11 @@ i0.MigrationStepWithVersion migrationSteps({
final migrator = i1.Migrator(database, schema);
await from3To4(migrator, schema);
return 4;
case 4:
final schema = Schema5(database: database);
final migrator = i1.Migrator(database, schema);
await from4To5(migrator, schema);
return 5;
default:
throw ArgumentError.value('Unknown migration from $currentVersion');
}
@@ -273,10 +324,12 @@ i1.OnUpgrade stepByStep({
required Future<void> Function(i1.Migrator m, Schema2 schema) from1To2,
required Future<void> Function(i1.Migrator m, Schema3 schema) from2To3,
required Future<void> Function(i1.Migrator m, Schema4 schema) from3To4,
required Future<void> Function(i1.Migrator m, Schema5 schema) from4To5,
}) => i0.VersionedSchema.stepByStepHelper(
step: migrationSteps(
from1To2: from1To2,
from2To3: from2To3,
from3To4: from3To4,
from4To5: from4To5,
),
);
53 changes: 52 additions & 1 deletion lib/model/settings.dart
Original file line number Diff line number Diff line change
@@ -3,6 +3,7 @@ import 'package:flutter/foundation.dart';
import '../generated/l10n/zulip_localizations.dart';
import 'binding.dart';
import 'database.dart';
import 'store.dart';

/// The user's choice of visual theme for the app.
///
@@ -44,7 +45,39 @@ enum BrowserPreference {
external,
}

extension GlobalSettingsHelpers on GlobalSettingsData {
/// Store for the user's account-independent settings.
///
/// From UI code, use [GlobalStoreWidget.settingsOf] to get hold of
/// an appropriate instance of this class.
class GlobalSettingsStore extends ChangeNotifier {
GlobalSettingsStore({
required GlobalStoreBackend backend,
required GlobalSettingsData data,
}) : _backend = backend, _data = data;

final GlobalStoreBackend _backend;

/// A cache of the [GlobalSettingsData] singleton in the underlying data store.
GlobalSettingsData _data;

/// The user's choice of [ThemeSetting];
/// null means the device-level choice of theme.
///
/// See also [setThemeSetting].
ThemeSetting? get themeSetting => _data.themeSetting;

/// The user's choice of [BrowserPreference];
/// null means use our default choice.
///
/// Consider using [effectiveBrowserPreference] or [getUrlLaunchMode].
///
/// See also [setBrowserPreference].
BrowserPreference? get browserPreference => _data.browserPreference;
Copy link
Member

Choose a reason for hiding this comment

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

A reason that effectiveBrowserPreference is separate from this was that we didn't opt into implementing a custom GlobalSettingsData.

Now that GlobalSettingsData _data is nicely hidden as a part of the implementation detail, we can rename effectiveBrowserPreference to browserPreference, replacing it, and skip the part of the dartdoc redirecting to effectiveBrowserPreference.

Copy link
Member

Choose a reason for hiding this comment

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

I guess keeping browserPreference nullable is consistent with setBrowserPreference, but I think we don't need to expose a way to set it to null.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I think hiding the raw browserPreference behind (a renamed) effectiveBrowserPreference could be a good follow-up change. Then setBrowserPreference would narrow its parameter type to match.

I'll leave that out of this PR, though, because I think it's not on the path to #1409.


/// The value of [BrowserPreference] to use:
/// the user's choice [browserPreference] if any, else our default.
///
/// See also [getUrlLaunchMode].
BrowserPreference get effectiveBrowserPreference {
if (browserPreference != null) return browserPreference!;
return switch (defaultTargetPlatform) {
@@ -61,6 +94,8 @@ extension GlobalSettingsHelpers on GlobalSettingsData {
};
}

/// The launch mode to use with `url_launcher`,
/// based on the user's choice in [browserPreference].
UrlLaunchMode getUrlLaunchMode(Uri url) {
switch (effectiveBrowserPreference) {
case BrowserPreference.inApp:
@@ -78,4 +113,20 @@ extension GlobalSettingsHelpers on GlobalSettingsData {
return UrlLaunchMode.externalApplication;
}
}

Copy link
Member

Choose a reason for hiding this comment

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

commit message nit:

settings [nfc]: Split out a GlobalSettingsStore, and GlobalStoreWidget.settingsOf

[…]

Those two points in turn will also help us adjust to be more
ergonomic the API the rest of the app uses for interacting with
these settings.

This part confuses me a little. Is there something missing here?

Copy link
Member Author

Choose a reason for hiding this comment

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

How's this?

Those two points in turn will also help us adjust the API the rest
of the app uses for interacting with these settings, so as to make
it more ergonomic.

Same thought but perhaps clearer syntax.

Future<void> _update(GlobalSettingsCompanion data) async {
await _backend.doUpdateGlobalSettings(data);
_data = _data.copyWithCompanion(data);
notifyListeners();
}

/// Set [themeSetting], persistently for future runs of the app.
Future<void> setThemeSetting(ThemeSetting? value) async {
await _update(GlobalSettingsCompanion(themeSetting: Value(value)));
}

/// Set [browserPreference], persistently for future runs of the app.
Future<void> setBrowserPreference(BrowserPreference? value) async {
await _update(GlobalSettingsCompanion(browserPreference: Value(value)));
}
}
Loading