Support some client settings; add settings page #1167

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

Work towards #97.

Fixes #1216;
Fixes #1228.

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.

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)

We are waiting for the next release that will fix this issue. Signed-off-by: Zixuan James Li <[email protected]>

The name "unset" was chosen instead of "default" because the latter is a keyword. We could write a custom `TypeConverter` to use "default" instead, but `textEnum` is more convenient. 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]>

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

A builder is needed when themeData needs to access GlobalStore through the BuildContext from the builder. Signed-off-by: Zixuan James Li <[email protected]>

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

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.

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 Fixes: zulip#1216 Signed-off-by: Zixuan James Li <[email protected]>

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

This preserves the default behavior (platform-dependent) when the preference is unset. Signed-off-by: Zixuan James Li <[email protected]>

The translation for the label is taken from zulip-mobile. The UI currently does not have a design, and is made to be as simple as possible to implement for now. Fixes: zulip#1228 Signed-off-by: Zixuan James Li <[email protected]>

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.

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 added 7 commits February 24, 2025 19:09

do not merge; disable checks

56bfe04

We are waiting for the next release that will fix this issue. Signed-off-by: Zixuan James Li <[email protected]>

db: Store GlobalSettings in database

1b3e5e0

The name "unset" was chosen instead of "default" because the latter is a keyword. We could write a custom `TypeConverter` to use "default" instead, but `textEnum` is more convenient. Signed-off-by: Zixuan James Li <[email protected]>

test [nfc]: Use eg.globalStore when possible

6c6f665

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

store: Store global settings on global store

44630c5

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

store [nfc]: Pass builder to GlobalStoreWidget

3ef7d0b

A builder is needed when themeData needs to access GlobalStore through the BuildContext from the builder. Signed-off-by: Zixuan James Li <[email protected]>

theme: Track theme through global settings

f9790ca

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

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 added 3 commits February 24, 2025 19:17

db: Store browser preference in database

15db74c

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

content: Follow browser preference setting when opening links

6eabffe

This preserves the default behavior (platform-dependent) when the preference is unset. Signed-off-by: Zixuan James Li <[email protected]>

settings: Add browser preference setting ui

6a58054

The translation for the label is taken from zulip-mobile. The UI currently does not have a design, and is made to be as simple as possible to implement for now. Fixes: zulip#1228 Signed-off-by: Zixuan James Li <[email protected]>

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support some client settings; add settings page #1167

Support some client settings; add settings page #1167

PIG208 commented Dec 17, 2024 •

edited

Loading

gnprice commented Dec 17, 2024

gnprice left a comment

gnprice Dec 19, 2024

gnprice Dec 19, 2024

gnprice Dec 19, 2024

PIG208 Dec 19, 2024

chrisbobbe Feb 22, 2025

PIG208 Feb 24, 2025

gnprice Dec 19, 2024

chrisbobbe left a comment

chrisbobbe Feb 22, 2025

chrisbobbe Feb 22, 2025

chrisbobbe Feb 22, 2025

chrisbobbe Feb 22, 2025

PIG208 Feb 25, 2025

chrisbobbe Feb 22, 2025

chrisbobbe Feb 22, 2025

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

		# 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));

Support some client settings; add settings page #1167

Are you sure you want to change the base?

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

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

PIG208 commented Dec 17, 2024 •

edited

Loading