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

wip; Store some client settings #1167

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from
2 changes: 2 additions & 0 deletions .gitattributes
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ assets/l10n/app_*.arb -diff
# though not the similar file that is the source for what to translate:
assets/l10n/app_en.arb diff

# Generated files for running migrations:
lib/model/schema_versions.dart -diff
# Generated files for testing migrations:
test/model/schemas/*.dart -diff
test/model/schemas/*.json -diff
Expand Down
3 changes: 3 additions & 0 deletions analysis_options.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@
include: package:flutter_lints/flutter.yaml

analyzer:
exclude:
# Generated code from drift can contain unused local variables.
- lib/model/database.g.dart
Comment on lines +7 to +8
Copy link
Member

Choose a reason for hiding this comment

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

This should be reported as a bug in Drift — it's fine for the generated code to do this but then it should contain an ignore comment for it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Filed simolus3/drift#3384 for that.

language:
strict-inference: true
strict-raw-types: true
Expand Down
4 changes: 2 additions & 2 deletions lib/model/binding.dart
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ abstract class ZulipBinding {
}

/// Get the app's singleton [GlobalStore],
/// calling [loadGlobalStore] if not already loaded.
/// loading it asynchronously if not already loaded.
///
/// Where possible, use [GlobalStoreWidget.of] to get access to a [GlobalStore].
/// Use this method only in contexts like notifications where
Expand Down Expand Up @@ -312,7 +312,7 @@ class PackageInfo {

/// A concrete binding for use in the live application.
///
/// The global store returned by [loadGlobalStore], and consequently by
/// The global store returned by [getGlobalStore], and consequently by
/// [GlobalStoreWidget.of] in application code, will be a [LiveGlobalStore].
/// It therefore uses a live server and live, persistent local database.
///
Expand Down
122 changes: 80 additions & 42 deletions lib/model/database.dart
Original file line number Diff line number Diff line change
@@ -1,12 +1,9 @@
import 'dart:io';

import 'package:drift/drift.dart';
import 'package:drift/native.dart';
import 'package:drift/remote.dart';
import 'package:path/path.dart' as path;
import 'package:path_provider/path_provider.dart';
import 'package:sqlite3/common.dart';

import 'schema_versions.dart';

part 'database.g.dart';

/// The table of [Account] records in the app's database.
Expand Down Expand Up @@ -39,29 +36,61 @@ class Accounts extends Table {

Column<String> get ackedPushToken => text().nullable()();

// If adding a column, be sure to add it to copyWithCompanion too.

@override
List<Set<Column<Object>>> get uniqueKeys => [
{realmUrl, userId},
{realmUrl, email},
];
}

extension AccountExtension on Account {
Account copyWithCompanion(AccountsCompanion data) { // TODO(drift): generate this
return Account(
id: data.id.present ? data.id.value : id,
realmUrl: data.realmUrl.present ? data.realmUrl.value : realmUrl,
userId: data.userId.present ? data.userId.value : userId,
email: data.email.present ? data.email.value : email,
apiKey: data.apiKey.present ? data.apiKey.value : apiKey,
zulipVersion: data.zulipVersion.present ? data.zulipVersion.value : zulipVersion,
zulipMergeBase: data.zulipMergeBase.present ? data.zulipMergeBase.value : zulipMergeBase,
zulipFeatureLevel: data.zulipFeatureLevel.present ? data.zulipFeatureLevel.value : zulipFeatureLevel,
ackedPushToken: data.ackedPushToken.present ? data.ackedPushToken.value : ackedPushToken,
);
}
/// The visual theme of the app.
///
/// See [zulipThemeData] for how themes are determined.
///
/// Renaming existing enum values will invalidate the database.
/// Write a migration if such a change is necessary.
enum ThemeSetting {
/// Corresponds to the default platform setting.
Comment on lines +46 to +52
Copy link
Member

Choose a reason for hiding this comment

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

Let's put these enum definitions in a new file lib/model/settings.dart.

Then that can also be the home of other logic we add in the future for individual settings.

unset,

/// Corresponds to [Brightness.light].
light,

/// Corresponds to [Brightness.dark].
dark,
}

/// What browser the user has set to use for opening links in messages.
///
/// See https://chat.zulip.org/#narrow/stream/48-mobile/topic/in-app.20browser
/// for the reasoning behind these options.
///
/// Renaming existing enum values will invalidate the database.
/// Write a migration if such a change is necessary.
enum BrowserPreference {
/// Use [UrlLaunchMode.externalApplication] on iOS,
/// [UrlLaunchMode.platformDefault] on Android.
unset,

/// Use the in-app browser.
embedded,

/// Use the user's default browser app.
external,
}

/// The table of the user's chosen settings independent of account, on this
/// client.
///
/// These apply across all the user's accounts on this client (i.e. on this
/// install of the app on this device).
@DataClassName('GlobalSettingsData')
class GlobalSettings extends Table {
Column<String> get themeSetting => textEnum<ThemeSetting>()
.withDefault(const Variable('unset'))();

Column<String> get browserPreference => textEnum<BrowserPreference>()
.withDefault(const Variable('unset'))();
}

class UriConverter extends TypeConverter<Uri, String> {
Expand All @@ -70,29 +99,19 @@ class UriConverter extends TypeConverter<Uri, String> {
@override Uri fromSql(String fromDb) => Uri.parse(fromDb);
}

LazyDatabase _openConnection() {
return LazyDatabase(() async {
// TODO decide if this path is the right one to use
final dbFolder = await getApplicationDocumentsDirectory();
final file = File(path.join(dbFolder.path, 'db.sqlite'));
return NativeDatabase.createInBackground(file);
});
}

@DriftDatabase(tables: [Accounts])
@DriftDatabase(tables: [Accounts, GlobalSettings])
class AppDatabase extends _$AppDatabase {
AppDatabase(super.e);

AppDatabase.live() : this(_openConnection());

// When updating the schema:
// * Make the change in the table classes, and bump schemaVersion.
// * Export the new schema and generate test migrations:
// $ tools/check --fix drift
// * Export the new schema and generate test migrations with drift,
// and generate database code with build_runner:
// $ tools/check --fix drift build_runner
// * Write a migration in `onUpgrade` below.
// * Write tests.
@override
int get schemaVersion => 2; // See note.
int get schemaVersion => 4; // See note.

@override
MigrationStrategy get migration {
Expand All @@ -115,12 +134,20 @@ class AppDatabase extends _$AppDatabase {
}
assert(1 <= from && from <= to && to <= schemaVersion);

if (from < 2 && 2 <= to) {
await m.addColumn(accounts, accounts.ackedPushToken);
}
// New migrations go here.
}
);
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);
},
Comment on lines +142 to +147
Copy link
Member

Choose a reason for hiding this comment

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

db: Start generating step by step migration helper

https://drift.simonbinder.eu/migrations/step_by_step/#manual-generation

Hmm, interesting. Yeah, this seems useful — I think this migration to 4 is already an example where this structure lets the new migration get added as just a separate thing, and with the more manual structure we'd have to have logic for it to interact with the migration to 3.

));
});
}

Future<int> createAccount(AccountsCompanion values) async {
Expand All @@ -138,6 +165,17 @@ class AppDatabase extends _$AppDatabase {
rethrow;
}
}

Future<GlobalSettingsData> ensureGlobalSettings() async {
final settings = await select(globalSettings).getSingleOrNull();
// TODO(db): Enforce the singleton constraint more robustly.
if (settings != null) {
return settings;
}

await into(globalSettings).insert(GlobalSettingsCompanion.insert());
return select(globalSettings).getSingle();
}
}

class AccountAlreadyExistsException implements Exception {}
Loading
Loading