Skip to content

Open links in external browser #4679

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
WesleyAC merged 5 commits into from
May 8, 2021
Merged

Open links in external browser #4679

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3710763
openLink [nfc]: Rename openLink to openLinkEmbedded.
WesleyAC Apr 19, 2021
3d455bd
openLink [nfc]: Add openLinkExternal function.
WesleyAC Apr 19, 2021
06b784c
messagesActions: Open links in external browser.
WesleyAC Apr 19, 2021
64f25d4
CompatibilityScreen [nfc]: Use `openLinkExternal` to open the App Store.
chrisbobbe Apr 21, 2021
d7dc4f9
Settings: Add "Open links with in-app browser" setting.
WesleyAC Apr 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/boot/store.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,14 @@ const migrations: {| [string]: (GlobalState) => GlobalState |} = {
accounts: state.accounts.filter(a => a.email !== ''),
}),

'28': state => ({
Comment on lines 285 to +286

@gnprice gnprice May 25, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

...state,
settings: {
...state.settings,
browser: 'default',
},
}),

// TIP: When adding a migration, consider just using `dropCache`.
};

Expand Down
1 change: 1 addition & 0 deletions src/common/Icons.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ const makeIcon = <Glyphs: string>(
return <FixedIcon name={name} {...props} />;
};

export const IconBrowser = makeIcon(Feather, 'chrome');
Comment thread
gnprice marked this conversation as resolved.
export const IconInbox = makeIcon(Feather, 'inbox');
export const IconMention = makeIcon(Feather, 'at-sign');
export const IconSearch = makeIcon(Feather, 'search');
Expand Down
4 changes: 2 additions & 2 deletions src/common/WebLink.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
import React from 'react';

import Label from './Label';
import openLink from '../utils/openLink';
import { openLinkEmbedded } from '../utils/openLink';
import { BRAND_COLOR, createStyleSheet } from '../styles';

