Open links in external browser #4679

WesleyAC · 2021-04-19T10:02:56Z

Based on #4343

chrisbobbe · 2021-04-21T20:20:34Z

Thanks, @WesleyAC, this LGTM! I tested on iOS and it works as expected.

I added one commit on top, to bump the new openLinkExternal toward more visibility/adoption. If that looks good, please merge at will.

WesleyAC · 2021-04-24T07:50:16Z

@chrisbobbe Thanks! I added a setting as per the CZO discussion, would be good to get a review on the last commit in this stack :)

WesleyAC · 2021-04-29T08:50:39Z

Ok, pushed a new version with defaults as discussed on CZO :)

chrisbobbe

Thanks, @WesleyAC! I'm not sure I've mentioned this yet but I'm quite looking forward to this. I often uninstall and reinstall the app on my iPhone, since debug and release builds share an identifier (you can't have one of each installed, like on Android). That means the embedded browser doesn't have a long memory for me being logged into things; I want to start using my normal cookies, as Greg mentions in his reasoning on CZO. A few comments below.

chrisbobbe · 2021-05-04T17:30:26Z

src/reduxTypes.js

+// * embedded: The in-app browser
+// * external: The user's default browser app
+// * default: 'external' on iOS, 'embedded' on Android
+//


nit: Use a jsdoc-style comment to describe interface (I think that's all of the comments you've added here on BrowserPreference).

chrisbobbe · 2021-05-04T17:40:26Z

src/settings/SettingsScreen.js

        />
+        <OptionRow
+          Icon={IconBrowser}
+          label="Open links with in-app browser"


Ah, it looks like OptionRow (possibly soon to be named SwitchRow, in #4683?) makes a <Label /> with label, and Label does try and look up a translation for the string. So we'll need to put this string in messages_en.json.

This reminds me of a discussion here, with an idea that would make this an error in dev, at least at the point of trying to render this <OptionRow /> (so in particular you'd have to be looking at the settings screen after this change). Does that sound good to you; should I send a PR?

Ah, good catch! A PR to error on untranslated strings in dev sounds great.

chrisbobbe · 2021-05-04T17:59:33Z

src/utils/openLink.js

+  }
+}
+
+export function openLinkWithUserPreference(url: string, getState: GetState): void {


When a function needs getState, we've more often made it into a thunk action creator; there are several examples of thunk action creators in fetchActions, like fetchMessages.

That has the nice benefit that the stuff you want to happen in it is guaranteed to happen after the store has rehydrated; that's because of our use of redux-action-buffer. I think the intent is that we can put a bunch of different kinds of stuff in thunk action creators—stuff that we want to prevent happening before the store has rehydrated, or at least that we don't want to have to reason about it running before then. 42adfb3 has some background on this, and here was a place where I hadn't originally reached for a thunk action creator but Greg mentioned it'd be a good choice.

The trouble here is that we're not in a file like fooActions.js. @gnprice, is that a barrier to putting any action creators in the file?

Or, I think I'd be OK merging this as-is with a comment noting that it could one day become a thunk action creator.

(waiting on @gnprice to weigh in here)

Hmm. I agree that this interface taking getState feels odd. I feel like the direction I'd rather go in is to separate this function from being concerned with our Redux store at all -- that feels to me like a layer that lives above this one.

Probably the simplest resolution is to have this just take a BrowserPreference. The caller is already looking around at various other bits of the state, and it's no trouble for it to to pick out that one too and pass it here.

OTOH if we had more call sites, it would start to feel weird that part of the interface of this function was effectively that you had to say something like getSettings(state).browser in its second argument. The exact bit of settings that this depends on feels like it should be an internal implementation detail.

Maybe the best solution is a point in between: have it take a SettingsState. Then:

The exact setting it needs is an internal detail, just like in the current version.

But it's no longer exposed to the whole Redux state.

Also the fact that it does depend on some user setting becomes explicit in the interface. I think it's good for that to be explicit -- I wouldn't e.g. want this to take just one argument url: string, and go find the Redux store as a global or something, or if it did then I'd at least want that connection to be drawn clearly in its jsdoc.

chrisbobbe · 2021-05-04T18:46:37Z

src/settings/settingsReducer.js

  onlineNotification: true,
  experimentalFeaturesEnabled: false,
  streamNotification: false,
+  browser: 'default',


Let's add a migration for adding this to the settings state. I was a bit surprised to see things working, in my local testing, without one.

It turns out that the REHYDRATE action doesn't clobber the initialState with the state that's just been grabbed from storage. Rather, it does a "shallow merge": https://github.com/zulip/zulip-mobile/blob/v27.162/src/third/redux-persist/autoRehydrate.js#L69. That's why there's still a state.settings.browser, post-rehydration.

It took me a while staring at the code to find that fact, and I'm not aware that we intentionally depend on it anywhere. Also, redux-persist and redux-persist-migrate sort of work together, so this logic could plausibly change as we overhaul redux-persist.

We've also found that it's good to err on the side of being explicit and literal in what goes in migrations; e.g., in 1193a57.

b058fa2 has an example of adding a migration for more data being stored. That one was definitely necessary, I think; the "shallow merge" couldn't have helped there.

WesleyAC · 2021-05-06T07:07:16Z

Should be ready for another look @chrisbobbe :)

