From 0a22d867be760e42dfd465dd71adad882ef717d0 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Tue, 17 Dec 2024 15:11:01 -0500 Subject: [PATCH 1/4] db: Store browser preference in database Signed-off-by: Zixuan James Li --- lib/model/database.dart | 11 +- lib/model/database.g.dart | 107 +++- lib/model/schema_versions.g.dart | 73 ++- lib/model/settings.dart | 12 + test/example_data.dart | 2 + test/model/database_test.dart | 21 + test/model/schemas/drift_schema_v4.json | 1 + test/model/schemas/schema.dart | 5 +- test/model/schemas/schema_v4.dart | 700 ++++++++++++++++++++++++ test/model/store_checks.dart | 1 + 10 files changed, 925 insertions(+), 8 deletions(-) create mode 100644 test/model/schemas/drift_schema_v4.json create mode 100644 test/model/schemas/schema_v4.dart diff --git a/lib/model/database.dart b/lib/model/database.dart index 263c1ab989..027cec9958 100644 --- a/lib/model/database.dart +++ b/lib/model/database.dart @@ -18,6 +18,9 @@ part 'database.g.dart'; class GlobalSettings extends Table { Column get themeSetting => textEnum() .nullable()(); + + Column get browserPreference => textEnum() + .nullable()(); } /// The table of [Account] records in the app's database. @@ -73,6 +76,8 @@ VersionedSchema _getSchema({ return Schema2(database: database); case 3: return Schema3(database: database); + case 4: + return Schema4(database: database); default: throw Exception('unknown schema version: $schemaVersion'); } @@ -93,7 +98,7 @@ class AppDatabase extends _$AppDatabase { // * Write a migration in `onUpgrade` below. // * Write tests. @override - int get schemaVersion => 3; // See note. + int get schemaVersion => 4; // See note. Future _dropAndCreateAll(Migrator m, { required int schemaVersion, @@ -145,6 +150,10 @@ class AppDatabase extends _$AppDatabase { from2To3: (m, schema) async { await m.createTable(schema.globalSettings); }, + from3To4: (m, schema) async { + await m.addColumn( + schema.globalSettings, schema.globalSettings.browserPreference); + }, )); }); } diff --git a/lib/model/database.g.dart b/lib/model/database.g.dart index fcb5292aa6..9c0946af5b 100644 --- a/lib/model/database.g.dart +++ b/lib/model/database.g.dart @@ -21,7 +21,18 @@ class $GlobalSettingsTable extends GlobalSettings requiredDuringInsert: false, ).withConverter($GlobalSettingsTable.$converterthemeSettingn); @override - List get $columns => [themeSetting]; + late final GeneratedColumnWithTypeConverter + browserPreference = GeneratedColumn( + 'browser_preference', + aliasedName, + true, + type: DriftSqlType.string, + requiredDuringInsert: false, + ).withConverter( + $GlobalSettingsTable.$converterbrowserPreferencen, + ); + @override + List get $columns => [themeSetting, browserPreference]; @override String get aliasedName => _alias ?? actualTableName; @override @@ -39,6 +50,13 @@ class $GlobalSettingsTable extends GlobalSettings data['${effectivePrefix}theme_setting'], ), ), + browserPreference: $GlobalSettingsTable.$converterbrowserPreferencen + .fromSql( + attachedDatabase.typeMapping.read( + DriftSqlType.string, + data['${effectivePrefix}browser_preference'], + ), + ), ); } @@ -55,12 +73,21 @@ class $GlobalSettingsTable extends GlobalSettings $converterthemeSettingn = JsonTypeConverter2.asNullable( $converterthemeSetting, ); + static JsonTypeConverter2 + $converterbrowserPreference = const EnumNameConverter( + BrowserPreference.values, + ); + static JsonTypeConverter2 + $converterbrowserPreferencen = JsonTypeConverter2.asNullable( + $converterbrowserPreference, + ); } class GlobalSettingsData extends DataClass implements Insertable { final ThemeSetting? themeSetting; - const GlobalSettingsData({this.themeSetting}); + final BrowserPreference? browserPreference; + const GlobalSettingsData({this.themeSetting, this.browserPreference}); @override Map toColumns(bool nullToAbsent) { final map = {}; @@ -69,6 +96,13 @@ class GlobalSettingsData extends DataClass $GlobalSettingsTable.$converterthemeSettingn.toSql(themeSetting), ); } + if (!nullToAbsent || browserPreference != null) { + map['browser_preference'] = Variable( + $GlobalSettingsTable.$converterbrowserPreferencen.toSql( + browserPreference, + ), + ); + } return map; } @@ -78,6 +112,10 @@ class GlobalSettingsData extends DataClass themeSetting == null && nullToAbsent ? const Value.absent() : Value(themeSetting), + browserPreference: + browserPreference == null && nullToAbsent + ? const Value.absent() + : Value(browserPreference), ); } @@ -90,6 +128,8 @@ class GlobalSettingsData extends DataClass themeSetting: $GlobalSettingsTable.$converterthemeSettingn.fromJson( serializer.fromJson(json['themeSetting']), ), + browserPreference: $GlobalSettingsTable.$converterbrowserPreferencen + .fromJson(serializer.fromJson(json['browserPreference'])), ); } @override @@ -99,13 +139,23 @@ class GlobalSettingsData extends DataClass 'themeSetting': serializer.toJson( $GlobalSettingsTable.$converterthemeSettingn.toJson(themeSetting), ), + 'browserPreference': serializer.toJson( + $GlobalSettingsTable.$converterbrowserPreferencen.toJson( + browserPreference, + ), + ), }; } GlobalSettingsData copyWith({ Value themeSetting = const Value.absent(), + Value browserPreference = const Value.absent(), }) => GlobalSettingsData( themeSetting: themeSetting.present ? themeSetting.value : this.themeSetting, + browserPreference: + browserPreference.present + ? browserPreference.value + : this.browserPreference, ); GlobalSettingsData copyWithCompanion(GlobalSettingsCompanion data) { return GlobalSettingsData( @@ -113,52 +163,66 @@ class GlobalSettingsData extends DataClass data.themeSetting.present ? data.themeSetting.value : this.themeSetting, + browserPreference: + data.browserPreference.present + ? data.browserPreference.value + : this.browserPreference, ); } @override String toString() { return (StringBuffer('GlobalSettingsData(') - ..write('themeSetting: $themeSetting') + ..write('themeSetting: $themeSetting, ') + ..write('browserPreference: $browserPreference') ..write(')')) .toString(); } @override - int get hashCode => themeSetting.hashCode; + int get hashCode => Object.hash(themeSetting, browserPreference); @override bool operator ==(Object other) => identical(this, other) || - (other is GlobalSettingsData && other.themeSetting == this.themeSetting); + (other is GlobalSettingsData && + other.themeSetting == this.themeSetting && + other.browserPreference == this.browserPreference); } class GlobalSettingsCompanion extends UpdateCompanion { final Value themeSetting; + final Value browserPreference; final Value rowid; const GlobalSettingsCompanion({ this.themeSetting = const Value.absent(), + this.browserPreference = const Value.absent(), this.rowid = const Value.absent(), }); GlobalSettingsCompanion.insert({ this.themeSetting = const Value.absent(), + this.browserPreference = const Value.absent(), this.rowid = const Value.absent(), }); static Insertable custom({ Expression? themeSetting, + Expression? browserPreference, Expression? rowid, }) { return RawValuesInsertable({ if (themeSetting != null) 'theme_setting': themeSetting, + if (browserPreference != null) 'browser_preference': browserPreference, if (rowid != null) 'rowid': rowid, }); } GlobalSettingsCompanion copyWith({ Value? themeSetting, + Value? browserPreference, Value? rowid, }) { return GlobalSettingsCompanion( themeSetting: themeSetting ?? this.themeSetting, + browserPreference: browserPreference ?? this.browserPreference, rowid: rowid ?? this.rowid, ); } @@ -171,6 +235,13 @@ class GlobalSettingsCompanion extends UpdateCompanion { $GlobalSettingsTable.$converterthemeSettingn.toSql(themeSetting.value), ); } + if (browserPreference.present) { + map['browser_preference'] = Variable( + $GlobalSettingsTable.$converterbrowserPreferencen.toSql( + browserPreference.value, + ), + ); + } if (rowid.present) { map['rowid'] = Variable(rowid.value); } @@ -181,6 +252,7 @@ class GlobalSettingsCompanion extends UpdateCompanion { String toString() { return (StringBuffer('GlobalSettingsCompanion(') ..write('themeSetting: $themeSetting, ') + ..write('browserPreference: $browserPreference, ') ..write('rowid: $rowid') ..write(')')) .toString(); @@ -804,11 +876,13 @@ abstract class _$AppDatabase extends GeneratedDatabase { typedef $$GlobalSettingsTableCreateCompanionBuilder = GlobalSettingsCompanion Function({ Value themeSetting, + Value browserPreference, Value rowid, }); typedef $$GlobalSettingsTableUpdateCompanionBuilder = GlobalSettingsCompanion Function({ Value themeSetting, + Value browserPreference, Value rowid, }); @@ -826,6 +900,12 @@ class $$GlobalSettingsTableFilterComposer column: $table.themeSetting, builder: (column) => ColumnWithTypeConverterFilters(column), ); + + ColumnWithTypeConverterFilters + get browserPreference => $composableBuilder( + column: $table.browserPreference, + builder: (column) => ColumnWithTypeConverterFilters(column), + ); } class $$GlobalSettingsTableOrderingComposer @@ -841,6 +921,11 @@ class $$GlobalSettingsTableOrderingComposer column: $table.themeSetting, builder: (column) => ColumnOrderings(column), ); + + ColumnOrderings get browserPreference => $composableBuilder( + column: $table.browserPreference, + builder: (column) => ColumnOrderings(column), + ); } class $$GlobalSettingsTableAnnotationComposer @@ -857,6 +942,12 @@ class $$GlobalSettingsTableAnnotationComposer column: $table.themeSetting, builder: (column) => column, ); + + GeneratedColumnWithTypeConverter + get browserPreference => $composableBuilder( + column: $table.browserPreference, + builder: (column) => column, + ); } class $$GlobalSettingsTableTableManager @@ -901,17 +992,23 @@ class $$GlobalSettingsTableTableManager updateCompanionCallback: ({ Value themeSetting = const Value.absent(), + Value browserPreference = + const Value.absent(), Value rowid = const Value.absent(), }) => GlobalSettingsCompanion( themeSetting: themeSetting, + browserPreference: browserPreference, rowid: rowid, ), createCompanionCallback: ({ Value themeSetting = const Value.absent(), + Value browserPreference = + const Value.absent(), Value rowid = const Value.absent(), }) => GlobalSettingsCompanion.insert( themeSetting: themeSetting, + browserPreference: browserPreference, rowid: rowid, ), withReferenceMapper: diff --git a/lib/model/schema_versions.g.dart b/lib/model/schema_versions.g.dart index bb5a62475a..aa2dec5a0c 100644 --- a/lib/model/schema_versions.g.dart +++ b/lib/model/schema_versions.g.dart @@ -181,9 +181,70 @@ i1.GeneratedColumn _column_9(String aliasedName) => true, type: i1.DriftSqlType.string, ); + +final class Schema4 extends i0.VersionedSchema { + Schema4({required super.database}) : super(version: 4); + @override + late final List 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, + ); +} + +class Shape2 extends i0.VersionedTable { + Shape2({required super.source, required super.alias}) : super.aliased(); + i1.GeneratedColumn get themeSetting => + columnsByName['theme_setting']! as i1.GeneratedColumn; + i1.GeneratedColumn get browserPreference => + columnsByName['browser_preference']! as i1.GeneratedColumn; +} + +i1.GeneratedColumn _column_10(String aliasedName) => + i1.GeneratedColumn( + 'browser_preference', + aliasedName, + true, + type: i1.DriftSqlType.string, + ); i0.MigrationStepWithVersion migrationSteps({ required Future Function(i1.Migrator m, Schema2 schema) from1To2, required Future Function(i1.Migrator m, Schema3 schema) from2To3, + required Future Function(i1.Migrator m, Schema4 schema) from3To4, }) { return (currentVersion, database) async { switch (currentVersion) { @@ -197,6 +258,11 @@ i0.MigrationStepWithVersion migrationSteps({ final migrator = i1.Migrator(database, schema); await from2To3(migrator, schema); return 3; + case 3: + final schema = Schema4(database: database); + final migrator = i1.Migrator(database, schema); + await from3To4(migrator, schema); + return 4; default: throw ArgumentError.value('Unknown migration from $currentVersion'); } @@ -206,6 +272,11 @@ i0.MigrationStepWithVersion migrationSteps({ i1.OnUpgrade stepByStep({ required Future Function(i1.Migrator m, Schema2 schema) from1To2, required Future Function(i1.Migrator m, Schema3 schema) from2To3, + required Future Function(i1.Migrator m, Schema4 schema) from3To4, }) => i0.VersionedSchema.stepByStepHelper( - step: migrationSteps(from1To2: from1To2, from2To3: from2To3), + step: migrationSteps( + from1To2: from1To2, + from2To3: from2To3, + from3To4: from3To4, + ), ); diff --git a/lib/model/settings.dart b/lib/model/settings.dart index 535d06aa01..2a5262cee7 100644 --- a/lib/model/settings.dart +++ b/lib/model/settings.dart @@ -24,3 +24,15 @@ enum ThemeSetting { }; } } + +/// What browser the user has set to use for opening links in messages. +/// +/// Renaming existing enum values will invalidate the database. +/// Write a migration if such a change is necessary. +enum BrowserPreference { + /// Use the in-app browser for HTTP links. + inApp, + + /// Use the user's default browser app. + external, +} diff --git a/test/example_data.dart b/test/example_data.dart index 03cabbda97..955f5dacfb 100644 --- a/test/example_data.dart +++ b/test/example_data.dart @@ -880,9 +880,11 @@ ChannelUpdateEvent channelUpdateEvent( GlobalSettingsData globalSettings({ ThemeSetting? themeSetting, + BrowserPreference? browserPreference, }) { return GlobalSettingsData( themeSetting: themeSetting, + browserPreference: browserPreference, ); } diff --git a/test/model/database_test.dart b/test/model/database_test.dart index 90dbcbddf1..28441e13ec 100644 --- a/test/model/database_test.dart +++ b/test/model/database_test.dart @@ -9,6 +9,8 @@ import 'package:zulip/model/settings.dart'; import 'schemas/schema.dart'; import 'schemas/schema_v1.dart' as v1; import 'schemas/schema_v2.dart' as v2; +import 'schemas/schema_v3.dart' as v3; +import 'schemas/schema_v4.dart' as v4; import 'store_checks.dart'; void main() { @@ -204,6 +206,25 @@ void main() { }); await after.close(); }); + + test('upgrade to v4, with data', () async { + final schema = await verifier.schemaAt(3); + final before = v3.DatabaseAtV3(schema.newConnection()); + await before.into(before.globalSettings).insert( + v3.GlobalSettingsCompanion.insert( + themeSetting: Value(ThemeSetting.light.name))); + await before.close(); + + final db = AppDatabase(schema.newConnection()); + await verifier.migrateAndValidate(db, 4); + await db.close(); + + final after = v4.DatabaseAtV4(schema.newConnection()); + final globalSettings = await after.select(after.globalSettings).getSingle(); + check(globalSettings.themeSetting).equals(ThemeSetting.light.name); + check(globalSettings.browserPreference).isNull(); + await after.close(); + }); }); } diff --git a/test/model/schemas/drift_schema_v4.json b/test/model/schemas/drift_schema_v4.json new file mode 100644 index 0000000000..718222de66 --- /dev/null +++ b/test/model/schemas/drift_schema_v4.json @@ -0,0 +1 @@ +{"_meta":{"description":"This file contains a serialized version of schema entities for drift.","version":"1.2.0"},"options":{"store_date_time_values_as_text":false},"entities":[{"id":0,"references":[],"type":"table","data":{"name":"global_settings","was_declared_in_moor":false,"columns":[{"name":"theme_setting","getter_name":"themeSetting","moor_type":"string","nullable":true,"customConstraints":null,"default_dart":null,"default_client_dart":null,"dsl_features":[],"type_converter":{"dart_expr":"const EnumNameConverter(ThemeSetting.values)","dart_type_name":"ThemeSetting"}},{"name":"browser_preference","getter_name":"browserPreference","moor_type":"string","nullable":true,"customConstraints":null,"default_dart":null,"default_client_dart":null,"dsl_features":[],"type_converter":{"dart_expr":"const EnumNameConverter(BrowserPreference.values)","dart_type_name":"BrowserPreference"}}],"is_virtual":false,"without_rowid":false,"constraints":[]}},{"id":1,"references":[],"type":"table","data":{"name":"accounts","was_declared_in_moor":false,"columns":[{"name":"id","getter_name":"id","moor_type":"int","nullable":false,"customConstraints":null,"defaultConstraints":"PRIMARY KEY AUTOINCREMENT","dialectAwareDefaultConstraints":{"sqlite":"PRIMARY KEY AUTOINCREMENT"},"default_dart":null,"default_client_dart":null,"dsl_features":["auto-increment"]},{"name":"realm_url","getter_name":"realmUrl","moor_type":"string","nullable":false,"customConstraints":null,"default_dart":null,"default_client_dart":null,"dsl_features":[],"type_converter":{"dart_expr":"const UriConverter()","dart_type_name":"Uri"}},{"name":"user_id","getter_name":"userId","moor_type":"int","nullable":false,"customConstraints":null,"default_dart":null,"default_client_dart":null,"dsl_features":[]},{"name":"email","getter_name":"email","moor_type":"string","nullable":false,"customConstraints":null,"default_dart":null,"default_client_dart":null,"dsl_features":[]},{"name":"api_key","getter_name":"apiKey","moor_type":"string","nullable":false,"customConstraints":null,"default_dart":null,"default_client_dart":null,"dsl_features":[]},{"name":"zulip_version","getter_name":"zulipVersion","moor_type":"string","nullable":false,"customConstraints":null,"default_dart":null,"default_client_dart":null,"dsl_features":[]},{"name":"zulip_merge_base","getter_name":"zulipMergeBase","moor_type":"string","nullable":true,"customConstraints":null,"default_dart":null,"default_client_dart":null,"dsl_features":[]},{"name":"zulip_feature_level","getter_name":"zulipFeatureLevel","moor_type":"int","nullable":false,"customConstraints":null,"default_dart":null,"default_client_dart":null,"dsl_features":[]},{"name":"acked_push_token","getter_name":"ackedPushToken","moor_type":"string","nullable":true,"customConstraints":null,"default_dart":null,"default_client_dart":null,"dsl_features":[]}],"is_virtual":false,"without_rowid":false,"constraints":[],"unique_keys":[["realm_url","user_id"],["realm_url","email"]]}}]} \ No newline at end of file diff --git a/test/model/schemas/schema.dart b/test/model/schemas/schema.dart index 209e70d788..22131b11bb 100644 --- a/test/model/schemas/schema.dart +++ b/test/model/schemas/schema.dart @@ -6,6 +6,7 @@ import 'package:drift/internal/migrations.dart'; import 'schema_v1.dart' as v1; import 'schema_v2.dart' as v2; import 'schema_v3.dart' as v3; +import 'schema_v4.dart' as v4; class GeneratedHelper implements SchemaInstantiationHelper { @override @@ -17,10 +18,12 @@ class GeneratedHelper implements SchemaInstantiationHelper { return v2.DatabaseAtV2(db); case 3: return v3.DatabaseAtV3(db); + case 4: + return v4.DatabaseAtV4(db); default: throw MissingSchemaException(version, versions); } } - static const versions = const [1, 2, 3]; + static const versions = const [1, 2, 3, 4]; } diff --git a/test/model/schemas/schema_v4.dart b/test/model/schemas/schema_v4.dart new file mode 100644 index 0000000000..e53e4fbe2a --- /dev/null +++ b/test/model/schemas/schema_v4.dart @@ -0,0 +1,700 @@ +// dart format width=80 +// GENERATED CODE, DO NOT EDIT BY HAND. +// ignore_for_file: type=lint +import 'package:drift/drift.dart'; + +class GlobalSettings extends Table + with TableInfo { + @override + final GeneratedDatabase attachedDatabase; + final String? _alias; + GlobalSettings(this.attachedDatabase, [this._alias]); + late final GeneratedColumn themeSetting = GeneratedColumn( + 'theme_setting', + aliasedName, + true, + type: DriftSqlType.string, + requiredDuringInsert: false, + ); + late final GeneratedColumn browserPreference = + GeneratedColumn( + 'browser_preference', + aliasedName, + true, + type: DriftSqlType.string, + requiredDuringInsert: false, + ); + @override + List get $columns => [themeSetting, browserPreference]; + @override + String get aliasedName => _alias ?? actualTableName; + @override + String get actualTableName => $name; + static const String $name = 'global_settings'; + @override + Set get $primaryKey => const {}; + @override + GlobalSettingsData map(Map data, {String? tablePrefix}) { + final effectivePrefix = tablePrefix != null ? '$tablePrefix.' : ''; + return GlobalSettingsData( + themeSetting: attachedDatabase.typeMapping.read( + DriftSqlType.string, + data['${effectivePrefix}theme_setting'], + ), + browserPreference: attachedDatabase.typeMapping.read( + DriftSqlType.string, + data['${effectivePrefix}browser_preference'], + ), + ); + } + + @override + GlobalSettings createAlias(String alias) { + return GlobalSettings(attachedDatabase, alias); + } +} + +class GlobalSettingsData extends DataClass + implements Insertable { + final String? themeSetting; + final String? browserPreference; + const GlobalSettingsData({this.themeSetting, this.browserPreference}); + @override + Map toColumns(bool nullToAbsent) { + final map = {}; + if (!nullToAbsent || themeSetting != null) { + map['theme_setting'] = Variable(themeSetting); + } + if (!nullToAbsent || browserPreference != null) { + map['browser_preference'] = Variable(browserPreference); + } + return map; + } + + GlobalSettingsCompanion toCompanion(bool nullToAbsent) { + return GlobalSettingsCompanion( + themeSetting: + themeSetting == null && nullToAbsent + ? const Value.absent() + : Value(themeSetting), + browserPreference: + browserPreference == null && nullToAbsent + ? const Value.absent() + : Value(browserPreference), + ); + } + + factory GlobalSettingsData.fromJson( + Map json, { + ValueSerializer? serializer, + }) { + serializer ??= driftRuntimeOptions.defaultSerializer; + return GlobalSettingsData( + themeSetting: serializer.fromJson(json['themeSetting']), + browserPreference: serializer.fromJson( + json['browserPreference'], + ), + ); + } + @override + Map toJson({ValueSerializer? serializer}) { + serializer ??= driftRuntimeOptions.defaultSerializer; + return { + 'themeSetting': serializer.toJson(themeSetting), + 'browserPreference': serializer.toJson(browserPreference), + }; + } + + GlobalSettingsData copyWith({ + Value themeSetting = const Value.absent(), + Value browserPreference = const Value.absent(), + }) => GlobalSettingsData( + themeSetting: themeSetting.present ? themeSetting.value : this.themeSetting, + browserPreference: + browserPreference.present + ? browserPreference.value + : this.browserPreference, + ); + GlobalSettingsData copyWithCompanion(GlobalSettingsCompanion data) { + return GlobalSettingsData( + themeSetting: + data.themeSetting.present + ? data.themeSetting.value + : this.themeSetting, + browserPreference: + data.browserPreference.present + ? data.browserPreference.value + : this.browserPreference, + ); + } + + @override + String toString() { + return (StringBuffer('GlobalSettingsData(') + ..write('themeSetting: $themeSetting, ') + ..write('browserPreference: $browserPreference') + ..write(')')) + .toString(); + } + + @override + int get hashCode => Object.hash(themeSetting, browserPreference); + @override + bool operator ==(Object other) => + identical(this, other) || + (other is GlobalSettingsData && + other.themeSetting == this.themeSetting && + other.browserPreference == this.browserPreference); +} + +class GlobalSettingsCompanion extends UpdateCompanion { + final Value themeSetting; + final Value browserPreference; + final Value rowid; + const GlobalSettingsCompanion({ + this.themeSetting = const Value.absent(), + this.browserPreference = const Value.absent(), + this.rowid = const Value.absent(), + }); + GlobalSettingsCompanion.insert({ + this.themeSetting = const Value.absent(), + this.browserPreference = const Value.absent(), + this.rowid = const Value.absent(), + }); + static Insertable custom({ + Expression? themeSetting, + Expression? browserPreference, + Expression? rowid, + }) { + return RawValuesInsertable({ + if (themeSetting != null) 'theme_setting': themeSetting, + if (browserPreference != null) 'browser_preference': browserPreference, + if (rowid != null) 'rowid': rowid, + }); + } + + GlobalSettingsCompanion copyWith({ + Value? themeSetting, + Value? browserPreference, + Value? rowid, + }) { + return GlobalSettingsCompanion( + themeSetting: themeSetting ?? this.themeSetting, + browserPreference: browserPreference ?? this.browserPreference, + rowid: rowid ?? this.rowid, + ); + } + + @override + Map toColumns(bool nullToAbsent) { + final map = {}; + if (themeSetting.present) { + map['theme_setting'] = Variable(themeSetting.value); + } + if (browserPreference.present) { + map['browser_preference'] = Variable(browserPreference.value); + } + if (rowid.present) { + map['rowid'] = Variable(rowid.value); + } + return map; + } + + @override + String toString() { + return (StringBuffer('GlobalSettingsCompanion(') + ..write('themeSetting: $themeSetting, ') + ..write('browserPreference: $browserPreference, ') + ..write('rowid: $rowid') + ..write(')')) + .toString(); + } +} + +class Accounts extends Table with TableInfo { + @override + final GeneratedDatabase attachedDatabase; + final String? _alias; + Accounts(this.attachedDatabase, [this._alias]); + late final GeneratedColumn id = GeneratedColumn( + 'id', + aliasedName, + false, + hasAutoIncrement: true, + type: DriftSqlType.int, + requiredDuringInsert: false, + defaultConstraints: GeneratedColumn.constraintIsAlways( + 'PRIMARY KEY AUTOINCREMENT', + ), + ); + late final GeneratedColumn realmUrl = GeneratedColumn( + 'realm_url', + aliasedName, + false, + type: DriftSqlType.string, + requiredDuringInsert: true, + ); + late final GeneratedColumn userId = GeneratedColumn( + 'user_id', + aliasedName, + false, + type: DriftSqlType.int, + requiredDuringInsert: true, + ); + late final GeneratedColumn email = GeneratedColumn( + 'email', + aliasedName, + false, + type: DriftSqlType.string, + requiredDuringInsert: true, + ); + late final GeneratedColumn apiKey = GeneratedColumn( + 'api_key', + aliasedName, + false, + type: DriftSqlType.string, + requiredDuringInsert: true, + ); + late final GeneratedColumn zulipVersion = GeneratedColumn( + 'zulip_version', + aliasedName, + false, + type: DriftSqlType.string, + requiredDuringInsert: true, + ); + late final GeneratedColumn zulipMergeBase = GeneratedColumn( + 'zulip_merge_base', + aliasedName, + true, + type: DriftSqlType.string, + requiredDuringInsert: false, + ); + late final GeneratedColumn zulipFeatureLevel = GeneratedColumn( + 'zulip_feature_level', + aliasedName, + false, + type: DriftSqlType.int, + requiredDuringInsert: true, + ); + late final GeneratedColumn ackedPushToken = GeneratedColumn( + 'acked_push_token', + aliasedName, + true, + type: DriftSqlType.string, + requiredDuringInsert: false, + ); + @override + List get $columns => [ + id, + realmUrl, + userId, + email, + apiKey, + zulipVersion, + zulipMergeBase, + zulipFeatureLevel, + ackedPushToken, + ]; + @override + String get aliasedName => _alias ?? actualTableName; + @override + String get actualTableName => $name; + static const String $name = 'accounts'; + @override + Set get $primaryKey => {id}; + @override + List> get uniqueKeys => [ + {realmUrl, userId}, + {realmUrl, email}, + ]; + @override + AccountsData map(Map data, {String? tablePrefix}) { + final effectivePrefix = tablePrefix != null ? '$tablePrefix.' : ''; + return AccountsData( + id: + attachedDatabase.typeMapping.read( + DriftSqlType.int, + data['${effectivePrefix}id'], + )!, + realmUrl: + attachedDatabase.typeMapping.read( + DriftSqlType.string, + data['${effectivePrefix}realm_url'], + )!, + userId: + attachedDatabase.typeMapping.read( + DriftSqlType.int, + data['${effectivePrefix}user_id'], + )!, + email: + attachedDatabase.typeMapping.read( + DriftSqlType.string, + data['${effectivePrefix}email'], + )!, + apiKey: + attachedDatabase.typeMapping.read( + DriftSqlType.string, + data['${effectivePrefix}api_key'], + )!, + zulipVersion: + attachedDatabase.typeMapping.read( + DriftSqlType.string, + data['${effectivePrefix}zulip_version'], + )!, + zulipMergeBase: attachedDatabase.typeMapping.read( + DriftSqlType.string, + data['${effectivePrefix}zulip_merge_base'], + ), + zulipFeatureLevel: + attachedDatabase.typeMapping.read( + DriftSqlType.int, + data['${effectivePrefix}zulip_feature_level'], + )!, + ackedPushToken: attachedDatabase.typeMapping.read( + DriftSqlType.string, + data['${effectivePrefix}acked_push_token'], + ), + ); + } + + @override + Accounts createAlias(String alias) { + return Accounts(attachedDatabase, alias); + } +} + +class AccountsData extends DataClass implements Insertable { + final int id; + final String realmUrl; + final int userId; + final String email; + final String apiKey; + final String zulipVersion; + final String? zulipMergeBase; + final int zulipFeatureLevel; + final String? ackedPushToken; + const AccountsData({ + required this.id, + required this.realmUrl, + required this.userId, + required this.email, + required this.apiKey, + required this.zulipVersion, + this.zulipMergeBase, + required this.zulipFeatureLevel, + this.ackedPushToken, + }); + @override + Map toColumns(bool nullToAbsent) { + final map = {}; + map['id'] = Variable(id); + map['realm_url'] = Variable(realmUrl); + map['user_id'] = Variable(userId); + map['email'] = Variable(email); + map['api_key'] = Variable(apiKey); + map['zulip_version'] = Variable(zulipVersion); + if (!nullToAbsent || zulipMergeBase != null) { + map['zulip_merge_base'] = Variable(zulipMergeBase); + } + map['zulip_feature_level'] = Variable(zulipFeatureLevel); + if (!nullToAbsent || ackedPushToken != null) { + map['acked_push_token'] = Variable(ackedPushToken); + } + return map; + } + + AccountsCompanion toCompanion(bool nullToAbsent) { + return AccountsCompanion( + id: Value(id), + realmUrl: Value(realmUrl), + userId: Value(userId), + email: Value(email), + apiKey: Value(apiKey), + zulipVersion: Value(zulipVersion), + zulipMergeBase: + zulipMergeBase == null && nullToAbsent + ? const Value.absent() + : Value(zulipMergeBase), + zulipFeatureLevel: Value(zulipFeatureLevel), + ackedPushToken: + ackedPushToken == null && nullToAbsent + ? const Value.absent() + : Value(ackedPushToken), + ); + } + + factory AccountsData.fromJson( + Map json, { + ValueSerializer? serializer, + }) { + serializer ??= driftRuntimeOptions.defaultSerializer; + return AccountsData( + id: serializer.fromJson(json['id']), + realmUrl: serializer.fromJson(json['realmUrl']), + userId: serializer.fromJson(json['userId']), + email: serializer.fromJson(json['email']), + apiKey: serializer.fromJson(json['apiKey']), + zulipVersion: serializer.fromJson(json['zulipVersion']), + zulipMergeBase: serializer.fromJson(json['zulipMergeBase']), + zulipFeatureLevel: serializer.fromJson(json['zulipFeatureLevel']), + ackedPushToken: serializer.fromJson(json['ackedPushToken']), + ); + } + @override + Map toJson({ValueSerializer? serializer}) { + serializer ??= driftRuntimeOptions.defaultSerializer; + return { + 'id': serializer.toJson(id), + 'realmUrl': serializer.toJson(realmUrl), + 'userId': serializer.toJson(userId), + 'email': serializer.toJson(email), + 'apiKey': serializer.toJson(apiKey), + 'zulipVersion': serializer.toJson(zulipVersion), + 'zulipMergeBase': serializer.toJson(zulipMergeBase), + 'zulipFeatureLevel': serializer.toJson(zulipFeatureLevel), + 'ackedPushToken': serializer.toJson(ackedPushToken), + }; + } + + AccountsData copyWith({ + int? id, + String? realmUrl, + int? userId, + String? email, + String? apiKey, + String? zulipVersion, + Value zulipMergeBase = const Value.absent(), + int? zulipFeatureLevel, + Value ackedPushToken = const Value.absent(), + }) => AccountsData( + id: id ?? this.id, + realmUrl: realmUrl ?? this.realmUrl, + userId: userId ?? this.userId, + email: email ?? this.email, + apiKey: apiKey ?? this.apiKey, + zulipVersion: zulipVersion ?? this.zulipVersion, + zulipMergeBase: + zulipMergeBase.present ? zulipMergeBase.value : this.zulipMergeBase, + zulipFeatureLevel: zulipFeatureLevel ?? this.zulipFeatureLevel, + ackedPushToken: + ackedPushToken.present ? ackedPushToken.value : this.ackedPushToken, + ); + AccountsData copyWithCompanion(AccountsCompanion data) { + return AccountsData( + id: data.id.present ? data.id.value : this.id, + realmUrl: data.realmUrl.present ? data.realmUrl.value : this.realmUrl, + userId: data.userId.present ? data.userId.value : this.userId, + email: data.email.present ? data.email.value : this.email, + apiKey: data.apiKey.present ? data.apiKey.value : this.apiKey, + zulipVersion: + data.zulipVersion.present + ? data.zulipVersion.value + : this.zulipVersion, + zulipMergeBase: + data.zulipMergeBase.present + ? data.zulipMergeBase.value + : this.zulipMergeBase, + zulipFeatureLevel: + data.zulipFeatureLevel.present + ? data.zulipFeatureLevel.value + : this.zulipFeatureLevel, + ackedPushToken: + data.ackedPushToken.present + ? data.ackedPushToken.value + : this.ackedPushToken, + ); + } + + @override + String toString() { + return (StringBuffer('AccountsData(') + ..write('id: $id, ') + ..write('realmUrl: $realmUrl, ') + ..write('userId: $userId, ') + ..write('email: $email, ') + ..write('apiKey: $apiKey, ') + ..write('zulipVersion: $zulipVersion, ') + ..write('zulipMergeBase: $zulipMergeBase, ') + ..write('zulipFeatureLevel: $zulipFeatureLevel, ') + ..write('ackedPushToken: $ackedPushToken') + ..write(')')) + .toString(); + } + + @override + int get hashCode => Object.hash( + id, + realmUrl, + userId, + email, + apiKey, + zulipVersion, + zulipMergeBase, + zulipFeatureLevel, + ackedPushToken, + ); + @override + bool operator ==(Object other) => + identical(this, other) || + (other is AccountsData && + other.id == this.id && + other.realmUrl == this.realmUrl && + other.userId == this.userId && + other.email == this.email && + other.apiKey == this.apiKey && + other.zulipVersion == this.zulipVersion && + other.zulipMergeBase == this.zulipMergeBase && + other.zulipFeatureLevel == this.zulipFeatureLevel && + other.ackedPushToken == this.ackedPushToken); +} + +class AccountsCompanion extends UpdateCompanion { + final Value id; + final Value realmUrl; + final Value userId; + final Value email; + final Value apiKey; + final Value zulipVersion; + final Value zulipMergeBase; + final Value zulipFeatureLevel; + final Value ackedPushToken; + const AccountsCompanion({ + this.id = const Value.absent(), + this.realmUrl = const Value.absent(), + this.userId = const Value.absent(), + this.email = const Value.absent(), + this.apiKey = const Value.absent(), + this.zulipVersion = const Value.absent(), + this.zulipMergeBase = const Value.absent(), + this.zulipFeatureLevel = const Value.absent(), + this.ackedPushToken = const Value.absent(), + }); + AccountsCompanion.insert({ + this.id = const Value.absent(), + required String realmUrl, + required int userId, + required String email, + required String apiKey, + required String zulipVersion, + this.zulipMergeBase = const Value.absent(), + required int zulipFeatureLevel, + this.ackedPushToken = const Value.absent(), + }) : realmUrl = Value(realmUrl), + userId = Value(userId), + email = Value(email), + apiKey = Value(apiKey), + zulipVersion = Value(zulipVersion), + zulipFeatureLevel = Value(zulipFeatureLevel); + static Insertable custom({ + Expression? id, + Expression? realmUrl, + Expression? userId, + Expression? email, + Expression? apiKey, + Expression? zulipVersion, + Expression? zulipMergeBase, + Expression? zulipFeatureLevel, + Expression? ackedPushToken, + }) { + return RawValuesInsertable({ + if (id != null) 'id': id, + if (realmUrl != null) 'realm_url': realmUrl, + if (userId != null) 'user_id': userId, + if (email != null) 'email': email, + if (apiKey != null) 'api_key': apiKey, + if (zulipVersion != null) 'zulip_version': zulipVersion, + if (zulipMergeBase != null) 'zulip_merge_base': zulipMergeBase, + if (zulipFeatureLevel != null) 'zulip_feature_level': zulipFeatureLevel, + if (ackedPushToken != null) 'acked_push_token': ackedPushToken, + }); + } + + AccountsCompanion copyWith({ + Value? id, + Value? realmUrl, + Value? userId, + Value? email, + Value? apiKey, + Value? zulipVersion, + Value? zulipMergeBase, + Value? zulipFeatureLevel, + Value? ackedPushToken, + }) { + return AccountsCompanion( + id: id ?? this.id, + realmUrl: realmUrl ?? this.realmUrl, + userId: userId ?? this.userId, + email: email ?? this.email, + apiKey: apiKey ?? this.apiKey, + zulipVersion: zulipVersion ?? this.zulipVersion, + zulipMergeBase: zulipMergeBase ?? this.zulipMergeBase, + zulipFeatureLevel: zulipFeatureLevel ?? this.zulipFeatureLevel, + ackedPushToken: ackedPushToken ?? this.ackedPushToken, + ); + } + + @override + Map toColumns(bool nullToAbsent) { + final map = {}; + if (id.present) { + map['id'] = Variable(id.value); + } + if (realmUrl.present) { + map['realm_url'] = Variable(realmUrl.value); + } + if (userId.present) { + map['user_id'] = Variable(userId.value); + } + if (email.present) { + map['email'] = Variable(email.value); + } + if (apiKey.present) { + map['api_key'] = Variable(apiKey.value); + } + if (zulipVersion.present) { + map['zulip_version'] = Variable(zulipVersion.value); + } + if (zulipMergeBase.present) { + map['zulip_merge_base'] = Variable(zulipMergeBase.value); + } + if (zulipFeatureLevel.present) { + map['zulip_feature_level'] = Variable(zulipFeatureLevel.value); + } + if (ackedPushToken.present) { + map['acked_push_token'] = Variable(ackedPushToken.value); + } + return map; + } + + @override + String toString() { + return (StringBuffer('AccountsCompanion(') + ..write('id: $id, ') + ..write('realmUrl: $realmUrl, ') + ..write('userId: $userId, ') + ..write('email: $email, ') + ..write('apiKey: $apiKey, ') + ..write('zulipVersion: $zulipVersion, ') + ..write('zulipMergeBase: $zulipMergeBase, ') + ..write('zulipFeatureLevel: $zulipFeatureLevel, ') + ..write('ackedPushToken: $ackedPushToken') + ..write(')')) + .toString(); + } +} + +class DatabaseAtV4 extends GeneratedDatabase { + DatabaseAtV4(QueryExecutor e) : super(e); + late final GlobalSettings globalSettings = GlobalSettings(this); + late final Accounts accounts = Accounts(this); + @override + Iterable> get allTables => + allSchemaEntities.whereType>(); + @override + List get allSchemaEntities => [ + globalSettings, + accounts, + ]; + @override + int get schemaVersion => 4; +} diff --git a/test/model/store_checks.dart b/test/model/store_checks.dart index 5b05935572..8187fc220d 100644 --- a/test/model/store_checks.dart +++ b/test/model/store_checks.dart @@ -31,6 +31,7 @@ extension GlobalStoreChecks on Subject { extension GlobalSettingsDataChecks on Subject { Subject get themeSetting => has((x) => x.themeSetting, 'themeSetting'); + Subject get browserPreference => has((x) => x.browserPreference, 'browserPreference'); } extension PerAccountStoreChecks on Subject { From 8d67f40e736b9819073b89f281c67d7aeff50bf1 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Tue, 11 Mar 2025 20:11:20 -0400 Subject: [PATCH 2/4] setting [nfc]: Make in-app explicitly the browser preference on Android The url_launcher documentation warns that the meaning of platformDefault may change at any time. We've chosen particular behaviors we want on different platforms, so express those choices explicitly. This change is NFC given the current behavior of url_launcher, which appears to be that platformDefault means an in-app browser for at least HTTP links. Co-authored-by: Greg Price Signed-off-by: Zixuan James Li --- lib/model/settings.dart | 42 ++++++++++++++++++++++++++++++++++ lib/widgets/content.dart | 12 +++------- test/model/settings_test.dart | 29 +++++++++++++++++++++++ test/model/store_checks.dart | 3 +++ test/widgets/content_test.dart | 32 +++++++++++++------------- test/widgets/profile_test.dart | 4 ++-- 6 files changed, 95 insertions(+), 27 deletions(-) create mode 100644 test/model/settings_test.dart diff --git a/lib/model/settings.dart b/lib/model/settings.dart index 2a5262cee7..0ade840d14 100644 --- a/lib/model/settings.dart +++ b/lib/model/settings.dart @@ -1,4 +1,8 @@ +import 'package:flutter/foundation.dart'; + import '../generated/l10n/zulip_localizations.dart'; +import 'binding.dart'; +import 'database.dart'; /// The user's choice of visual theme for the app. /// @@ -31,8 +35,46 @@ enum ThemeSetting { /// Write a migration if such a change is necessary. enum BrowserPreference { /// Use the in-app browser for HTTP links. + /// + /// For other types of links (e.g. mailto) where a browser won't work, + /// this falls back to [UrlLaunchMode.platformDefault]. inApp, /// Use the user's default browser app. external, } + +extension GlobalSettingsHelpers on GlobalSettingsData { + BrowserPreference get defaultBrowserPreference { + return switch (defaultTargetPlatform) { + // On iOS we prefer UrlLaunchMode.externalApplication because (for + // HTTP URLs) UrlLaunchMode.platformDefault uses SFSafariViewController, + // which gives an awkward UX as described here: + // https://chat.zulip.org/#narrow/stream/48-mobile/topic/in-app.20browser/near/1169118 + TargetPlatform.iOS => BrowserPreference.external, + + // On Android we prefer an in-app browser. See discussion from 2021: + // https://chat.zulip.org/#narrow/channel/48-mobile/topic/in-app.20browser/near/1169095 + // That's also the `url_launcher` default (at least as of 2025). + _ => BrowserPreference.inApp, + }; + } + + UrlLaunchMode getUrlLaunchMode(Uri url) { + switch (defaultBrowserPreference) { + case BrowserPreference.inApp: + if (!(url.scheme == 'https' || url.scheme == 'http')) { + // For URLs on non-HTTP schemes such as `mailto`, + // `url_launcher.launchUrl` rejects `inAppBrowserView` with an error: + // https://github.com/flutter/packages/blob/9cc6f370/packages/url_launcher/url_launcher/lib/src/url_launcher_uri.dart#L46-L51 + // TODO(upstream/url_launcher): should fall back in this case instead + // (as the `launchUrl` doc already says it may do). + return UrlLaunchMode.platformDefault; + } + return UrlLaunchMode.inAppBrowserView; + + case BrowserPreference.external: + return UrlLaunchMode.externalApplication; + } + } +} diff --git a/lib/widgets/content.dart b/lib/widgets/content.dart index 55b5331c3c..afafd03e8c 100644 --- a/lib/widgets/content.dart +++ b/lib/widgets/content.dart @@ -1,7 +1,6 @@ import 'dart:async'; import 'package:flutter/cupertino.dart'; -import 'package:flutter/foundation.dart'; import 'package:flutter/gestures.dart'; import 'package:flutter/material.dart'; import 'package:flutter/rendering.dart'; @@ -16,6 +15,7 @@ import '../model/avatar_url.dart'; import '../model/binding.dart'; import '../model/content.dart'; import '../model/internal_link.dart'; +import '../model/settings.dart'; import 'code_block.dart'; import 'dialog.dart'; import 'icons.dart'; @@ -1439,18 +1439,12 @@ void _launchUrl(BuildContext context, String urlString) async { return; } + final globalSettings = GlobalStoreWidget.of(context).globalSettings; bool launched = false; String? errorMessage; try { launched = await ZulipBinding.instance.launchUrl(url, - mode: switch (defaultTargetPlatform) { - // On iOS we prefer LaunchMode.externalApplication because (for - // HTTP URLs) LaunchMode.platformDefault uses SFSafariViewController, - // which gives an awkward UX as described here: - // https://chat.zulip.org/#narrow/stream/48-mobile/topic/in-app.20browser/near/1169118 - TargetPlatform.iOS => UrlLaunchMode.externalApplication, - _ => UrlLaunchMode.platformDefault, - }); + mode: globalSettings.getUrlLaunchMode(url)); } on PlatformException catch (e) { errorMessage = e.message; } diff --git a/test/model/settings_test.dart b/test/model/settings_test.dart new file mode 100644 index 0000000000..721fd68be1 --- /dev/null +++ b/test/model/settings_test.dart @@ -0,0 +1,29 @@ +import 'package:checks/checks.dart'; +import 'package:flutter/foundation.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:zulip/model/binding.dart'; + +import '../example_data.dart' as eg; +import 'store_checks.dart'; +import 'store_test.dart'; + +void main() { + final httpLink = Uri.parse('http://chat.zulip.org'); + final nonHttpLink = Uri.parse('mailto:chat@zulip.org'); + + group('getUrlLaunchMode', () { + testAndroidIos('use our per-platform defaults for HTTP links', () { + final globalStore = eg.globalStore(globalSettings: eg.globalSettings()); + check(globalStore).globalSettings.getUrlLaunchMode(httpLink).equals( + defaultTargetPlatform == TargetPlatform.android + ? UrlLaunchMode.inAppBrowserView : UrlLaunchMode.externalApplication); + }); + + testAndroidIos('use our per-platform defaults for non-HTTP links', () { + final globalStore = eg.globalStore(globalSettings: eg.globalSettings()); + check(globalStore).globalSettings.getUrlLaunchMode(nonHttpLink).equals( + defaultTargetPlatform == TargetPlatform.android + ? UrlLaunchMode.platformDefault : UrlLaunchMode.externalApplication); + }); + }); +} diff --git a/test/model/store_checks.dart b/test/model/store_checks.dart index 8187fc220d..1c521cd11e 100644 --- a/test/model/store_checks.dart +++ b/test/model/store_checks.dart @@ -3,6 +3,7 @@ import 'package:zulip/api/core.dart'; import 'package:zulip/api/model/initial_snapshot.dart'; import 'package:zulip/api/model/model.dart'; import 'package:zulip/model/autocomplete.dart'; +import 'package:zulip/model/binding.dart'; import 'package:zulip/model/database.dart'; import 'package:zulip/model/recent_dm_conversations.dart'; import 'package:zulip/model/settings.dart'; @@ -32,6 +33,8 @@ extension GlobalStoreChecks on Subject { extension GlobalSettingsDataChecks on Subject { Subject get themeSetting => has((x) => x.themeSetting, 'themeSetting'); Subject get browserPreference => has((x) => x.browserPreference, 'browserPreference'); + Subject get defaultBrowserPreference => has((x) => x.defaultBrowserPreference, 'defaultBrowserPreference'); + Subject getUrlLaunchMode(Uri url) => has((x) => x.getUrlLaunchMode(url), 'getUrlLaunchMode'); } extension PerAccountStoreChecks on Subject { diff --git a/test/widgets/content_test.dart b/test/widgets/content_test.dart index 88f94a002e..d958a92d1b 100644 --- a/test/widgets/content_test.dart +++ b/test/widgets/content_test.dart @@ -521,7 +521,7 @@ void main() { final expectedLaunchUrl = expectedVideo.hrefUrl; await tester.tap(find.byIcon(Icons.play_arrow_rounded)); check(testBinding.takeLaunchUrlCalls()) - .single.equals((url: Uri.parse(expectedLaunchUrl), mode: LaunchMode.platformDefault)); + .single.equals((url: Uri.parse(expectedLaunchUrl), mode: LaunchMode.inAppBrowserView)); } testWidgets('video preview for youtube embed', (tester) async { @@ -793,7 +793,7 @@ void main() { await tapText(tester, find.text('hello')); final expectedLaunchMode = defaultTargetPlatform == TargetPlatform.iOS ? - LaunchMode.externalApplication : LaunchMode.platformDefault; + LaunchMode.externalApplication : LaunchMode.inAppBrowserView; check(testBinding.takeLaunchUrlCalls()) .single.equals((url: Uri.parse('https://example/'), mode: expectedLaunchMode)); }, variant: const TargetPlatformVariant({TargetPlatform.android, TargetPlatform.iOS})); @@ -811,11 +811,11 @@ void main() { await tester.tapAt(base.translate(1*fontSize, 0)); // "fXo bar baz" check(testBinding.takeLaunchUrlCalls()) - .single.equals((url: Uri.parse('https://a/'), mode: LaunchMode.platformDefault)); + .single.equals((url: Uri.parse('https://a/'), mode: LaunchMode.inAppBrowserView)); await tester.tapAt(base.translate(9*fontSize, 0)); // "foo bar bXz" check(testBinding.takeLaunchUrlCalls()) - .single.equals((url: Uri.parse('https://b/'), mode: LaunchMode.platformDefault)); + .single.equals((url: Uri.parse('https://b/'), mode: LaunchMode.inAppBrowserView)); }); testWidgets('link nested in other spans', (tester) async { @@ -823,7 +823,7 @@ void main() { '

