Support setting browser preference #1400

PIG208 · 2025-03-11T22:51:01Z

This is taken from #1167, addressing the reviews there.

gnprice · 2025-03-11T23:33:36Z

lib/widgets/settings.dart

+    final openLinksWithInAppBrowser =
+      GlobalStoreWidget.of(context).globalSettings.urlLaunchMode
+      == UrlLaunchMode.platformDefault;


Given that we'll be presenting the setting as either "in-app: yes" or "in-app: no", let's make the semantics of our default match that presentation. IOW on Android let's adjust our default behavior so that it's the same as the behavior we end up with if the database says inApp.

That'd be good to do as a prep commit, before introducing the setting.

PIG208 · 2025-03-12T00:51:46Z

Thanks! This is ready for review.

Screenshot

gnprice · 2025-03-13T06:51:49Z

test/widgets/content_test.dart

-        .single.equals((url: Uri.parse('https://a/'), mode: LaunchMode.platformDefault));
+        .single.equals((url: Uri.parse('https://a/'), mode: LaunchMode.inAppBrowserView));


setting: Make in-app explicitly the default browser preference on Android This way, the default behavior when opening links on Android is equivalent as if the browser preference is set to in-app.

I think both these two commits:
d1242a2 content [nfc]: Follow browser preference setting when opening links
f2a8d6a setting: Make in-app explicitly the default browser preference on Android

will be clearer to understand if the "explicitly in-app" commit comes first, as I suggested at #1400 (comment) .

Let's save making that revision for our call tomorrow. I can demonstrate how I'd go about re-splitting these changes (I think this is a case where that will be easier than trying to treat it as a rebase), and I think it'll be a good example.

PIG208 · 2025-03-13T21:51:00Z

Thanks for the help! I have updated the PR.

gnprice

Thanks!

This is quite close now — a handful of nits below, following the revision I just pushed which expands some of the explanation in this commit:
f06b14c setting [nfc]: Make in-app explicitly the browser preference on Android

gnprice · 2025-03-14T04:37:10Z

lib/model/settings.dart

+
+/// What browser the user has set to use for opening links in messages.
+///
+/// See https://chat.zulip.org/#narrow/stream/48-mobile/topic/in-app.20browser


nit: always use a permalink (so for Zulip topics, either /near/ or /with/)

Removed the link because I think the dartdoc on the individual enum options explain them well enough, and that we have links to this conversation from effectiveBrowserPreference.

This line was originally from zulip/zulip-mobile#4679 (comment), where the code that interprets the enum values live elsewhere.

gnprice · 2025-03-14T05:00:21Z

lib/model/settings.dart

+      BrowserPreference.inApp => url.scheme == 'http' || url.scheme == 'https'
+        ? UrlLaunchMode.inAppBrowserView : UrlLaunchMode.platformDefault,


This condition calls for a comment to explain why we need this (and why it takes the particular shape it does).

Much of the needed information is in the commit message, so just needs to be moved from there. I also had some changes I wanted to make to the framing of that, so I made a revision which I'll push along with this review.

Ah I see! It makes sense that the upstream behavior needs to change.

gnprice · 2025-03-14T05:02:28Z

test/model/settings_test.dart

+        defaultTargetPlatform == TargetPlatform.iOS
+          ? UrlLaunchMode.externalApplication : UrlLaunchMode.inAppBrowserView);


nit: the conditions in these two tests are confusing because they go opposite ways (so the expected values come in opposite orders) — either way seems fine, just make them consistent

gnprice · 2025-03-14T05:04:04Z

test/model/settings_test.dart

+      final globalStore = eg.globalStore(globalSettings: eg.globalSettings(
+        browserPreference: null));


nit: this bit of setup should arrive in the "follow setting" commit, rather than the preceding commit where the setting isn't yet involved

gnprice · 2025-03-14T05:07:02Z

assets/l10n/app_en.arb

@@ -811,6 +811,10 @@
  "@themeSettingSystem": {
    "description": "Label for system theme setting."
  },