chrisbobbe · 2021-05-06T23:47:50Z

src/reduxTypes.js

+/* parse this.
+/*
+/* See https://chat.zulip.org/#narrow/stream/48-mobile/topic/in-app.20browser
+/* for the reasoning behind these options.


nit: there are extra /'s on the left; looks like they should be spaces

chrisbobbe · 2021-05-06T23:53:19Z

LGTM, pending that spot where we're waiting for @gnprice to weigh in, here. As I mentioned, I'd be happy enough with a small comment and no further action there, so I'd imagine we could time out waiting for Greg in a day or two if you'd like to. 🙂

Co-authored-by: Shaurya Jain <[email protected]>

Fixes: zulip#4323 Co-authored-by: Shaurya Jain <[email protected]>

This will let people choose whether they want to use the in-app browser, or their external browser app. This defaults to enabled on Android, and disabled on iOS. The setting for this could probably be improved - See the discussion [1] on CZO for what this looks like in other apps. I've opted to use a simple switch, since that's what we're already using in this UI, and I don't want to redesign it right now. I expect we will want to redesign it in the future, but given that we have so few settings right now, I'm not too worried about the settings screen being confusing/overwhelming. Another thing that I'd like to improve is the icon - I'm using the Feather icon for Google Chrome, since the only other browser-related icons I could find were the globe icon we're using for the language setting in this same screen, and the compass icon, which is less recognizable than the Chrome one. I'm unhappy with using a branded icon here, but Feather doesn't give us very many options, sadly. [1]: https://chat.zulip.org/#narrow/stream/48-mobile/topic/in-app.20browser/near/1168259

WesleyAC · 2021-05-08T08:25:36Z

Added that comment, going ahead and merging. Happy to circle back later if we decide we want a thunk action creator instead.

gnprice

Thanks @WesleyAC and @chrisbobbe !

Just took a proper look through these changes today. Comments below, including one on that thunk-action-creator thread and one on the icon.

src/utils/openLink.js

src/message/messagesActions.js

src/common/Icons.js

gnprice · 2021-05-25T03:17:12Z

src/reduxTypes.js

+/**
+ * The values for this mean:
+ *
+ * * embedded: The in-app browser
+ * * external: The user's default browser app
+ * * default: 'external' on iOS, 'embedded' on Android
+ *
+ * Use the `shouldUseInAppBrowser` function from src/utils/openLink.js in order to
+ * parse this.
+ *
+ * See https://chat.zulip.org/#narrow/stream/48-mobile/topic/in-app.20browser
+ * for the reasoning behind these options.
+ */
+export type BrowserPreference = 'embedded' | 'external' | 'default';


Thanks for this detailed jsdoc!