word

'); await tapText(tester, find.text('word')); check(testBinding.takeLaunchUrlCalls()) - .single.equals((url: Uri.parse('https://a/'), mode: LaunchMode.platformDefault)); + .single.equals((url: Uri.parse('https://a/'), mode: LaunchMode.inAppBrowserView)); }); testWidgets('link containing other spans', (tester) async { @@ -836,11 +836,11 @@ void main() { await tester.tapAt(base.translate(1*fontSize, 0)); // "tXo words" check(testBinding.takeLaunchUrlCalls()) - .single.equals((url: Uri.parse('https://a/'), mode: LaunchMode.platformDefault)); + .single.equals((url: Uri.parse('https://a/'), mode: LaunchMode.inAppBrowserView)); await tester.tapAt(base.translate(6*fontSize, 0)); // "two woXds" check(testBinding.takeLaunchUrlCalls()) - .single.equals((url: Uri.parse('https://a/'), mode: LaunchMode.platformDefault)); + .single.equals((url: Uri.parse('https://a/'), mode: LaunchMode.inAppBrowserView)); }); testWidgets('relative links are resolved', (tester) async { @@ -848,7 +848,7 @@ void main() { '

word

'); await tapText(tester, find.text('word')); check(testBinding.takeLaunchUrlCalls()) - .single.equals((url: Uri.parse('${eg.realmUrl}a/b?c#d'), mode: LaunchMode.platformDefault)); + .single.equals((url: Uri.parse('${eg.realmUrl}a/b?c#d'), mode: LaunchMode.inAppBrowserView)); }); testWidgets('link inside HeadingNode', (tester) async { @@ -856,7 +856,7 @@ void main() { '
word
'); await tapText(tester, find.text('word')); check(testBinding.takeLaunchUrlCalls()) - .single.equals((url: Uri.parse('https://a/'), mode: LaunchMode.platformDefault)); + .single.equals((url: Uri.parse('https://a/'), mode: LaunchMode.inAppBrowserView)); }); testWidgets('error dialog if invalid link', (tester) async { @@ -910,7 +910,7 @@ void main() { await tapText(tester, find.text('invalid')); final expectedUrl = eg.realmUrl.resolve('/#narrow/stream/1-check/topic'); check(testBinding.takeLaunchUrlCalls()) - .single.equals((url: expectedUrl, mode: LaunchMode.platformDefault)); + .single.equals((url: expectedUrl, mode: LaunchMode.inAppBrowserView)); check(pushedRoutes).isEmpty(); }); }); @@ -1060,11 +1060,11 @@ void main() { await tester.tap(find.text('Zulip — organized team chat')); check(testBinding.takeLaunchUrlCalls()) - .single.equals((url: url, mode: LaunchMode.platformDefault)); + .single.equals((url: url, mode: LaunchMode.inAppBrowserView)); await tester.tap(find.byType(RealmContentNetworkImage)); check(testBinding.takeLaunchUrlCalls()) - .single.equals((url: url, mode: LaunchMode.platformDefault)); + .single.equals((url: url, mode: LaunchMode.inAppBrowserView)); debugNetworkImageHttpClientProvider = null; }); @@ -1078,7 +1078,7 @@ void main() { await tester.tap(find.byType(RealmContentNetworkImage)); check(testBinding.takeLaunchUrlCalls()) - .single.equals((url: url, mode: LaunchMode.platformDefault)); + .single.equals((url: url, mode: LaunchMode.inAppBrowserView)); debugNetworkImageHttpClientProvider = null; }); @@ -1088,11 +1088,11 @@ void main() { await tester.tap(find.text('Zulip — organized team chat')); check(testBinding.takeLaunchUrlCalls()) - .single.equals((url: url, mode: LaunchMode.platformDefault)); + .single.equals((url: url, mode: LaunchMode.inAppBrowserView)); await tester.tap(find.byType(RealmContentNetworkImage)); check(testBinding.takeLaunchUrlCalls()) - .single.equals((url: url, mode: LaunchMode.platformDefault)); + .single.equals((url: url, mode: LaunchMode.inAppBrowserView)); debugNetworkImageHttpClientProvider = null; }); @@ -1102,7 +1102,7 @@ void main() { await tester.tap(find.byType(RealmContentNetworkImage)); check(testBinding.takeLaunchUrlCalls()) - .single.equals((url: url, mode: LaunchMode.platformDefault)); + .single.equals((url: url, mode: LaunchMode.inAppBrowserView)); debugNetworkImageHttpClientProvider = null; }); }); diff --git a/test/widgets/profile_test.dart b/test/widgets/profile_test.dart index 38f6c223e3..30f6433528 100644 --- a/test/widgets/profile_test.dart +++ b/test/widgets/profile_test.dart @@ -164,7 +164,7 @@ void main() { await tester.tap(find.text(testUrl)); check(testBinding.takeLaunchUrlCalls()).single.equals(( url: Uri.parse(testUrl), - mode: LaunchMode.platformDefault, + mode: LaunchMode.inAppBrowserView, )); }); @@ -191,7 +191,7 @@ void main() { await tester.tap(find.text('externalValue')); check(testBinding.takeLaunchUrlCalls()).single.equals(( url: Uri.parse('http://example/externalValue'), - mode: LaunchMode.platformDefault, + mode: LaunchMode.inAppBrowserView, )); }); From 8901b79680a8f26cbce8eb8b4b8e38c79227190a Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Thu, 20 Feb 2025 14:58:29 -0500 Subject: [PATCH 3/4] content [nfc]: Follow browser preference setting when opening links This preserves the previous behavior (external application on iOS, and in-app browser on Android) when browserPreference is `null`. There is no way for the client to update the setting for now, hence the NFC. The helper could have been a static method instead of an extension, similar to ThemeSetting.displayName, but that will be a lot more verbose for the caller. Signed-off-by: Zixuan James Li --- lib/model/settings.dart | 5 +++-- test/model/settings_test.dart | 32 ++++++++++++++++++++++++++++---- test/model/store_checks.dart | 2 +- test/widgets/content_test.dart | 13 +++++++++++++ 4 files changed, 45 insertions(+), 7 deletions(-) diff --git a/lib/model/settings.dart b/lib/model/settings.dart index 0ade840d14..928eb0e1cd 100644 --- a/lib/model/settings.dart +++ b/lib/model/settings.dart @@ -45,7 +45,8 @@ enum BrowserPreference { } extension GlobalSettingsHelpers on GlobalSettingsData { - BrowserPreference get defaultBrowserPreference { + BrowserPreference get effectiveBrowserPreference { + if (browserPreference != null) return browserPreference!; return switch (defaultTargetPlatform) { // On iOS we prefer UrlLaunchMode.externalApplication because (for // HTTP URLs) UrlLaunchMode.platformDefault uses SFSafariViewController, @@ -61,7 +62,7 @@ extension GlobalSettingsHelpers on GlobalSettingsData { } UrlLaunchMode getUrlLaunchMode(Uri url) { - switch (defaultBrowserPreference) { + switch (effectiveBrowserPreference) { case BrowserPreference.inApp: if (!(url.scheme == 'https' || url.scheme == 'http')) { // For URLs on non-HTTP schemes such as `mailto`, diff --git a/test/model/settings_test.dart b/test/model/settings_test.dart index 721fd68be1..e4e0def398 100644 --- a/test/model/settings_test.dart +++ b/test/model/settings_test.dart @@ -2,6 +2,7 @@ import 'package:checks/checks.dart'; import 'package:flutter/foundation.dart'; import 'package:flutter_test/flutter_test.dart'; import 'package:zulip/model/binding.dart'; +import 'package:zulip/model/settings.dart'; import '../example_data.dart' as eg; import 'store_checks.dart'; @@ -12,18 +13,41 @@ void main() { final nonHttpLink = Uri.parse('mailto:chat@zulip.org'); group('getUrlLaunchMode', () { - testAndroidIos('use our per-platform defaults for HTTP links', () { - final globalStore = eg.globalStore(globalSettings: eg.globalSettings()); + testAndroidIos('globalSettings.browserPreference is null; use our per-platform defaults for HTTP links', () { + final globalStore = eg.globalStore(globalSettings: eg.globalSettings( + browserPreference: null)); check(globalStore).globalSettings.getUrlLaunchMode(httpLink).equals( defaultTargetPlatform == TargetPlatform.android ? UrlLaunchMode.inAppBrowserView : UrlLaunchMode.externalApplication); }); - testAndroidIos('use our per-platform defaults for non-HTTP links', () { - final globalStore = eg.globalStore(globalSettings: eg.globalSettings()); + testAndroidIos('globalSettings.browserPreference is null; use our per-platform defaults for non-HTTP links', () { + final globalStore = eg.globalStore(globalSettings: eg.globalSettings( + browserPreference: null)); check(globalStore).globalSettings.getUrlLaunchMode(nonHttpLink).equals( defaultTargetPlatform == TargetPlatform.android ? UrlLaunchMode.platformDefault : UrlLaunchMode.externalApplication); }); + + testAndroidIos('globalSettings.browserPreference is inApp; follow the user preference for http links', () { + final globalStore = eg.globalStore(globalSettings: eg.globalSettings( + browserPreference: BrowserPreference.inApp)); + check(globalStore).globalSettings.getUrlLaunchMode(httpLink).equals( + UrlLaunchMode.inAppBrowserView); + }); + + testAndroidIos('globalSettings.browserPreference is inApp; use platform default for non-http links', () { + final globalStore = eg.globalStore(globalSettings: eg.globalSettings( + browserPreference: BrowserPreference.inApp)); + check(globalStore).globalSettings.getUrlLaunchMode(nonHttpLink).equals( + UrlLaunchMode.platformDefault); + }); + + testAndroidIos('globalSettings.browserPreference is external; follow the user preference', () { + final globalStore = eg.globalStore(globalSettings: eg.globalSettings( + browserPreference: BrowserPreference.external)); + check(globalStore).globalSettings.getUrlLaunchMode(httpLink).equals( + UrlLaunchMode.externalApplication); + }); }); } diff --git a/test/model/store_checks.dart b/test/model/store_checks.dart index 1c521cd11e..96edf05d41 100644 --- a/test/model/store_checks.dart +++ b/test/model/store_checks.dart @@ -33,7 +33,7 @@ extension GlobalStoreChecks on Subject { extension GlobalSettingsDataChecks on Subject { Subject get themeSetting => has((x) => x.themeSetting, 'themeSetting'); Subject get browserPreference => has((x) => x.browserPreference, 'browserPreference'); - Subject get defaultBrowserPreference => has((x) => x.defaultBrowserPreference, 'defaultBrowserPreference'); + Subject get effectiveBrowserPreference => has((x) => x.effectiveBrowserPreference, 'effectiveBrowserPreference'); Subject getUrlLaunchMode(Uri url) => has((x) => x.getUrlLaunchMode(url), 'getUrlLaunchMode'); } diff --git a/test/widgets/content_test.dart b/test/widgets/content_test.dart index d958a92d1b..c2bb5fee4f 100644 --- a/test/widgets/content_test.dart +++ b/test/widgets/content_test.dart @@ -9,6 +9,7 @@ import 'package:url_launcher/url_launcher.dart'; import 'package:zulip/api/core.dart'; import 'package:zulip/model/content.dart'; import 'package:zulip/model/narrow.dart'; +import 'package:zulip/model/settings.dart'; import 'package:zulip/model/store.dart'; import 'package:zulip/widgets/content.dart'; import 'package:zulip/widgets/icons.dart'; @@ -798,6 +799,18 @@ void main() { .single.equals((url: Uri.parse('https://example/'), mode: expectedLaunchMode)); }, variant: const TargetPlatformVariant({TargetPlatform.android, TargetPlatform.iOS})); + testWidgets('follow browser preference setting to open URL', (tester) async { + await testBinding.globalStore.updateGlobalSettings( + eg.globalSettings( + browserPreference: BrowserPreference.inApp).toCompanion(false)); + await prepare(tester, + '

