Experimental feature flags; general bool global settings #1421

gnprice · 2025-03-19T00:21:52Z

(Note NOMERGE demo commit at the end. That illustrates how to use this new facility, and should be removed before merging.)

This introduces a system for experimental feature flags which the user can enable or disable in settings.

One thing I'd like to avoid with such flags is having to do a database migration each time we add or remove one. (Those are fairly heavyweight with Drift's migration system, as each schema version produces several additional copies of our schema in various generated files. One aspect I appreciate in Django's migration system, as seen in the Zulip server, is that it avoids that sort of overhead.)

In order to do that, this puts the new flags as rows in a table, effectively a key-value map. The assumptions that make that table work don't really use the fact that these are specifically being used as experimental feature flags; so we set it up to generically accommodate any sort of boolean global setting, in case we add others in the future.

When no experimental feature flag currently exists in the app, the settings UI for them doesn't appear. That's true in the version of this PR intended for merge, without the NOMERGE commit at the end.

That NOMERGE commit therefore enables manually testing this UI. It also serves to demonstrate adding a new feature flag in this system, which involves a very small amount of code.

Selected commit messages

`f036fea` log: Introduce profilePrint; use it for timing initial fetch

`285414c` db: Add profile-mode timings of load at startup

As we add more features using the database and more queries we make
from it here at the app's startup, this will help us check that we
aren't unduly delaying startup.

Results on my Pixel 8, with 2 account records:

db load time 91.9ms total: 1.5ms init, 85.4ms settings, 5.0ms accounts
db load time 87.8ms total: 1.7ms init, 78.1ms settings, 8.0ms accounts
db load time 93.3ms total: 1.2ms init, 83.7ms settings, 8.5ms accounts
db load time 78.1ms total: 1.4ms init, 71.8ms settings, 4.8ms accounts
db load time 86.9ms total: 1.2ms init, 77.8ms settings, 7.9ms accounts

`e50b68d` settings: Make general bool global settings, so as to add without migrations

This new table to read looks to add something like 5-8ms to the app's
startup time. Not nothing; but not a big cost compared to the time
we spend loading server data right afterward.

Specifically, results on my Pixel 8 (compare to those in the
previous commit):

db load time 117.9ms total: 15.2ms init, 89.2ms settings, 5.1ms bool-settings, 8.3ms accounts
db load time 90.9ms total: 1.3ms init, 78.5ms settings, 8.3ms bool-settings, 2.8ms accounts
db load time 87.2ms total: 1.4ms init, 71.8ms settings, 5.9ms bool-settings, 8.1ms accounts
db load time 85.7ms total: 1.2ms init, 72.8ms settings, 4.9ms bool-settings, 6.8ms accounts
db load time 91.3ms total: 1.4ms init, 80.0ms settings, 7.5ms bool-settings, 2.5ms accounts
db load time 83.8ms total: 1.1ms init, 70.0ms settings, 5.0ms bool-settings, 7.7ms accounts

`e36ed8e` settings: Add the concept of experimental feature flags

Fixes #1409.

`216eecd` NOMERGE demo flag: renderKatex

This demonstrates what's involved in introducing a new
experimental feature flag.

gnprice · 2025-03-19T00:27:12Z

Screenshots:

Main settings page	New settings sub-page

gnprice · 2025-03-19T19:39:07Z