Like all our jsdoc, it should also get a one-line summary at the top. It seems I haven't written that down in our style guide, hmm. But when I do, I don't expect to improve on this item and the one after it (just some details are different because it's a different language):
https://dart.dev/guides/language/effective-dart/documentation#do-start-doc-comments-with-a-single-sentence-summary
In particular:

Provide just enough context for the reader to orient themselves and decide if they should keep reading or look elsewhere for the solution to their problem.

Here, one way of looking at it would be: this nicely provides the answer corresponding to each possible value. But it doesn't really say what question the answers are answering. That'd be a good subject for the summary line.

So one version would be:

/** * The user's preference on what browser to open links with. * * The values for this mean: * * embedded: The in-app browser * * external: The user's default browser app * * default: 'external' on iOS, 'embedded' on Android * …

(That also demonstrates a slight reformatting of the list that I think makes it a bit easier to visually parse.)

gnprice · 2021-05-25T03:38:02Z

src/utils/openLink.js

+  }
+}
+
+export function openLinkWithUserPreference(url: string, getState: GetState): void {


Hmm. I agree that this interface taking getState feels odd. I feel like the direction I'd rather go in is to separate this function from being concerned with our Redux store at all -- that feels to me like a layer that lives above this one.

Probably the simplest resolution is to have this just take a BrowserPreference. The caller is already looking around at various other bits of the state, and it's no trouble for it to to pick out that one too and pass it here.

OTOH if we had more call sites, it would start to feel weird that part of the interface of this function was effectively that you had to say something like getSettings(state).browser in its second argument. The exact bit of settings that this depends on feels like it should be an internal implementation detail.

Maybe the best solution is a point in between: have it take a SettingsState. Then:

The exact setting it needs is an internal detail, just like in the current version.

But it's no longer exposed to the whole Redux state.

Also the fact that it does depend on some user setting becomes explicit in the interface. I think it's good for that to be explicit -- I wouldn't e.g. want this to take just one argument url: string, and go find the Redux store as a global or something, or if it did then I'd at least want that connection to be drawn clearly in its jsdoc.

gnprice · 2021-05-25T03:43:59Z

src/boot/store.js


+  '28': state => ({


Should get a quick explanation, like the other migrations above. (Chris already took care of this in 74ae528.)

WesleyAC requested review from gnprice and chrisbobbe April 19, 2021 10:02

chrisbobbe force-pushed the open-links-in-external-browser branch from 118070a to b7a88f7 Compare April 21, 2021 20:17

WesleyAC force-pushed the open-links-in-external-browser branch from 4f3e1b2 to 693f597 Compare April 24, 2021 07:44

WesleyAC force-pushed the open-links-in-external-browser branch from 693f597 to fb197b8 Compare April 29, 2021 08:50

chrisbobbe reviewed May 4, 2021

View reviewed changes

WesleyAC force-pushed the open-links-in-external-browser branch from fb197b8 to 8064cb1 Compare May 6, 2021 07:05

chrisbobbe reviewed May 6, 2021

View reviewed changes

WesleyAC force-pushed the open-links-in-external-browser branch from 8064cb1 to 3c8a1f2 Compare May 7, 2021 03:55

WesleyAC and others added 5 commits May 8, 2021 14:19

openLink [nfc]: Rename openLink to openLinkEmbedded.

3710763

Co-authored-by: Shaurya Jain <[email protected]>

openLink [nfc]: Add openLinkExternal function.

3d455bd

Co-authored-by: Shaurya Jain <[email protected]>

messagesActions: Open links in external browser.

06b784c

Fixes: zulip#4323 Co-authored-by: Shaurya Jain <[email protected]>

CompatibilityScreen [nfc]: Use openLinkExternal to open the App Store.

64f25d4

WesleyAC force-pushed the open-links-in-external-browser branch from 3c8a1f2 to d7dc4f9 Compare May 8, 2021 06:19

WesleyAC merged commit 3ab6a51 into zulip:master May 8, 2021

gnprice reviewed May 25, 2021

View reviewed changes

This was referenced May 25, 2021

ServerCompatNotice: Add, to nag users connected to servers <2.0.0. #4750

Merged

Followups on open-links-where setting, #4679 #4766

Closed

chrisbobbe mentioned this pull request Jun 14, 2021

Compose area: Enforce mandatory topics #4798

Merged

chrisbobbe mentioned this pull request Sep 7, 2021

Open links in different ways (browser and webview) #4343

Closed

PIG208 mentioned this pull request Mar 14, 2025

Support setting browser preference zulip/zulip-flutter#1400

Merged

Uh oh!

Open links in external browser #4679

Open links in external browser #4679

Uh oh!

Conversation

WesleyAC commented Apr 19, 2021

Uh oh!

chrisbobbe commented Apr 21, 2021

Uh oh!

WesleyAC commented Apr 24, 2021

Uh oh!

WesleyAC commented Apr 29, 2021

Uh oh!

chrisbobbe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe May 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WesleyAC commented May 6, 2021

Uh oh!

chrisbobbe May 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented May 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WesleyAC commented May 8, 2021

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chrisbobbe May 4, 2021 •

edited

Loading

chrisbobbe May 6, 2021 •

edited

Loading

chrisbobbe commented May 6, 2021 •

edited

Loading