hello

'); + + await tapText(tester, find.text('hello')); + check(testBinding.takeLaunchUrlCalls()).single.equals(( + url: Uri.parse('https://example/'), mode: LaunchMode.inAppBrowserView)); + }, variant: const TargetPlatformVariant({TargetPlatform.android, TargetPlatform.iOS})); + testWidgets('multiple links in paragraph', (tester) async { const fontSize = kBaseFontSize; From b0307378c4b513d3fe320259be34ab61df49dfa6 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Thu, 20 Feb 2025 14:59:25 -0500 Subject: [PATCH 4/4] settings: Add browser preference setting ui The UI currently does not have a design, and is made to be as simple as possible to implement for now. Fixes: #1228 Signed-off-by: Zixuan James Li --- assets/l10n/app_en.arb | 4 ++ lib/generated/l10n/zulip_localizations.dart | 6 +++ .../l10n/zulip_localizations_ar.dart | 3 ++ .../l10n/zulip_localizations_en.dart | 3 ++ .../l10n/zulip_localizations_ja.dart | 3 ++ .../l10n/zulip_localizations_nb.dart | 3 ++ .../l10n/zulip_localizations_pl.dart | 3 ++ .../l10n/zulip_localizations_ru.dart | 3 ++ .../l10n/zulip_localizations_sk.dart | 3 ++ lib/widgets/settings.dart | 24 ++++++++++ test/flutter_checks.dart | 4 ++ test/widgets/settings_test.dart | 45 +++++++++++++++++++ 12 files changed, 104 insertions(+) diff --git a/assets/l10n/app_en.arb b/assets/l10n/app_en.arb index 7f37a00dad..1973f5fab7 100644 --- a/assets/l10n/app_en.arb +++ b/assets/l10n/app_en.arb @@ -811,6 +811,10 @@ "@themeSettingSystem": { "description": "Label for system theme setting." }, + "openLinksWithInAppBrowser": "Open links with in-app browser", + "@openLinksWithInAppBrowser": { + "description": "Label for toggling setting to open links with in-app browser" + }, "pollWidgetQuestionMissing": "No question.", "@pollWidgetQuestionMissing": { "description": "Text to display for a poll when the question is missing" diff --git a/lib/generated/l10n/zulip_localizations.dart b/lib/generated/l10n/zulip_localizations.dart index 2f03458e05..d4bdd2db47 100644 --- a/lib/generated/l10n/zulip_localizations.dart +++ b/lib/generated/l10n/zulip_localizations.dart @@ -1185,6 +1185,12 @@ abstract class ZulipLocalizations { /// **'System'** String get themeSettingSystem; + /// Label for toggling setting to open links with in-app browser + /// + /// In en, this message translates to: + /// **'Open links with in-app browser'** + String get openLinksWithInAppBrowser; + /// Text to display for a poll when the question is missing /// /// In en, this message translates to: diff --git a/lib/generated/l10n/zulip_localizations_ar.dart b/lib/generated/l10n/zulip_localizations_ar.dart index fd7905924b..5d76c0bfae 100644 --- a/lib/generated/l10n/zulip_localizations_ar.dart +++ b/lib/generated/l10n/zulip_localizations_ar.dart @@ -634,6 +634,9 @@ class ZulipLocalizationsAr extends ZulipLocalizations { @override String get themeSettingSystem => 'System'; + @override + String get openLinksWithInAppBrowser => 'Open links with in-app browser'; + @override String get pollWidgetQuestionMissing => 'No question.'; diff --git a/lib/generated/l10n/zulip_localizations_en.dart b/lib/generated/l10n/zulip_localizations_en.dart index 1d19cfc7b0..17df08bfd5 100644 --- a/lib/generated/l10n/zulip_localizations_en.dart +++ b/lib/generated/l10n/zulip_localizations_en.dart @@ -634,6 +634,9 @@ class ZulipLocalizationsEn extends ZulipLocalizations { @override String get themeSettingSystem => 'System'; + @override + String get openLinksWithInAppBrowser => 'Open links with in-app browser'; + @override String get pollWidgetQuestionMissing => 'No question.'; diff --git a/lib/generated/l10n/zulip_localizations_ja.dart b/lib/generated/l10n/zulip_localizations_ja.dart index 58a4a1de59..9baef2dbae 100644 --- a/lib/generated/l10n/zulip_localizations_ja.dart +++ b/lib/generated/l10n/zulip_localizations_ja.dart @@ -634,6 +634,9 @@ class ZulipLocalizationsJa extends ZulipLocalizations { @override String get themeSettingSystem => 'System'; + @override + String get openLinksWithInAppBrowser => 'Open links with in-app browser'; + @override String get pollWidgetQuestionMissing => 'No question.'; diff --git a/lib/generated/l10n/zulip_localizations_nb.dart b/lib/generated/l10n/zulip_localizations_nb.dart index a5bba71bd1..fb3ca10833 100644 --- a/lib/generated/l10n/zulip_localizations_nb.dart +++ b/lib/generated/l10n/zulip_localizations_nb.dart @@ -634,6 +634,9 @@ class ZulipLocalizationsNb extends ZulipLocalizations { @override String get themeSettingSystem => 'System'; + @override + String get openLinksWithInAppBrowser => 'Open links with in-app browser'; + @override String get pollWidgetQuestionMissing => 'No question.'; diff --git a/lib/generated/l10n/zulip_localizations_pl.dart b/lib/generated/l10n/zulip_localizations_pl.dart index e78e60dcb2..45ad091f43 100644 --- a/lib/generated/l10n/zulip_localizations_pl.dart +++ b/lib/generated/l10n/zulip_localizations_pl.dart @@ -634,6 +634,9 @@ class ZulipLocalizationsPl extends ZulipLocalizations { @override String get themeSettingSystem => 'System'; + @override + String get openLinksWithInAppBrowser => 'Open links with in-app browser'; + @override String get pollWidgetQuestionMissing => 'Brak pytania.'; diff --git a/lib/generated/l10n/zulip_localizations_ru.dart b/lib/generated/l10n/zulip_localizations_ru.dart index e4bd460f72..946fe8c231 100644 --- a/lib/generated/l10n/zulip_localizations_ru.dart +++ b/lib/generated/l10n/zulip_localizations_ru.dart @@ -634,6 +634,9 @@ class ZulipLocalizationsRu extends ZulipLocalizations { @override String get themeSettingSystem => 'System'; + @override + String get openLinksWithInAppBrowser => 'Open links with in-app browser'; + @override String get pollWidgetQuestionMissing => 'Нет вопроса.'; diff --git a/lib/generated/l10n/zulip_localizations_sk.dart b/lib/generated/l10n/zulip_localizations_sk.dart index 9ec68077ae..90d71704f9 100644 --- a/lib/generated/l10n/zulip_localizations_sk.dart +++ b/lib/generated/l10n/zulip_localizations_sk.dart @@ -634,6 +634,9 @@ class ZulipLocalizationsSk extends ZulipLocalizations { @override String get themeSettingSystem => 'System'; + @override + String get openLinksWithInAppBrowser => 'Open links with in-app browser'; + @override String get pollWidgetQuestionMissing => 'Bez otázky.'; diff --git a/lib/widgets/settings.dart b/lib/widgets/settings.dart index 8ece4d0578..7777759c4f 100644 --- a/lib/widgets/settings.dart +++ b/lib/widgets/settings.dart @@ -24,6 +24,7 @@ class SettingsPage extends StatelessWidget { title: Text(zulipLocalizations.settingsPageTitle)), body: Column(children: [ const _ThemeSetting(), + const _BrowserPreferenceSetting(), ])); } } @@ -54,3 +55,26 @@ class _ThemeSetting extends StatelessWidget { ]); } } + +class _BrowserPreferenceSetting extends StatelessWidget { + const _BrowserPreferenceSetting(); + + void _handleChange(BuildContext context, bool newOpenLinksWithInAppBrowser) { + GlobalStoreWidget.of(context).updateGlobalSettings( + GlobalSettingsCompanion(browserPreference: Value( + newOpenLinksWithInAppBrowser ? BrowserPreference.inApp + : BrowserPreference.external))); + } + + @override + Widget build(BuildContext context) { + final zulipLocalizations = ZulipLocalizations.of(context); + final openLinksWithInAppBrowser = + GlobalStoreWidget.of(context).globalSettings.effectiveBrowserPreference + == BrowserPreference.inApp; + return SwitchListTile.adaptive( + title: Text(zulipLocalizations.openLinksWithInAppBrowser), + value: openLinksWithInAppBrowser, + onChanged: (newValue) => _handleChange(context, newValue)); + } +} diff --git a/test/flutter_checks.dart b/test/flutter_checks.dart index 2f3eac042f..49e402d158 100644 --- a/test/flutter_checks.dart +++ b/test/flutter_checks.dart @@ -176,6 +176,10 @@ extension RadioListTileChecks on Subject> { Subject get checked => has((x) => x.checked, 'checked'); } +extension SwitchListTileChecks on Subject { + Subject get value => has((x) => x.value, 'value'); +} + extension ThemeDataChecks on Subject { Subject get brightness => has((x) => x.brightness, 'brightness'); } diff --git a/test/widgets/settings_test.dart b/test/widgets/settings_test.dart index 2eb59d2c14..65336479eb 100644 --- a/test/widgets/settings_test.dart +++ b/test/widgets/settings_test.dart @@ -83,4 +83,49 @@ void main() { debugBrightnessOverride = null; }); }); + + group('BrowserPreference', () { + Finder useInAppBrowserSwitchFinder = find.ancestor( + of: find.text('Open links with in-app browser'), + matching: find.byType(SwitchListTile)); + + void checkSwitchAndGlobalSettings(WidgetTester tester, { + required bool checked, + required BrowserPreference? expectedBrowserPreference, + }) { + check(tester.widget(useInAppBrowserSwitchFinder)) + .value.equals(checked); + check(testBinding.globalStore) + .globalSettings.browserPreference.equals(expectedBrowserPreference); + } + + testWidgets('smoke', (tester) async { + await testBinding.globalStore.updateGlobalSettings( + eg.globalSettings( + browserPreference: BrowserPreference.external).toCompanion(false)); + await prepare(tester); + checkSwitchAndGlobalSettings(tester, + checked: false, expectedBrowserPreference: BrowserPreference.external); + + await tester.tap(useInAppBrowserSwitchFinder); + await tester.pump(); + checkSwitchAndGlobalSettings(tester, + checked: true, expectedBrowserPreference: BrowserPreference.inApp); + }); + + testWidgets('use our per-platform default browser preference', (tester) async { + await prepare(tester); + bool expectInApp = defaultTargetPlatform == TargetPlatform.android; + checkSwitchAndGlobalSettings(tester, + checked: expectInApp, expectedBrowserPreference: null); + + await tester.tap(useInAppBrowserSwitchFinder); + await tester.pump(); + expectInApp = !expectInApp; + checkSwitchAndGlobalSettings(tester, + checked: expectInApp, + expectedBrowserPreference: expectInApp + ? BrowserPreference.inApp : BrowserPreference.external); + }, variant: TargetPlatformVariant({TargetPlatform.android, TargetPlatform.iOS})); + }); }