(Filed this as #1427.)

One more thing before this should be merged: #1416 should have adjusted AppDatabase._dropAndCreateAll to reflect the change it made to the onCreate in migration, making it insert the GlobalSettings row.

Without that, if you switch to a branch (like this one) with a later schema version, then back to schema version 5 — only really possible as a developer — you get this error at startup:

E/flutter (15620): [ERROR:flutter/runtime/dart_vm_initializer.cc(40)] Unhandled Exception: Bad state: No element
E/flutter (15620): #0      List.single (dart:core-patch/growable_array.dart:358:22)
E/flutter (15620): #1      Selectable.getSingle (package:drift/src/runtime/query_builder/statements/query.dart:239:26)
E/flutter (15620): <asynchronous suspension>
E/flutter (15620): #2      AppDatabase.getGlobalSettings (package:zulip/model/database.dart:186:12)
E/flutter (15620): <asynchronous suspension>
E/flutter (15620): #3      LiveGlobalStore.load (package:zulip/model/store.dart:886:28)
E/flutter (15620): <asynchronous suspension>
E/flutter (15620): #4      _GlobalStoreWidgetState.initState.<anonymous closure> (package:zulip/widgets/store.dart:89:21)
E/flutter (15620): <asynchronous suspension>

and the app fails to get past the loading screen.

To unwedge it, you can switch to a version before schema version 5 (e.g., to this week's v0.0.27 release) and then forward again; or uninstall the app and reinstall.

I'll fix that, sending a separate PR. That should be merged before this one is, to reduce the window of history that can trigger the bug.

I don't think any of the changes in this PR will need to be revised for that, though. So this remains ready for review.

PIG208

Thanks for adding this! I tested this on my device and it works well switching before/after the demo commit. Overall this looks good to me. Left some comments.

PIG208 · 2025-03-20T23:23:41Z

lib/model/settings.dart

@@ -45,6 +45,94 @@ enum BrowserPreference {
  external,
 }

+/// A general category of account-independent setting the user might set.
+///
+/// Different kinds of settings call for different treatment in the UI,


nit: "treatment" -> "treatments" ?

No, I think "different treatment" is the usual way to put this.

For one handy source of examples, try Linguee:
https://www.linguee.com/english-chinese/search?source=auto&query=different+treatment
First example there has:

according them different treatment as they were different legal concepts.

PIG208 · 2025-03-20T23:29:20Z

lib/model/store.dart

@@ -49,6 +49,8 @@ abstract class GlobalStoreBackend {
  /// This should only be called from [GlobalSettingsStore].


Do we want to have this also apply ti doSetBoolGlobalSetting?

Hmm yeah, I guess that method should get dartdoc.

PIG208 · 2025-03-20T23:55:57Z

lib/model/store.dart

+      await _db.into(_db.boolGlobalSettings).insert(
+        BoolGlobalSettingRow(name: setting.name, value: value),
+        mode: InsertMode.insertOrReplace);


How about using upsert (here and in tests):

Suggested change

await _db.into(_db.boolGlobalSettings).insert(

BoolGlobalSettingRow(name: setting.name, value: value),

mode: InsertMode.insertOrReplace);

await _db.into(_db.boolGlobalSettings).insertOnConflictUpdate(

BoolGlobalSettingRow(name: setting.name, value: value));

This is a bit different because the underlying SQL will be different: it updates the same row, instead of deleting and replacing. But there should be no difference for this use case.

/// Note that this is subtly different from [InsertMode.replace]! When using /// [InsertMode.replace], the old row will be deleted and replaced with the /// new row. With [insertOnConflictUpdate], columns from the old row that are /// not present on [entity] are unchanged, and no row will be deleted.

Sure, I guess that's a bit shorter.

PIG208 · 2025-03-21T00:11:06Z

lib/model/database.dart

+    final rows = await select(boolGlobalSettings).get();
+    for (final row in rows) {
+      final setting = BoolGlobalSetting.byName(row.name);
+      if (setting == null) continue;


I think it will be beneficial to have tests verify that this doesn't throw when there are rows with unknown setting names.

Hmm yeah, good catch. In this version no tests fail if I cut this line and say setting! on the next one.

PIG208 · 2025-03-21T00:22:08Z

test/model/database_test.dart

+    test('BoolGlobalSettings insert, then get', () async {
+      check(await db.getBoolGlobalSettings()).isEmpty();
+
+      // As in doSetBoolGlobalSetting for `value` non-null.


The comments here seem to refer to specifically LiveGlobalStoreBackend's implementation. Now that we have the GlobalStoreBackend abstraction, what if we make a real instance of GlobalStoreBackend and use that for tests?

backend = LiveGlobalStoreBackend(db: db); // uses a public constructor that doesn't exist yet

Then I'm not sure if database_test.dart would be the best home for these tests, if not for testing getBoolGlobalSettings specifically.

Yeah, I think there's definitely some refactoring to be done here. In particular one thing I'd like to do is get the (live) implementations of getBoolGlobalSettings and doSetBoolGlobalSetting to live next to each other — and probably in settings.dart alongside the other details of storing settings.

I might not get to that refactoring before launch, though.

PIG208 · 2025-03-21T00:27:48Z

lib/model/settings.dart

+  // (this list is empty so far)
+  ;
+
+  const BoolGlobalSetting(this.type, this.default_);


nit: move this or the final fields set by it so that they are next to each other

gnprice · 2025-03-21T05:22:39Z

Thanks for the careful review! Pushed a revision; PTAL.

Also sent #1428 to handle the downgrades issue mentioned above. Before merge, this should wait for that, and also have its NOMERGE commit dropped.

PIG208 · 2025-03-21T23:53:42Z

Thanks! This LGTM; will review #1428 soon.

gnprice · 2025-03-24T21:14:49Z

Thanks! And #1428 is now merged, so merging this.

FTR here's that NOMERGE demo commit which I'm now dropping:
b3512ed NOMERGE demo flag: renderKatex

git log --stat -p -1

commit b3512ed0039fa7e057ec80c069403244c4933a3d
Author: Greg Price <[email protected]>
Date:   Sun Mar 16 22:16:57 2025 -0700

    NOMERGE demo flag: renderKatex
    
    This demonstrates what's involved in introducing a new
    experimental feature flag.
---
 lib/model/settings.dart  |  2 ++
 lib/widgets/content.dart | 37 +++++++++++++++++++++++++++----------
 2 files changed, 29 insertions(+), 10 deletions(-)

diff --git lib/model/settings.dart lib/model/settings.dart
index dc1bdc524..d7b7161e8 100644
--- lib/model/settings.dart
+++ lib/model/settings.dart
@@ -110,6 +110,8 @@ enum BoolGlobalSetting {
   /// (Having one stable value in this enum is also handy for tests.)
   placeholderIgnore(GlobalSettingType.placeholder, false),
 
+  renderKatex(GlobalSettingType.experimentalFeatureFlag, false),
+
   // Former settings which might exist in the database,
   // whose names should therefore not be reused:
   // (this list is empty so far)
diff --git lib/widgets/content.dart lib/widgets/content.dart
index c94960282..20e39e866 100644
--- lib/widgets/content.dart
+++ lib/widgets/content.dart
@@ -15,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';
@@ -831,11 +832,19 @@ class MathBlock extends StatelessWidget {
   @override
   Widget build(BuildContext context) {
     final contentTheme = ContentTheme.of(context);
-    return _CodeBlockContainer(
-      borderColor: contentTheme.colorMathBlockBorder,
-      child: Text.rich(TextSpan(
-        style: contentTheme.codeBlockTextStyles.plain,
-        children: [TextSpan(text: node.texSource)])));
+    final globalSettings = GlobalStoreWidget.settingsOf(context);
+
+    final renderKatex = globalSettings.getBool(BoolGlobalSetting.renderKatex);
+    if (!renderKatex) {
+      return _CodeBlockContainer(
+        borderColor: contentTheme.colorMathBlockBorder,
+        child: Text.rich(TextSpan(
+          style: contentTheme.codeBlockTextStyles.plain,
+          children: [TextSpan(text: node.texSource)])));
+    }
+
+    return Text(style: contentTheme.textStyleError,
+      '(error: KaTeX unimplemented)'); // TODO(#1408)
   }
 }
 
@@ -1147,11 +1156,19 @@ class _InlineContentBuilder {
           child: MessageImageEmoji(node: node));
 
       case MathInlineNode():
-        return TextSpan(
-          style: widget.style
-            .merge(ContentTheme.of(_context!).textStyleInlineMath)
-            .apply(fontSizeFactor: kInlineCodeFontSizeFactor),
-          children: [TextSpan(text: node.texSource)]);
+        final contentTheme = ContentTheme.of(_context!);
+        final globalSettings = GlobalStoreWidget.settingsOf(_context!);
+        final renderKatex = globalSettings.getBool(BoolGlobalSetting.renderKatex);
+        if (!renderKatex) {
+          return TextSpan(
+            style: widget.style
+              .merge(ContentTheme.of(_context!).textStyleInlineMath)
+              .apply(fontSizeFactor: kInlineCodeFontSizeFactor),
+            children: [TextSpan(text: node.texSource)]);
+        }
+
+        return TextSpan(style: contentTheme.textStyleError,
+          text: '(error: KaTeX unimplemented)'); // TODO(#1408)
 
       case GlobalTimeNode():
         return WidgetSpan(alignment: PlaceholderAlignment.middle,

As we add more settings, this will keep things better clustered logically together.

As we add more features using the database and more queries we make from it here at the app's startup, this will help us check that we aren't unduly delaying startup. Results on my Pixel 8, with 2 account records: db load time 91.9ms total: 1.5ms init, 85.4ms settings, 5.0ms accounts db load time 87.8ms total: 1.7ms init, 78.1ms settings, 8.0ms accounts db load time 93.3ms total: 1.2ms init, 83.7ms settings, 8.5ms accounts db load time 78.1ms total: 1.4ms init, 71.8ms settings, 4.8ms accounts db load time 86.9ms total: 1.2ms init, 77.8ms settings, 7.9ms accounts

This aligns these tests more closely with how we expect most app code to interact with these settings (using GlobalStoreWidget.settingsOf).

This probably should have happened as part of 1cf31a4 and 9201ae4, oh well.

This is the same abbreviation we use in implementation code. Especially when the name occurs repeatedly in a line, this can make the code usefully more concise.

…rations This new table to read looks to add something like 5-8ms to the app's startup time. Not nothing; but not a big cost compared to the time we spend loading server data right afterward. Specifically, results on my Pixel 8 (compare to those in the previous commit): db load time 117.9ms total: 15.2ms init, 89.2ms settings, 5.1ms bool-settings, 8.3ms accounts db load time 90.9ms total: 1.3ms init, 78.5ms settings, 8.3ms bool-settings, 2.8ms accounts db load time 87.2ms total: 1.4ms init, 71.8ms settings, 5.9ms bool-settings, 8.1ms accounts db load time 85.7ms total: 1.2ms init, 72.8ms settings, 4.9ms bool-settings, 6.8ms accounts db load time 91.3ms total: 1.4ms init, 80.0ms settings, 7.5ms bool-settings, 2.5ms accounts db load time 83.8ms total: 1.1ms init, 70.0ms settings, 5.0ms bool-settings, 7.7ms accounts

Fixes zulip#1409.

gnprice added the maintainer review PR ready for review by Zulip maintainers label Mar 19, 2025

gnprice requested a review from PIG208 March 19, 2025 00:21

gnprice mentioned this pull request Mar 20, 2025

api: Don't allow connecting to servers <4.0 #1410

Merged

PIG208 reviewed Mar 21, 2025

View reviewed changes

This was referenced Mar 21, 2025

Schema downgrades fail (from future schema versions) #1427

Closed

Fix DB downgrades to match creating from scratch #1428

Merged

gnprice force-pushed the pr-feature-flags branch from 216eecd to 8d9f89c Compare March 21, 2025 05:21

PIG208 added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels Mar 21, 2025

gnprice force-pushed the pr-feature-flags branch from 8d9f89c to b3512ed Compare March 24, 2025 19:53

gnprice force-pushed the pr-feature-flags branch from b3512ed to 6a77be0 Compare March 24, 2025 21:15

gnprice added 9 commits March 24, 2025 14:15

settings [nfc]: Reorder GlobalSettingsStore members to group by setting

aa9f3e9

As we add more settings, this will keep things better clustered logically together.

log: Introduce profilePrint; use it for timing initial fetch

8a1c562

settings test [nfc]: Use globalSettings as variable

8f5df97

This aligns these tests more closely with how we expect most app code to interact with these settings (using GlobalStoreWidget.settingsOf).

settings test [nfc]: Move tests from store_test; rename

9456d44

This probably should have happened as part of 1cf31a4 and 9201ae4, oh well.

db test [nfc]: Rename database local as "db"

8f4155d

This is the same abbreviation we use in implementation code. Especially when the name occurs repeatedly in a line, this can make the code usefully more concise.

settings: Add the concept of experimental feature flags

cb3fa1d

Fixes zulip#1409.

content [nfc]: Dedupe a pair of ContentTheme.of lookups

8cf9c37

gnprice force-pushed the pr-feature-flags branch from 6a77be0 to 8cf9c37 Compare March 24, 2025 21:15

gnprice merged commit 8cf9c37 into zulip:main Mar 24, 2025
1 check passed

gnprice deleted the pr-feature-flags branch March 24, 2025 21:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Experimental feature flags; general bool global settings #1421

Experimental feature flags; general bool global settings #1421

gnprice commented Mar 19, 2025

gnprice commented Mar 19, 2025

gnprice commented Mar 19, 2025 •

edited

Loading

PIG208 left a comment

PIG208 Mar 20, 2025

gnprice Mar 21, 2025

PIG208 Mar 20, 2025

gnprice Mar 21, 2025

PIG208 Mar 20, 2025

gnprice Mar 21, 2025

PIG208 Mar 21, 2025

gnprice Mar 21, 2025

PIG208 Mar 21, 2025

gnprice Mar 21, 2025

PIG208 Mar 21, 2025

gnprice commented Mar 21, 2025

PIG208 commented Mar 21, 2025

gnprice commented Mar 24, 2025

		@@ -49,6 +49,8 @@ abstract class GlobalStoreBackend {
		/// This should only be called from [GlobalSettingsStore].

Experimental feature flags; general bool global settings #1421

Experimental feature flags; general bool global settings #1421

Conversation

gnprice commented Mar 19, 2025

Selected commit messages

f036fea log: Introduce profilePrint; use it for timing initial fetch

285414c db: Add profile-mode timings of load at startup

e50b68d settings: Make general bool global settings, so as to add without migrations

e36ed8e settings: Add the concept of experimental feature flags

216eecd NOMERGE demo flag: renderKatex

gnprice commented Mar 19, 2025

gnprice commented Mar 19, 2025 • edited Loading

PIG208 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice commented Mar 21, 2025

PIG208 commented Mar 21, 2025

gnprice commented Mar 24, 2025

`f036fea` log: Introduce profilePrint; use it for timing initial fetch

`285414c` db: Add profile-mode timings of load at startup

`e50b68d` settings: Make general bool global settings, so as to add without migrations

`e36ed8e` settings: Add the concept of experimental feature flags

`216eecd` NOMERGE demo flag: renderKatex

gnprice commented Mar 19, 2025 •

edited

Loading