type Props = $ReadOnly<{|
Expand All @@ -29,7 +29,7 @@ export default function WebLink(props: Props) {
style={componentStyles.link}
text={props.label}
onPress={() => {
openLink(props.url.toString());
openLinkEmbedded(props.url.toString());
}}
/>
);
Expand Down
4 changes: 2 additions & 2 deletions src/lightbox/LightboxActionSheet.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import share from './share';
import shareImage from './shareImage';
import { showToast } from '../utils/info';
import * as api from '../api';
import openLink from '../utils/openLink';
import { openLinkEmbedded } from '../utils/openLink';

type DownloadImageType = {|
src: string,
Expand Down Expand Up @@ -37,7 +37,7 @@ const tryToDownloadImage = async ({ src, auth }: DownloadImageType) => {
const tempUrl = await api.tryGetFileTemporaryUrl(src, auth);
if (tempUrl === null) {
showToast('Please download the image from your browser');
openLink(new URL(src, auth.realm).toString());
openLinkEmbedded(new URL(src, auth.realm).toString());
return;
}

Expand Down
4 changes: 2 additions & 2 deletions src/lightbox/shareImage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,15 @@ import type { Auth } from '../types';
import ShareFileAndroid from '../nativeModules/ShareFileAndroid';
import { showToast } from '../utils/info';
import * as api from '../api';
import openLink from '../utils/openLink';
import { openLinkEmbedded } from '../utils/openLink';
import * as logging from '../utils/logging';

export default async (url: string, auth: Auth) => {
const tempUrl = await api.tryGetFileTemporaryUrl(url, auth);

if (tempUrl === null) {
showToast('Please share the image from your browser');
openLink(new URL(url, auth.realm).toString());
openLinkEmbedded(new URL(url, auth.realm).toString());
return;
}

Expand Down
6 changes: 3 additions & 3 deletions src/message/messagesActions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import * as NavigationService from '../nav/NavigationService';
import type { Narrow, Dispatch, GetState } from '../types';
import { getAuth } from '../selectors';
import { getMessageIdFromLink, getNarrowFromLink } from '../utils/internalLinks';
import openLink from '../utils/openLink';
import { openLinkWithUserPreference } from '../utils/openLink';
import { navigateToChat } from '../nav/navActions';
import { FIRST_UNREAD_ANCHOR } from '../anchor';
import { getStreamsById } from '../subscriptions/subscriptionSelectors';
Expand Down Expand Up @@ -35,10 +35,10 @@ export const messageLinkPress = (href: string) => async (
const anchor = getMessageIdFromLink(href, auth.realm);
dispatch(doNarrow(narrow, anchor));
} else if (!isUrlOnRealm(href, auth.realm)) {
openLink(href);
openLinkWithUserPreference(href, getState);
} else {
const url =
(await api.tryGetFileTemporaryUrl(href, auth)) ?? new URL(href, auth.realm).toString();
openLink(url);
openLinkWithUserPreference(url, getState);
}
};
16 changes: 16 additions & 0 deletions src/reduxTypes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,13 +263,29 @@ export type RealmState = {|
// https://github.com/zulip/zulip-mobile/issues/4009#issuecomment-619280681.
export type ThemeName = 'default' | 'night';

/**
* 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';
Comment on lines +266 to +279

@gnprice gnprice May 25, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


export type SettingsState = {|
locale: string,
theme: ThemeName,
offlineNotification: boolean,
onlineNotification: boolean,
experimentalFeaturesEnabled: boolean,
streamNotification: boolean,
browser: BrowserPreference,
|};

export type StreamsState = Stream[];
Expand Down
6 changes: 3 additions & 3 deletions src/settings/LegalScreen.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import type { AppNavigationProp } from '../nav/AppNavigator';
import type { Dispatch } from '../types';
import { connect } from '../react-redux';
import { Screen, OptionButton } from '../common';
import openLink from '../utils/openLink';
import { openLinkEmbedded } from '../utils/openLink';
import { getCurrentRealm } from '../selectors';

type Props = $ReadOnly<{|
Expand All @@ -21,12 +21,12 @@ type Props = $ReadOnly<{|
class LegalScreen extends PureComponent<Props> {
openTermsOfService = () => {
const { realm } = this.props;
openLink(new URL('/terms/?nav=no', realm).toString());
openLinkEmbedded(new URL('/terms/?nav=no', realm).toString());
};

openPrivacyPolicy = () => {
const { realm } = this.props;
openLink(new URL('/privacy/?nav=no', realm).toString());
openLinkEmbedded(new URL('/privacy/?nav=no', realm).toString());
};

render() {
Expand Down
16 changes: 14 additions & 2 deletions src/settings/SettingsScreen.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,13 @@ import { ScrollView } from 'react-native';
import type { RouteProp } from '../react-navigation';
import type { MainTabsNavigationProp } from '../main/MainTabsScreen';
import * as NavigationService from '../nav/NavigationService';
import type { Dispatch } from '../types';
import type { Dispatch, BrowserPreference } from '../types';
import { createStyleSheet } from '../styles';
import { connect } from '../react-redux';
import { getSettings } from '../selectors';
import { OptionButton, OptionRow } from '../common';
import {
IconBrowser,
IconDiagnostics,
IconNotifications,
IconNight,
Expand All @@ -25,6 +26,7 @@ import {
navigateToDiagnostics,
navigateToLegal,
} from '../actions';
import { shouldUseInAppBrowser } from '../utils/openLink';

const styles = createStyleSheet({
optionWrapper: {
Expand All @@ -37,6 +39,7 @@ type Props = $ReadOnly<{|
route: RouteProp<'settings', void>,

theme: string,
browser: BrowserPreference,
dispatch: Dispatch,
|}>;

Expand All @@ -47,7 +50,7 @@ class SettingsScreen extends PureComponent<Props> {
};

render() {
const { theme } = this.props;
const { dispatch, theme, browser } = this.props;

return (
<ScrollView style={styles.optionWrapper}>
Expand All @@ -57,6 +60,14 @@ class SettingsScreen extends PureComponent<Props> {
value={theme === 'night'}
onValueChange={this.handleThemeChange}
/>
<OptionRow
Icon={IconBrowser}
label="Open links with in-app browser"

@chrisbobbe chrisbobbe May 4, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@WesleyAC WesleyAC May 6, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

value={shouldUseInAppBrowser(browser)}
onValueChange={value => {
dispatch(settingsChange({ browser: value ? 'embedded' : 'external' }));
}}
/>
<OptionButton
Icon={IconNotifications}
label="Notifications"
Expand Down Expand Up @@ -92,4 +103,5 @@ class SettingsScreen extends PureComponent<Props> {

export default connect(state => ({
theme: getSettings(state).theme,
browser: getSettings(state).browser,
}))(SettingsScreen);
1 change: 1 addition & 0 deletions src/settings/settingsReducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ const initialState: SettingsState = {
onlineNotification: true,
experimentalFeaturesEnabled: false,
streamNotification: false,
browser: 'default',

@chrisbobbe chrisbobbe May 4, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 WesleyAC May 6, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

};

export default (state: SettingsState = initialState, action: Action): SettingsState => {
Expand Down
4 changes: 2 additions & 2 deletions src/start/AuthScreen.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ import { encodeParamsForUrl } from '../utils/url';
import * as webAuth from './webAuth';
import { loginSuccess, navigateToDevAuth, navigateToPasswordAuth } from '../actions';
import IosCompliantAppleAuthButton from './IosCompliantAppleAuthButton';
import openLink from '../utils/openLink';
import { openLinkEmbedded } from '../utils/openLink';

/**
* Describes a method for authenticating to the server.
Expand Down Expand Up @@ -270,7 +270,7 @@ class AuthScreen extends PureComponent<Props> {
id_token: credential.identityToken,
});

openLink(new URL(`/complete/apple/?${params}`, this.props.realm).toString());
openLinkEmbedded(new URL(`/complete/apple/?${params}`, this.props.realm).toString());

// Currently, the rest is handled with the `zulip://` redirect,
// same as in the web flow.
Expand Down
5 changes: 3 additions & 2 deletions src/start/CompatibilityScreen.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
/* @flow strict-local */
import React, { PureComponent } from 'react';
import { Image, Text, View, Platform, Linking } from 'react-native';
import { Image, Text, View, Platform } from 'react-native';

import { openLinkExternal } from '../utils/openLink';
import { Touchable } from '../common';
import { BRAND_COLOR, createStyleSheet } from '../styles';
import appStoreBadgePNG from '../../static/img/app-store-badge.png';
Expand Down Expand Up @@ -51,7 +52,7 @@ export default class CompatibilityScreen extends PureComponent<{||}> {
: 'https://play.google.com/store/apps/details?id=com.zulipmobile';

openStoreURL = () => {
Linking.openURL(this.storeURL);
openLinkExternal(this.storeURL);
};

render() {
Expand Down
4 changes: 2 additions & 2 deletions src/start/webAuth.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { NativeModules, Platform } from 'react-native';
import SafariView from 'react-native-safari-view';

import type { Auth } from '../types';
import openLink from '../utils/openLink';
import { openLinkEmbedded } from '../utils/openLink';
import { tryParseUrl } from '../utils/url';
import { base64ToHex, hexToAscii, xorHexStrings } from '../utils/encoding';

Expand Down Expand Up @@ -48,7 +48,7 @@ export const generateRandomToken = async (): Promise<string> => {
export const generateOtp = async (): Promise<string> => generateRandomToken();

export const openBrowser = (url: string, otp: string) => {
openLink(`${url}?mobile_flow_otp=${otp}`);
openLinkEmbedded(`${url}?mobile_flow_otp=${otp}`);
};

export const closeBrowser = () => {
Expand Down
33 changes: 30 additions & 3 deletions src/utils/openLink.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,38 @@
/* @flow strict-local */
import { NativeModules, Platform } from 'react-native';
import { NativeModules, Platform, Linking } from 'react-native';
import SafariView from 'react-native-safari-view';

export default (url: string): void => {
import type { BrowserPreference, GetState } from '../types';
import { getSettings } from '../selectors';

export function openLinkEmbedded(url: string): void {
Comment thread
gnprice marked this conversation as resolved.
if (Platform.OS === 'ios') {
SafariView.show({ url: encodeURI(url) });
} else {
NativeModules.CustomTabsAndroid.openURL(url);
}
};
}

export function openLinkExternal(url: string): void {
Linking.openURL(url);
}

export function shouldUseInAppBrowser(browser: BrowserPreference): boolean {
if (browser === 'default') {
return Platform.OS === 'android';
} else {
return browser === 'embedded';
}
}

// TODO: We may want to turn this into a thunk action creator.
// See https://github.com/zulip/zulip-mobile/pull/4679#discussion_r625991786
export function openLinkWithUserPreference(url: string, getState: GetState): void {

@chrisbobbe chrisbobbe May 4, 2021 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@WesleyAC WesleyAC May 6, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(waiting on @gnprice to weigh in here)

@gnprice gnprice May 25, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const state = getState();
const browser = getSettings(state).browser;
if (shouldUseInAppBrowser(browser)) {
openLinkEmbedded(url);
} else {
openLinkExternal(url);
}
}
1 change: 1 addition & 0 deletions static/translations/messages_en.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@
"No Internet connection": "No Internet connection",
"Settings": "Settings",
"Night mode": "Night mode",
"Open links with in-app browser": "Open links with in-app browser",
"Language": "Language",
"Arabic": "Arabic",
"Bokmål": "Bokmål",
Expand Down