diff --git a/src/chat/ChatScreen.js b/src/chat/ChatScreen.js index cd36cf80355..ca2f9642ebd 100644 --- a/src/chat/ChatScreen.js +++ b/src/chat/ChatScreen.js @@ -29,7 +29,7 @@ import { getAuth, getCaughtUpForNarrow } from '../selectors'; import { showErrorAlert } from '../utils/info'; import { TranslationContext } from '../boot/TranslationProvider'; import * as api from '../api'; -import { useEdgeTriggeredEffect } from '../reactUtils'; +import { useConditionalEffect } from '../reactUtils'; type Props = $ReadOnly<{| navigation: AppNavigationProp<'chat'>, @@ -71,9 +71,9 @@ const useMessagesWithFetch = args => { // like using instance variables in class components: // https://reactjs.org/docs/hooks-faq.html#is-there-something-like-instance-variables const shouldFetchWhenNextFocused = React.useRef(false); - const scheduleFetch = () => { + const scheduleFetch = useCallback(() => { shouldFetchWhenNextFocused.current = true; - }; + }, []); const [fetchError, setFetchError] = React.useState(null); @@ -92,7 +92,7 @@ const useMessagesWithFetch = args => { if (eventQueueId !== null) { scheduleFetch(); } - }, [eventQueueId]); + }, [eventQueueId, scheduleFetch]); // If we stop having any data at all about the messages in this narrow -- // we don't know any, and nor do we know if there are some -- then @@ -104,7 +104,7 @@ const useMessagesWithFetch = args => { // isFetching false, even though the fetch effect will cause a rerender // with isFetching true. It'd be nice to avoid that. const nothingKnown = messages.length === 0 && !caughtUp.older && !caughtUp.newer; - useEdgeTriggeredEffect(scheduleFetch, nothingKnown, true); + useConditionalEffect(scheduleFetch, nothingKnown); // On first mount, fetch. (This also makes a fetch no longer scheduled, // so the if-scheduled fetch below doesn't also fire.) diff --git a/src/common/SmartUrlInput.js b/src/common/SmartUrlInput.js index a0983349d01..daab8bbf989 100644 --- a/src/common/SmartUrlInput.js +++ b/src/common/SmartUrlInput.js @@ -1,15 +1,12 @@ /* @flow strict-local */ import React, { useState, useRef, useCallback, useContext } from 'react'; import type { Node } from 'react'; -import { TextInput, TouchableWithoutFeedback, View } from 'react-native'; +import { TextInput, View } from 'react-native'; import { useFocusEffect } from '@react-navigation/native'; import type { ViewStyleProp } from 'react-native/Libraries/StyleSheet/StyleSheet'; import type { AppNavigationProp } from '../nav/AppNavigator'; -import { ThemeContext, createStyleSheet } from '../styles'; -import { autocompleteRealmPieces, autocompleteRealm, fixRealmUrl } from '../utils/url'; -import type { Protocol } from '../utils/url'; -import ZulipText from './ZulipText'; +import { ThemeContext, createStyleSheet, HALF_COLOR } from '../styles'; const styles = createStyleSheet({ wrapper: { @@ -17,38 +14,18 @@ const styles = createStyleSheet({ opacity: 0.8, }, realmInput: { + flex: 1, padding: 0, fontSize: 20, }, - realmPlaceholder: { - opacity: 0.75, - }, - realmInputEmpty: { - width: 1, - }, }); type Props = $ReadOnly<{| - /** - * The protocol which will be used if the user doesn't specify one. - * Should almost certainly be "https://". - */ - defaultProtocol: Protocol, - /** - * The example organization name that will be displayed while the - * entry field is empty. Appears, briefly, as the initial (lowest- - * level) component of the realm's domain. - */ - defaultOrganization: string, - /** - * The default domain to which the user's input will be appended, if - * it appears not to contain an explicit domain. - */ - defaultDomain: string, // TODO: Currently this type is acceptable because the only // `navigation` prop we pass to a `SmartUrlInput` instance is the // one from a component on AppNavigator. navigation: AppNavigationProp<>, + style?: ViewStyleProp, onChangeText: (value: string) => void, onSubmitEditing: () => Promise, @@ -56,15 +33,7 @@ type Props = $ReadOnly<{| |}>; export default function SmartUrlInput(props: Props): Node { - const { - defaultProtocol, - defaultOrganization, - defaultDomain, - style, - onChangeText, - onSubmitEditing, - enablesReturnKeyAutomatically, - } = props; + const { style, onChangeText, onSubmitEditing, enablesReturnKeyAutomatically } = props; // We should replace the fixme with // `React$ElementRef` when we can. Currently, that @@ -72,10 +41,6 @@ export default function SmartUrlInput(props: Props): Node { // this is probably down to bugs in Flow's special support for React. const textInputRef = useRef<$FlowFixMe>(); - /** - * The actual input string, exactly as entered by the user, - * without modifications by autocomplete. - */ const [value, setValue] = useState(''); const themeContext = useContext(ThemeContext); @@ -86,6 +51,14 @@ export default function SmartUrlInput(props: Props): Node { useFocusEffect( useCallback(() => { if (textInputRef.current) { + // Sometimes the effect of this `.focus()` is immediately undone + // (the keyboard is closed) by a Keyboard.dismiss() from React + // Navigation's internals. Seems like a complex bug, but the symptom + // isn't terrible, it just means that on back-navigating to this + // screen, sometimes the keyboard flicks open then closed, instead + // of just opening. Shrug. See + // https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/realm-input/near/1346690 + // // `.current` is not type-checked; see definition. textInputRef.current.focus(); } @@ -95,52 +68,18 @@ export default function SmartUrlInput(props: Props): Node { const handleChange = useCallback( (_value: string) => { setValue(_value); - - onChangeText( - fixRealmUrl( - autocompleteRealm(_value, { protocol: defaultProtocol, domain: defaultDomain }), - ), - ); + onChangeText(_value); }, - [defaultDomain, defaultProtocol, onChangeText], + [onChangeText], ); - const urlPress = useCallback(() => { - if (textInputRef.current) { - // `.current` is not type-checked; see definition. - textInputRef.current.blur(); - setTimeout(() => { - if (textInputRef.current) { - // `.current` is not type-checked; see definition. - textInputRef.current.focus(); - } - }, 100); - } - }, []); - - const renderPlaceholderPart = (text: string) => ( - - - - ); - - const [prefix, , suffix] = autocompleteRealmPieces(value, { - domain: defaultDomain, - protocol: defaultProtocol, - }); - return ( - {prefix !== null && renderPlaceholderPart(prefix)} - {!value && renderPlaceholderPart(defaultOrganization)} - {suffix !== null && renderPlaceholderPart(suffix)} ); } diff --git a/src/reactUtils.js b/src/reactUtils.js index 1d028d34aed..5fb44c006bf 100644 --- a/src/reactUtils.js +++ b/src/reactUtils.js @@ -85,33 +85,22 @@ export const useHasStayedTrueForMs = (value: boolean, duration: number): boolean }; /** - * Invoke the callback as an effect when `value` turns from false to true. + * Like `useEffect`, but the callback only runs when `value` is true. * - * This differs from putting `value` in a `useEffect` dependency list in - * that the callback will *not* be invoked when the value changes in the - * other direction, from true to false. + * Callers should wrap the callback in `useCallback` with an appropriate + * array of dependencies. * - * `includeStart` controls what happens if `value` is true on the very first - * render. If `includeStart` is true, then the effect is triggered (so - * treating its previous state of nonexistence as equivalent to being - * false). If `includeStart` is false, then the effect is not triggered (so - * treating its previous state of nonexistence as not a valid state to - * compare to.) + * The callback will run once at the beginning of every period of `value` + * being true, and again throughout such a period whenever the value of the + * callback changes. * - * The callback is not permitted to return a cleanup function, because it's - * not clear what the semantics should be of when such a cleanup function - * would be run. + * As with `useEffect`, the cleanup function, if provided, will run once for + * every time the callback is called. If `value` goes from true to false, + * the cleanup function will be called at that time. */ -export const useEdgeTriggeredEffect = ( - cb: () => void, - value: boolean, - includeStart: boolean, -): void => { - const prev = usePrevious(value, !includeStart); - useEffect(() => { - if (value && !prev) { - cb(); - } - // No dependencies list -- the effect itself decides whether to act. - }); -}; +// The claims about when `cb` runs assume that useEffect doesn't run its +// callback on non-initial renders where its dependencies are unchanged. The +// docs could be clearer about that: +// https://reactjs.org/docs/hooks-effect.html#tip-optimizing-performance-by-skipping-effects +export const useConditionalEffect = (cb: () => void | (() => void), value: boolean): void => + useEffect(() => (value ? cb() : undefined), [value, cb]); diff --git a/src/start/RealmInputScreen.js b/src/start/RealmInputScreen.js index 9a8147b443f..89cf0faaddb 100644 --- a/src/start/RealmInputScreen.js +++ b/src/start/RealmInputScreen.js @@ -27,6 +27,14 @@ type State = {| progress: boolean, |}; +const urlFromInputValue = (realmInputValue: string): URL | void => { + const withScheme = /^https?:\/\//.test(realmInputValue) + ? realmInputValue + : `https://${realmInputValue}`; + + return tryParseUrl(withScheme); +}; + export default class RealmInputScreen extends PureComponent { state: State = { progress: false, @@ -37,7 +45,7 @@ export default class RealmInputScreen extends PureComponent { tryRealm: () => Promise = async () => { const { realmInputValue } = this.state; - const parsedRealm = tryParseUrl(realmInputValue); + const parsedRealm = urlFromInputValue(realmInputValue); if (!parsedRealm) { this.setState({ error: 'Please enter a valid URL' }); return; @@ -92,9 +100,6 @@ export default class RealmInputScreen extends PureComponent { { text="Enter" progress={progress} onPress={this.tryRealm} - disabled={tryParseUrl(realmInputValue) === undefined} + disabled={urlFromInputValue(realmInputValue) === undefined} /> ); diff --git a/src/utils/__tests__/url-test.js b/src/utils/__tests__/url-test.js index c4091cf7398..1487cbbe09c 100644 --- a/src/utils/__tests__/url-test.js +++ b/src/utils/__tests__/url-test.js @@ -4,15 +4,11 @@ import { getResource, isUrlOnRealm, parseProtocol, - fixRealmUrl, - autocompleteRealmPieces, - autocompleteRealm, isUrlAbsolute, isUrlRelative, isUrlPathAbsolute, } from '../url'; import type { Auth } from '../../types'; -import type { AutocompletionDefaults } from '../url'; const urlClassifierCases = { // These data are mostly a selection from this resource: @@ -161,155 +157,3 @@ describe('parseProtocol', () => { expect(parseProtocol('\xA0http://example.org')).toEqual(['http://', 'example.org']); }); }); - -describe('fixRealmUrl', () => { - test('undefined input results in empty string', () => { - expect(fixRealmUrl()).toEqual(''); - }); - - test('empty url input results in empty string', () => { - expect(fixRealmUrl('')).toEqual(''); - }); - - test('when a realm url is missing a protocol, prepend https', () => { - expect(fixRealmUrl('example.com')).toEqual('https://example.com'); - }); - - test('when a realm url has a trailing "/" remove it', () => { - expect(fixRealmUrl('https://example.com/')).toEqual('https://example.com'); - }); - - test('when a realm url has two trailing "/" remove them', () => { - expect(fixRealmUrl('https://example.com//')).toEqual('https://example.com'); - }); - - test('when input url is correct, do not change it', () => { - expect(fixRealmUrl('https://example.com')).toEqual('https://example.com'); - }); - - test('remove white-space around input', () => { - expect(fixRealmUrl(' https://example.com/ ')).toEqual('https://example.com'); - }); - - test('remove white-space inside input', () => { - const result = fixRealmUrl('https://subdomain .example. com/ '); - expect(result).toEqual('https://subdomain.example.com'); - }); -}); - -describe('autocompleteRealmPieces', () => { - const exampleData: AutocompletionDefaults = { - protocol: 'http://', - domain: 'example.com', - }; - - test('the empty string yields reasonable values', () => { - const [head, , tail] = autocompleteRealmPieces('', exampleData); - expect(head).toEqual('http://'); - expect(tail).toEqual('.example.com'); - }); - - /* Test that input value is unchanged. - - Future versions of `autocompleteRealmPieces` may alter certain inputs -- - for example, by trimming spaces, standardizing to lowercase, or escaping - via punycode -- but the particular values tested here should all remain - unaltered. - */ - const doSimpleCompletion = (input: string, data?: AutocompletionDefaults) => { - const [head, output, tail] = autocompleteRealmPieces(input, data ?? exampleData); - expect(input).toEqual(output); - return [head, tail]; - }; - - test('a plain word is fully autocompleted', () => { - const [head, tail] = doSimpleCompletion('host-name'); - expect(head).toEqual('http://'); - expect(tail).toEqual('.example.com'); - }); - - test('an explicit `http` is recognized', () => { - const [head, tail] = doSimpleCompletion('http://host-name'); - expect(head).toBeFalsy(); - expect(tail).toEqual('.example.com'); - }); - - test('an explicit `https` is recognized', () => { - const [head, tail] = doSimpleCompletion('https://host-name'); - expect(head).toBeFalsy(); - expect(tail).toEqual('.example.com'); - }); - - test('an explicit IPv4 is recognized', () => { - const [head, tail] = doSimpleCompletion('23.6.64.128'); - expect(head).toBeTruthy(); - expect(tail).toBeFalsy(); - }); - - test('an explicit IPv6 is recognized', () => { - const [head, tail] = doSimpleCompletion('[2a02:26f0:12f:293:0:0:0:255e]'); - expect(head).toBeTruthy(); - expect(tail).toBeFalsy(); - }); - - test('localhost with an explicit port is recognized', () => { - const [head, tail] = doSimpleCompletion('localhost:9991'); - expect(head).toBeTruthy(); - expect(tail).toBeFalsy(); - }); - - test('full host name is recognized', () => { - const [head, tail] = doSimpleCompletion('my-server.example.com'); - expect(head).toBeTruthy(); - expect(tail).toBeFalsy(); - }); - - test('full host and protocol are recognized', () => { - const [head, tail] = doSimpleCompletion('http://my-server.com'); - expect(head).toBeFalsy(); - expect(tail).toBeFalsy(); - }); - - test('fully explicit localhost is recognized', () => { - const [head, tail] = doSimpleCompletion('http://localhost:9991'); - expect(head).toBeFalsy(); - expect(tail).toBeFalsy(); - }); -}); - -describe('autocompleteRealm', () => { - const zulipData: AutocompletionDefaults = { - protocol: 'https://', - domain: 'zulipchat.com', - }; - - test('when no value is entered return empty string', () => { - const result = autocompleteRealm('', zulipData); - expect(result).toEqual(''); - }); - - test('when a protocol is provided, use it', () => { - const result = autocompleteRealm('http://example', zulipData); - expect(result).toEqual('http://example.zulipchat.com'); - }); - - test('do not use any other protocol than http and https', () => { - const result = autocompleteRealm('ftp://example', zulipData); - expect(result).toStartWith('https://ftp://'); - }); - - test('if the hostname contains a dot, consider it complete', () => { - const result = autocompleteRealm('mydomain.org', zulipData); - expect(result).toEqual('https://mydomain.org'); - }); - - test('if the hostname contains multiple dots, consider it complete', () => { - const result = autocompleteRealm('subdomain.mydomain.org', zulipData); - expect(result).toEqual('https://subdomain.mydomain.org'); - }); - - test('if the hostname contains a colon, consider it complete', () => { - const result = autocompleteRealm('localhost:9991', zulipData); - expect(result).toEqual('https://localhost:9991'); - }); -}); diff --git a/src/utils/url.js b/src/utils/url.js index 84912acf825..96fd4dac8d8 100644 --- a/src/utils/url.js +++ b/src/utils/url.js @@ -115,8 +115,6 @@ export type Protocol = 'https://' | 'http://'; const protocolRegex = /^\s*((?:http|https):\/\/)(.*)$/; -const hasProtocol = (url: string = '') => url.search(protocolRegex) !== -1; - // Split a (possible) URL into protocol and non-protocol parts. // The former will be null if no recognized protocol is a component // of the string. @@ -137,17 +135,6 @@ export const parseProtocol = (value: string): [Protocol | null, string] => { return [null, value]; }; -export const fixRealmUrl = (url: string = ''): string => { - if (url === '') { - return ''; - } - const trimmedUrl = url - .replace(/\s/g, '') // strip any spaces, internal or otherwise - .replace(/\/+$/, ''); // eliminate trailing slash(es) - - return hasProtocol(trimmedUrl) ? trimmedUrl : `https://${trimmedUrl}`; -}; - export const getFileExtension = (filename: string): string => filename.split('.').pop(); export const isUrlAnImage = (url: string): boolean => @@ -165,45 +152,3 @@ const mimes = { export const getMimeTypeFromFileExtension = (extension: string): string => mimes[extension.toLowerCase()] || 'application/octet-stream'; - -export type AutocompletionDefaults = {| - protocol: Protocol, - domain: string, -|}; - -export type AutocompletionPieces = [Protocol | null, string, string | null]; - -/** - * A short list of some characters not permitted in subdomain name elements. - */ -const disallowedCharacters: $ReadOnlyArray = [...'.:/']; - -/** - * Given user input purporting to identify a Zulip realm, provide a prefix, - * derived value, and suffix which may suffice to turn it into a full URL. - * - * Presently, the derived value will always be equal to the input value; - * this property should not be relied on, as it may change in future. - */ -export const autocompleteRealmPieces = ( - value: string, - defaults: AutocompletionDefaults, -): AutocompletionPieces => { - const [protocol, nonProtocolValue] = parseProtocol(value); - - const prefix = protocol === null ? defaults.protocol : null; - - // If the user supplies one of these characters, assume they know what they're doing. - const suffix = disallowedCharacters.some(c => nonProtocolValue.includes(c)) - ? null - : `.${defaults.domain}`; - - return [prefix, value, suffix]; -}; - -export const autocompleteRealm = (value: string, data: AutocompletionDefaults): string => - value === '' - ? '' - : autocompleteRealmPieces(value, data) - .filter(s => s) - .join('');