+  "openLinksWithInAppBrowser": "Open links with in-app browser",


The translation for the label is taken from zulip-mobile. The user facing string refers to "in in-app browser" when the actual value is `UrlLaunchMode.platformDefault`. This is consistent with the behavior the user sees when the link is a HTTP URL on Android and iOS, which should cover the majority of use cases of this setting (and probably what the user would expect).

This apologetic paragraph can get a lot simpler now 🙂 — I think really can just be cut.

The exact semantics are now "Open links with in-app browser where possible", and the implementation code now makes clear that those are the semantics. And having a "where possible" qualifier be implicit is pretty normal for UI text (outside of UIs for quite technical domains).

PIG208 · 2025-03-14T19:22:51Z

Thanks! Updated the PR.

gnprice · 2025-03-15T04:04:50Z

Thanks! Looks good; merging.

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

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

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 <[email protected]>

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 assigned gnprice Mar 11, 2025

PIG208 added the integration review Added by maintainers when PR may be ready for integration label Mar 11, 2025

PIG208 requested a review from gnprice March 11, 2025 22:51

gnprice reviewed Mar 11, 2025

View reviewed changes

PIG208 force-pushed the pr-settings branch 2 times, most recently from 3ec51af to 85c0d5b Compare March 12, 2025 00:49

gnprice reviewed Mar 13, 2025

View reviewed changes

gnprice force-pushed the pr-settings branch from 85c0d5b to 36d1c6f Compare March 13, 2025 19:48

PIG208 force-pushed the pr-settings branch from 36d1c6f to fe06129 Compare March 13, 2025 21:50

gnprice force-pushed the pr-settings branch from fe06129 to a8e9028 Compare March 14, 2025 05:12

gnprice reviewed Mar 14, 2025

View reviewed changes

PIG208 force-pushed the pr-settings branch from a8e9028 to 36a3534 Compare March 14, 2025 19:21

PIG208 and others added 4 commits March 14, 2025 21:04

db: Store browser preference in database

0a22d86

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

settings: Add browser preference setting ui

b030737

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]>

gnprice force-pushed the pr-settings branch from 36a3534 to b030737 Compare March 15, 2025 04:07

gnprice merged commit b030737 into zulip:main Mar 15, 2025
1 check passed

PIG208 mentioned this pull request Mar 17, 2025

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support setting browser preference #1400

Support setting browser preference #1400

PIG208 commented Mar 11, 2025

gnprice Mar 11, 2025

PIG208 commented Mar 12, 2025 •

edited

Loading

gnprice Mar 13, 2025

PIG208 commented Mar 13, 2025

gnprice left a comment

gnprice Mar 14, 2025

PIG208 Mar 14, 2025

gnprice Mar 14, 2025

PIG208 Mar 14, 2025

gnprice Mar 14, 2025

gnprice Mar 14, 2025

gnprice Mar 14, 2025

PIG208 commented Mar 14, 2025

gnprice commented Mar 15, 2025

		.single.equals((url: Uri.parse('https://a/'), mode: LaunchMode.platformDefault));
		.single.equals((url: Uri.parse('https://a/'), mode: LaunchMode.inAppBrowserView));

		BrowserPreference.inApp => url.scheme == 'http' \|\| url.scheme == 'https'
		? UrlLaunchMode.inAppBrowserView : UrlLaunchMode.platformDefault,

		defaultTargetPlatform == TargetPlatform.iOS
		? UrlLaunchMode.externalApplication : UrlLaunchMode.inAppBrowserView);

		final globalStore = eg.globalStore(globalSettings: eg.globalSettings(
		browserPreference: null));

Support setting browser preference #1400

Support setting browser preference #1400

Conversation

PIG208 commented Mar 11, 2025

Choose a reason for hiding this comment

PIG208 commented Mar 12, 2025 • edited Loading

Choose a reason for hiding this comment

PIG208 commented Mar 13, 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

PIG208 commented Mar 14, 2025

gnprice commented Mar 15, 2025

PIG208 commented Mar 12, 2025 •

edited

Loading