Support some client settings; add settings page #1167

PIG208 · 2024-12-17T00:26:09Z

Work towards #97.

screenshots

gnprice · 2024-12-17T18:58:01Z

Thanks for taking this on! Looking forward to it.

One first high-level comment: in the branch we merge, let's have at least one of these two settings get added as its own separate commit, complete with a migration. That way we have a nice demonstration of what it'll look like to add each new setting in the future.

gnprice

I've skimmed now through more of this code. A few other high-level comments below.

gnprice · 2024-12-19T04:21:51Z

lib/model/database.dart

+            from2To3: (m, schema) async {
+              await m.createTable(schema.globalSettings);
+            },
+            from3To4: (m, schema) async {
+              await m.addColumn(
+                schema.globalSettings, schema.globalSettings.browserPreference);


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.

gnprice · 2024-12-19T04:22:56Z

lib/model/schema_versions.dart

+import 'package:drift/drift.dart' as i1;
+import 'package:drift/drift.dart'; // ignore_for_file: type=lint,unused_import
+
+// GENERATED BY drift_dev, DO NOT MODIFY.


Let's give this a .g.dart name, so it's clear in a more uniform way that it's generated.

gnprice · 2024-12-19T04:23:37Z

analysis_options.yaml

+    # Generated code from drift can contain unused local variables.
+    - lib/model/database.g.dart


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.

Filed simolus3/drift#3384 for that.

That issue has been marked as fixed! 🎉 So can we drop this commit?

I hope that there will be a release with this patch soon (last one was 2 weeks ago). If not we can just ask politely.

Seems like a good time to ask Simon nicely about when he anticipates making the next release. It looks like he's not doing the thing where every bugfix prompts an immediate release.

If a release doesn't come out shortly, let's just have this branch upgrade our drift_dev to a version from Git, namely the commit that fixed this small bug.

gnprice · 2024-12-19T04:34:56Z

lib/model/database.dart

+/// 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 {


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.

chrisbobbe

This is exciting!

One question: what UI text does zulip-mobile use for the browser setting? Would it make sense to copy that?

chrisbobbe · 2025-02-22T01:25:30Z

analysis_options.yaml

+    # Generated code from drift can contain unused local variables.
+    - lib/model/database.g.dart


That issue has been marked as fixed! 🎉 So can we drop this commit?

chrisbobbe · 2025-02-22T01:32:54Z

test/widgets/content_test.dart

-        .single.equals((url: Uri.parse(expectedLaunchUrl), mode: LaunchMode.platformDefault));
+        .single.equals((url: Uri.parse(expectedLaunchUrl), mode: LaunchMode.inAppBrowserView));


db: Store browser preference in database

I think these test changes are meant for a later commit:

content: Follow browser preference setting when opening links

(caught with git rebase --exec 'tools/check --diff @~')

chrisbobbe · 2025-02-22T01:35:29Z

lib/model/binding.dart

@@ -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.


store [nfc]: Update outdated references to loadGlobalStore

commit-message nit: truncate commit ID to 9 characters

chrisbobbe · 2025-02-22T01:55:44Z

lib/widgets/app.dart

+            // The context has to be taken from the [Builder] because
+            // [zulipThemeData] requires access to [GlobalStoreWidget] in the tree.
+            // TODO: any way to remove the [Builder], like how we pulled out
+            //   [_handleGenerateInitialRoutes]?
+            theme: zulipThemeData(context),


I guess we could have GlobalStoreWidget take a buildChild param instead of child (or support passing one or the other 🤷‍♂️), and it would pass its own context to buildChild?

Chose to name it builder to match Builder.builder and refactored places that use GlobalStoreWidget (one in lib and a handful in tests).

chrisbobbe · 2025-02-22T02:04:42Z

test/widgets/theme_test.dart

@@ -97,6 +100,27 @@ void main() {
        check(() => a.lerp(b, 0.5)).returnsNormally();
      });
    });
+
+    testWidgets('follow globalSettings.themeSetting if not unset', (tester) async {


follow globalSettings.themeSetting if not unset

I know what you mean by "if not unset", but it feels slightly unsatisfying: we do still want to follow globalSettings.themeSetting if it's unset 🙂, but the details are just a bit more complicated for that value.

I also see that this test exercises the "unset" value, but incompletely: with "unset", let's also test that the theme responds when the device setting is changed:

tester.platformDispatcher.platformBrightnessTestValue = Brightness.dark; await tester.pump(); check(zulipThemeData(element)).brightness.equals(Brightness.dark);

chrisbobbe · 2025-02-22T02:31:40Z

test/widgets/settings_test.dart

+      of: find.text('Use external browser'),
+      matching: find.byType(SwitchListTile));
+
+    testWidgets('smoke', (tester) async {


Let's also test that the switch is in the right place, and responds correctly, when the setting hasn't been set yet (the part meant by "effective" in effectiveBrowserPreference)

chrisbobbe · 2025-02-25T00:13:29Z

Oh, and we have an issue for the theme setting 🙂 so the PR description and relevant commit can be marked as fixing that:

ui: Allow selecting a different dark/light-theme setting than device setting #1216

PIG208 · 2025-02-25T00:16:11Z

There is also one for browser:

Offer "Open links with in-app browser" setting #1228

I didn't fix them because they do not implement a final design. Perhaps we can have a new design issue for the settings page.

chrisbobbe · 2025-02-25T00:23:20Z

I didn't fix them because they do not implement a final design. Perhaps we can have a new design issue for the settings page.

Sure, that sounds good: a new issue for redesigning the settings page as a whole. Then the individual issues, #1216 and #1228, don't need to stay open to track that redesign, and they can be marked as fixed in this PR; what do you think?

PIG208 · 2025-02-25T00:27:48Z

Opened #1375.

PIG208 · 2025-02-26T03:53:24Z

Forgot to post a message here. This PR is ready for review.

what UI text does zulip-mobile use for the browser setting? Would it make sense to copy that?

Good point! The string is "Open links with in-app browser"; switched to that instead.

chrisbobbe · 2025-03-03T21:37:27Z

Thanks, LGTM! Marking for Greg's review.

PIG208 · 2025-03-05T01:33:33Z

Still need to make some updates the browser settings changes. Will push again once I'm done.

PIG208 · 2025-03-05T23:26:38Z

Updated the PR. This should be ready for review again!

gnprice

Thanks for the revision! Here's comments on the same prefix as before, 6 commits:
6230f5d do not merge; disable checks
f28eeda db: Store GlobalSettings in database
e6f9809 store [nfc]: Update outdated references to loadGlobalStore
3ef3507 test [nfc]: Use eg.globalStore when possible
9897cff store: Store global settings on global store
c54b92a theme: Track theme through global settings

All small, so I'll also take a look next at the subsequent commits.

gnprice · 2025-03-10T19:23:32Z

lib/model/database.dart

+/// 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 {


nit: order the class definitions matching #1167 (comment)

gnprice · 2025-03-10T19:32:45Z

lib/widgets/app.dart

+      child: Builder(
+        builder: (context) {
+          return MaterialApp(


nit: keep a bit more compact (and less indented):

Suggested change

child: Builder(

builder: (context) {

return MaterialApp(

child: Builder(builder: (context) {

return MaterialApp(

gnprice · 2025-03-10T19:36:17Z

lib/model/settings.dart

+import 'binding.dart';
+import 'database.dart';
+
+/// The user's choice of visual theme for the app


nit: summary line of dartdoc gets a period:

Suggested change

/// The user's choice of visual theme for the app

/// The user's choice of visual theme for the app.

gnprice · 2025-03-10T19:37:51Z

test/model/store_checks.dart

@@ -47,3 +51,9 @@ extension PerAccountStoreChecks on Subject<PerAccountStore> {
  Subject<RecentDmConversationsView> get recentDmConversationsView => has((x) => x.recentDmConversationsView, 'recentDmConversationsView');
  Subject<AutocompleteViewManager> get autocompleteViewManager => has((x) => x.autocompleteViewManager, 'autocompleteViewManager');
 }
+
+extension GlobalSettingsDataChecks on Subject<GlobalSettingsData> {


nit: keep in logical order, matching the corresponding code

gnprice · 2025-03-10T19:45:57Z

lib/model/store.dart

+  GlobalSettingsData get globalSettings => _globalSettings;
+  GlobalSettingsData _globalSettings;
+
+  /// Update the global settings in the store, return the new version.


nit: the syntax in the existing updateAccount doc is correct:

Suggested change

/// Update the global settings in the store, return the new version.

/// Update the global settings in the store, returning the new version.

Other examples would include "do A, resulting in B" or "do A, arriving at B".

gnprice · 2025-03-10T19:47:24Z

test/model/store_checks.dart

@@ -24,6 +26,7 @@ extension GlobalStoreChecks on Subject<GlobalStore> {
  Subject<Iterable<int>> get accountIds => has((x) => x.accountIds, 'accountIds');
  Subject<Iterable<({ int accountId, Account account })>> get accountEntries => has((x) => x.accountEntries, 'accountEntries');
  Subject<Account?> getAccount(int id) => has((x) => x.getAccount(id), 'getAccount($id)');
+  Subject<GlobalSettingsData> get globalSettings => has((x) => x.globalSettings, 'globalSettings');


nit: match order of the underlying properties

gnprice · 2025-03-10T19:48:27Z

test/model/test_store.dart

+  TestGlobalStore({required super.globalSettings, required super.accounts})
+    : _globalSettings = globalSettings;
+
+  GlobalSettingsData? _globalSettings;


Suggested change

GlobalSettingsData? _globalSettings;

GlobalSettingsData _globalSettings;

right?

Hmm or: what if we just leave this field out? Seems like the base class takes care of it.

And in the case of accounts we don't find we need a corresponding field here.

Yeah. It should be very similar to doUpdateAccount:

@override Future<void> doUpdateAccount(int accountId, AccountsCompanion data) async { // Nothing to do. }

gnprice

OK, and here's comments from reading the next two commits:
91992c2 settings: Add settings page with vanilla Flutter widgets
2421036 db: Store browser preference in database

gnprice · 2025-03-10T19:59:01Z

lib/model/settings.dart

+    switch (themeSetting) {
+      case null:
+        return zulipLocalizations.themeSettingSystem;


nit: use switch-expression

gnprice · 2025-03-10T20:03:11Z

test/flutter_checks.dart

@@ -74,6 +74,18 @@ extension ValueListenableChecks<T> on Subject<ValueListenable<T>> {
  Subject<T> get value => has((c) => c.value, 'value');
 }

+extension ThemeDataChecks on Subject<ThemeData> {


nit: for logical ordering, put these Material-specific items down among the other Material-specific items near the end of the file

gnprice · 2025-03-10T20:09:43Z

lib/model/store.dart

+  /// Update the global settings in the store, return the new version.
+  ///
+  /// The global settings must already exist in the store.
+  Future<GlobalSettingsData> updateGlobalSettings(GlobalSettingsCompanion data) async {


This should get a couple of tests. See the GlobalStore.updateAccount tests for examples.

gnprice · 2025-03-10T20:13:09Z

test/model/database_test.dart

+
+      final after = v4.DatabaseAtV4(schema.newConnection());
+      final globalSettings = await after.select(after.globalSettings).getSingle();
+      check(globalSettings.themeSetting).equals(ThemeSetting.light.name);


nit:

Suggested change

check(globalSettings.themeSetting).equals(ThemeSetting.light.name);

check(globalSettings).themeSetting.equals(ThemeSetting.light.name);

(then cascade as usual)

… Oh I see, the type here is v4.GlobalSettingsData, not something more widely shared. OK, no need to have a checks-extension for it.

gnprice · 2025-03-10T20:17:53Z

test/model/database_test.dart

 import 'schemas/schema_v2.dart' as v2;
+import 'store_checks.dart';
+import 'schemas/schema_v3.dart' as v3;


nit: keep sorted

gnprice · 2025-03-10T20:18:38Z

lib/widgets/settings.dart

+        const _BrowserPreferenceSetting(),
+        const _ThemeSetting(),


Seems like theme should come first — it's going to be a much more popular setting to consider.

PIG208 · 2025-03-10T21:44:19Z

Updated the PR. This should be ready now.

gnprice

Thanks! I've now read the whole thing; comments below.

gnprice · 2025-03-10T22:14:08Z

lib/model/database.g.dart

@@ -133,7 +384,6 @@ class $AccountsTable extends Accounts with TableInfo<$AccountsTable, Account> {
    if (data.containsKey('id')) {
      context.handle(_idMeta, id.isAcceptableOrUnknown(data['id']!, _idMeta));
    }
-    context.handle(_realmUrlMeta, const VerificationResult.success());


nit: generated code should update in tandem with the deps upgrade that causes it

gnprice · 2025-03-10T22:20:22Z

test/model/store_test.dart

+      check(notifyCount).equals(1);
+    });
+
+    // TOOD integration tests with sqlite


nit:

Suggested change

// TOOD integration tests with sqlite

// TODO integration tests with sqlite

gnprice · 2025-03-10T22:24:50Z

test/model/store_test.dart

+      check(globalStore).globalSettings.browserPreference.equals(
+        BrowserPreference.external);


This doesn't need to test each setting as we add it; at that point we'd just be testing the generated code from Drift.

gnprice · 2025-03-10T22:36:28Z

lib/model/settings.dart

+  UrlLaunchMode get urlLaunchMode {
+    if (browserPreference != null) {
+      return switch (browserPreference!) {
+        BrowserPreference.embedded => UrlLaunchMode.platformDefault,


This line doesn't look right, does it. 🙂 If the user has explicitly chosen in-app, let's make it in-app even if that isn't the default on a given platform.

(I think you may have asked me about this the other day, but if I said to keep this I was probably thinking of the spot a few lines below where we're defining our own defaults.)

Or is there a reason why that translation doesn't work?

For Android and iOS, platformDefault for HTTP URLs is already in-app browser view; I remember that we decided to keep platformDefault because that setting, "open links with in-app browser", applies to those links to the end-user as expected, but does not make sense for other types of links like mailto.

Testing this, it looks like attempting to open mailto links in-app results in an error:

E/flutter (14030): [ERROR:flutter/runtime/dart_vm_initializer.cc(40)] Unhandled Exception: Invalid argument (url): To use an in-app web view, you must provide an http(s) URL.: Instance of '_SimpleUri' E/flutter (14030): #0 launchUrl (package:url_launcher/src/url_launcher_uri.dart:49:5) E/flutter (14030): #1 LiveZulipBinding.launchUrl (package:zulip/model/binding.dart:355:25) E/flutter (14030): #2 _launchUrl (package:zulip/widgets/content.dart:1446:44)

The comment on the rationale of this is in the build method of the widget that controls this setting, but you bringing it up makes me feel that it probably fits better in the dartdoc of this extension method.

Or perhaps better, in the dartdoc for the embedded (or inApp) enum.

gnprice · 2025-03-10T22:40:47Z

test/widgets/content_test.dart

+      check(testBinding.takeLaunchUrlCalls())
+        .single.equals((
+          url: Uri.parse('https://example/'), mode: LaunchMode.platformDefault));


nit: .single compacts nicely onto previous line:

Suggested change

check(testBinding.takeLaunchUrlCalls())

.single.equals((

url: Uri.parse('https://example/'), mode: LaunchMode.platformDefault));

check(testBinding.takeLaunchUrlCalls()).single.equals((

url: Uri.parse('https://example/'), mode: LaunchMode.platformDefault));

gnprice · 2025-03-10T22:46:55Z

lib/model/settings.dart

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


How about:

Suggested change

/// Use the in-app browser.

embedded,

/// Use the in-app browser.

inApp,

Seems like that would best align the name with how we're calling it in other code and in the UI.

gnprice · 2025-03-10T22:48:57Z

lib/widgets/settings.dart

+import 'page.dart';
+import 'store.dart';
+
+class SettingsPage extends StatelessWidget {


This does not aim to fully implement the wip design for the settings page. We offer it mainly to test settings, so the implementation is kept as simple as possible. WIP design: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=446-21372&t=BZKpTQPSiBDNxwvB-0

Let's replace "WIP design" with more of a complete sentence — it keeps catching my eye as if the "WIP" is a marker about the state of this commit itself.

Cool, this is helpful:

This does not aim to fully implement the wip design for the settings page. We offer it mainly to test settings, so the implementation is kept as simple as possible. Old design for reference: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=446-21372&t=BZKpTQPSiBDNxwvB-0

Let's make a couple of adjustments: s/wip design/draft design/, s/Old design/Draft design (old)/. That way (a) it more fully avoids that "wip" keyword 🙂, (b) it's a bit clearer that the two references are to the same design.

Oh, and a separate nit on the commit message:

settings: Add settings page with vanilla Flutter widgets

I'd read "vanilla Flutter widgets" first of all to mean package:flutter/widgets.dart, aka "the widgets library", in contrast to the Material or Cupertino libraries. Here what we actually mean is we're using Flutter's Material widgets, specifically the adaptive flavor of them.

So let's say:

settings: Add easy settings page using (adaptive) Material widgets This will use generic Material-style controls on Android, and generic iOS-style controls on iOS.

(and then the other information).

PIG208 · 2025-03-11T02:29:57Z

This should be ready for review. Made some follow-up comments to #1167 (comment).

gnprice

Thanks for the revision!

The first 7/10 commits, up through the settings UI, are virtually ready to merge:
7629e44 fix url; deps: Update drift to 2.26.0
98a1a5a db: Store GlobalSettings in database
2ab1c5c store [nfc]: Update outdated references to loadGlobalStore
c84aed0 test [nfc]: Use eg.globalStore when possible
80da3ae store: Store global settings on global store
b214218 theme: Track theme through global settings
ed0afcb settings: Add settings page with vanilla Flutter widgets

but for the browser preference, there's still a substantive comment below. So let's split that part out to a followup PR.

For those first 7 commits, other than nits below, the first commit message just needs to be updated for merge.

gnprice · 2025-03-11T20:11:27Z

lib/model/settings.dart

+  /// Use the in-app browser for HTTP links.
+  ///
+  /// This translates to [UrlLaunchMode.platformDefault], so that non-in-app
+  /// browsers/apps will still be used for other types of links (e.g. mailto)
+  /// on both iOS and Android.
+  inApp,


Cool, putting it here on the enum's doc is helpful for explanation (continuing from #1167 (comment)). Still this translation doesn't seem right.

The launchUrl doc says:

The default behavior of LaunchMode.platformDefault is up to each platform, and its behavior for a given platform may change over time as new modes are supported, so clients that want a specific mode should request it rather than rely on any currently observed default behavior.

so it's explicitly warning not to make the assumption this is making.

The logic in launchUrl that can reject inAppWebView is pretty specific:

if ((mode == LaunchMode.inAppWebView || mode == LaunchMode.inAppBrowserView) && !(url.scheme == 'https' || url.scheme == 'http')) { throw ArgumentError.value(url, 'url', 'To use an in-app web view, you must provide an http(s) URL.'); }

so I think we can just have our own bit of logic that checks if the scheme is http or https, and if not then it replaces inAppBrowserView with platformDefault.

gnprice · 2025-03-11T20:15:53Z

lib/widgets/settings.dart

+import 'page.dart';
+import 'store.dart';
+
+class SettingsPage extends StatelessWidget {


Cool, this is helpful:

This does not aim to fully implement the wip design for the settings page. We offer it mainly to test settings, so the implementation is kept as simple as possible. Old design for reference: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=446-21372&t=BZKpTQPSiBDNxwvB-0

Let's make a couple of adjustments: s/wip design/draft design/, s/Old design/Draft design (old)/. That way (a) it more fully avoids that "wip" keyword 🙂, (b) it's a bit clearer that the two references are to the same design.

gnprice · 2025-03-11T20:21:41Z

lib/widgets/settings.dart

+import 'page.dart';
+import 'store.dart';
+
+class SettingsPage extends StatelessWidget {


Oh, and a separate nit on the commit message:

settings: Add settings page with vanilla Flutter widgets

I'd read "vanilla Flutter widgets" first of all to mean package:flutter/widgets.dart, aka "the widgets library", in contrast to the Material or Cupertino libraries. Here what we actually mean is we're using Flutter's Material widgets, specifically the adaptive flavor of them.

So let's say:

settings: Add easy settings page using (adaptive) Material widgets This will use generic Material-style controls on Android, and generic iOS-style controls on iOS.

(and then the other information).

This contains a fix for simolus3/drift#3384. Signed-off-by: Zixuan James Li <[email protected]>

Signed-off-by: Zixuan James Li <[email protected]>

These were supposed to be updated in commit fc80be1 where we replaced loadGlobalStore with getGlobalStore. `LiveGlobalBindings.load` is the only thing left analogous to the previously referred to `loadGlobalStore` method. To avoid direct referencce to this specific implementation on `GlobalStore`, mention that loading it for the first time is expected to be asynchronous instead. Signed-off-by: Zixuan James Li <[email protected]>

Later we will add more required parameters to TestGlobalStore. Signed-off-by: Zixuan James Li <[email protected]>

To add integration tests for global store with sqlite, we might want to switch to LiveGlobalStore, or at least make part of it testable with an in-memory database (like we do for database_test.dart). Leaving that to the future. Signed-off-by: Zixuan James Li <[email protected]>

Signed-off-by: Zixuan James Li <[email protected]>

This will use generic Material-style controls on Android, and generic iOS-style controls on iOS. This does not aim to fully implement the draft design for the settings page. We offer it mainly to test settings, so the implementation is kept as simple as possible. Draft design for reference: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=446-21372&t=BZKpTQPSiBDNxwvB-0 Fixes: zulip#1216 Signed-off-by: Zixuan James Li <[email protected]>

PIG208 · 2025-03-11T20:36:51Z

Thanks! Updated the PR and the first comment in this thread.

gnprice · 2025-03-11T21:16:53Z

Thanks! Looks good; merging.

PIG208 force-pushed the pr-settings branch 3 times, most recently from 370a328 to e91e9d5 Compare December 17, 2024 00:28

PIG208 force-pushed the pr-settings branch from e91e9d5 to 8c20dc9 Compare December 18, 2024 22:01

gnprice reviewed Dec 19, 2024

View reviewed changes

PIG208 force-pushed the pr-settings branch from 8c20dc9 to 843cc69 Compare December 30, 2024 04:51

PIG208 mentioned this pull request Jan 3, 2025

Prepare schema migration/testing code for upcoming database changes #1248

Merged

PIG208 force-pushed the pr-settings branch from 843cc69 to c94352a Compare January 22, 2025 18:29

PIG208 force-pushed the pr-settings branch from c94352a to 195ff7f Compare February 21, 2025 00:29

PIG208 changed the title ~~wip; Store some client settings~~ Support some client settings; add settings page Feb 21, 2025

PIG208 marked this pull request as ready for review February 21, 2025 00:32

PIG208 added the maintainer review PR ready for review by Zulip maintainers label Feb 21, 2025

PIG208 assigned chrisbobbe Feb 21, 2025

PIG208 requested a review from chrisbobbe February 21, 2025 00:37

chrisbobbe reviewed Feb 22, 2025

View reviewed changes

PIG208 force-pushed the pr-settings branch from 195ff7f to 9d27190 Compare February 25, 2025 00:11

PIG208 requested a review from chrisbobbe February 25, 2025 00:17

PIG208 force-pushed the pr-settings branch from 9d27190 to 6a58054 Compare February 25, 2025 00:18

PIG208 mentioned this pull request Feb 25, 2025

Settings page UI redesign #1375

Open

chrisbobbe assigned gnprice and unassigned gnprice Mar 3, 2025

chrisbobbe requested a review from gnprice March 3, 2025 21:37

chrisbobbe assigned gnprice Mar 3, 2025

PIG208 force-pushed the pr-settings branch 2 times, most recently from 20349a1 to 15df1d9 Compare March 5, 2025 01:15

PIG208 force-pushed the pr-settings branch from 15df1d9 to 199df7c Compare March 5, 2025 23:26

PIG208 force-pushed the pr-settings branch from 199df7c to 10acb77 Compare March 6, 2025 22:08

gnprice reviewed Mar 10, 2025

View reviewed changes

PIG208 force-pushed the pr-settings branch from 10acb77 to e26481b Compare March 10, 2025 21:31

gnprice reviewed Mar 10, 2025

View reviewed changes

PIG208 force-pushed the pr-settings branch from e26481b to 19e13be Compare March 11, 2025 01:11

gnprice reviewed Mar 11, 2025

View reviewed changes

PIG208 added 7 commits March 11, 2025 16:30

deps: Update drift to 2.26.0

8b7c242

This contains a fix for simolus3/drift#3384. Signed-off-by: Zixuan James Li <[email protected]>

db: Store GlobalSettings in database

c5dce93

Signed-off-by: Zixuan James Li <[email protected]>

test [nfc]: Use eg.globalStore when possible

b2a1ffd

Later we will add more required parameters to TestGlobalStore. Signed-off-by: Zixuan James Li <[email protected]>

theme: Track theme through global settings

99d1bbe

Signed-off-by: Zixuan James Li <[email protected]>

PIG208 force-pushed the pr-settings branch from 19e13be to c9cae02 Compare March 11, 2025 20:32

gnprice merged commit c9cae02 into zulip:main Mar 11, 2025
1 check passed

PIG208 mentioned this pull request Mar 11, 2025

Support setting browser preference #1400

Merged

This was referenced Mar 12, 2025

store: Have UpdateMachine.load check account still exists after async gaps #1386

Merged

test [nfc]: Make globalSettings optional on TestGlobalStore #1403

Merged

GlobalSettingsStore, and other refactors #1416

Merged

gnprice mentioned this pull request Mar 19, 2025

Store some client settings #97

Closed

		# Generated code from drift can contain unused local variables.
		- lib/model/database.g.dart

		.single.equals((url: Uri.parse(expectedLaunchUrl), mode: LaunchMode.platformDefault));
		.single.equals((url: Uri.parse(expectedLaunchUrl), mode: LaunchMode.inAppBrowserView));

	/// The user's choice of visual theme for the app
	/// The user's choice of visual theme for the app.

	/// Update the global settings in the store, return the new version.
	/// Update the global settings in the store, returning the new version.

	GlobalSettingsData? _globalSettings;
	GlobalSettingsData _globalSettings;

	check(globalSettings.themeSetting).equals(ThemeSetting.light.name);
	check(globalSettings).themeSetting.equals(ThemeSetting.light.name);

	// TOOD integration tests with sqlite
	// TODO integration tests with sqlite

		check(globalStore).globalSettings.browserPreference.equals(
		BrowserPreference.external);

Support some client settings; add settings page #1167

Support some client settings; add settings page #1167

Conversation

PIG208 commented Dec 17, 2024 • edited Loading

gnprice commented Dec 17, 2024

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Feb 25, 2025

PIG208 commented Feb 25, 2025

chrisbobbe commented Feb 25, 2025

PIG208 commented Feb 25, 2025

PIG208 commented Feb 26, 2025

chrisbobbe commented Mar 3, 2025

PIG208 commented Mar 5, 2025

PIG208 commented Mar 5, 2025

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 commented Mar 10, 2025

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 Mar 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 commented Mar 11, 2025 • edited Loading

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 commented Mar 11, 2025

gnprice commented Mar 11, 2025

PIG208 commented Dec 17, 2024 •

edited

Loading

PIG208 Mar 11, 2025 •

edited

Loading

PIG208 commented Mar 11, 2025 •

edited

Loading