Skip to content
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

Bugfix: ExceptionsManagerModule android crashes #968

Merged
merged 9 commits into from
Jul 30, 2019
+43 −35
Merged

Bugfix: ExceptionsManagerModule android crashes #968

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
c164a8e
issue 1
estebanmino Jul 30, 2019
3a68846
more like issue 1
estebanmino Jul 30, 2019
05fc1b2
fix website icon
estebanmino Jul 30, 2019
0764118
animations
estebanmino Jul 30, 2019
50501fe
rm code
estebanmino Jul 30, 2019
3110d6e
comment
estebanmino Jul 30, 2019
8227295
Merge branch 'develop' into bugfix/android-crashes
estebanmino Jul 30, 2019
202e18c
rm state
estebanmino Jul 30, 2019
432b98d
Merge branch 'develop' into bugfix/android-crashes
Jul 30, 2019
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
9 changes: 5 additions & 4 deletions app/components/UI/AccountApproval/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,11 +237,12 @@ class AccountApproval extends PureComponent {

render = () => {
const {
currentPageInformation: { title, url },
currentPageInformation: { url },
currentPageInformation,
selectedAddress,
identities
} = this.props;
const host = getHost(url);
const title = typeof currentPageInformation.title === 'string' ? currentPageInformation.title : getHost(url);
Copy link
Contributor

Choose a reason for hiding this comment

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

I literally made this change last night and it didn't go to production yet.
See https://github.com/MetaMask/metamask-mobile/pull/963/files#diff-6b966e9916d92d54ae676b69d898840cR244

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, title was the problem not host. I was able to get to some cases where title was {} making everything crash

return (
<View style={styles.root}>
<View style={styles.titleWrapper}>
Expand All @@ -259,9 +260,9 @@ class AccountApproval extends PureComponent {
<View style={styles.wrapper}>
<View style={styles.header}>
<View style={styles.dapp}>
<WebsiteIcon style={styles.icon} title={title || host} url={url} />
<WebsiteIcon style={styles.icon} title={title} url={url} />
<Text style={styles.headerTitle} numberOfLines={1}>
{title || host}
{title}
</Text>
<Text style={styles.headerUrl} numberOfLines={1}>
{url}
Expand Down
8 changes: 6 additions & 2 deletions app/components/UI/SignatureRequest/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import WebsiteIcon from '../WebsiteIcon';
import { renderAccountName } from '../../../util/address';
import Analytics from '../../../core/Analytics';
import ANALYTICS_EVENT_OPTS from '../../../util/analytics';
import { getHost } from '../../../util/browser';

const styles = StyleSheet.create({
wrapper: {
Expand Down Expand Up @@ -148,14 +149,17 @@ class SignatureRequest extends PureComponent {
renderPageInformation = () => {
const {
domain,
currentPageInformation: { title, url }
currentPageInformation: { url },
currentPageInformation
} = this.props;
const title = typeof currentPageInformation.title === 'string' ? currentPageInformation.title : getHost(url);
const name = domain && typeof domain.name === 'string';
return (
<View style={styles.domainWrapper}>
<WebsiteIcon style={styles.domainLogo} viewStyle={styles.assetLogo} title={title} url={url} />
<Text style={styles.domainTitle}>{title}</Text>
<Text style={styles.domainText}>{url}</Text>
{domain && <Text style={styles.domainText}>{domain.name}</Text>}
{name && <Text style={styles.domainText}>{name}</Text>}
</View>
);
};
Expand Down
36 changes: 22 additions & 14 deletions app/components/UI/UrlAutocomplete/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,21 +139,29 @@ class UrlAutocomplete extends PureComponent {

onSubmitInput = () => this.props.onSubmit(this.props.input);

renderUrlOption = (url, name, onPress) => (
<TouchableOpacity containerStyle={styles.item} onPress={onPress} key={url}>
<View style={styles.itemWrapper}>
<WebsiteIcon style={styles.bookmarkIco} url={url} title={name} textStyle={styles.fallbackTextStyle} />
<View style={styles.textContent}>
<Text style={styles.name} numberOfLines={1}>
{name || getHost(url)}
</Text>
<Text style={styles.url} numberOfLines={1}>
{url}
</Text>
renderUrlOption = (url, name, onPress) => {
name = typeof name === 'string' ? name : getHost(url);
return (
<TouchableOpacity containerStyle={styles.item} onPress={onPress} key={url}>
<View style={styles.itemWrapper}>
<WebsiteIcon
style={styles.bookmarkIco}
url={url}
title={name}
textStyle={styles.fallbackTextStyle}
/>
<View style={styles.textContent}>
<Text style={styles.name} numberOfLines={1}>
{name}
</Text>
<Text style={styles.url} numberOfLines={1}>
{url}
</Text>
</View>
</View>
</View>
</TouchableOpacity>
);
</TouchableOpacity>
);
};

render() {
if (this.props.input.length < 2) return <View style={styles.wrapper} />;
Expand Down
18 changes: 13 additions & 5 deletions app/components/UI/WebsiteIcon/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,18 @@ export default class WebsiteIcon extends PureComponent {
};

state = {
renderIconUrlError: false
renderIconUrlError: false,
apiLogoUrl: undefined,
title: undefined
};

componentDidMount() {
const { url } = this.props;
const apiLogoUrl = { uri: this.getIconUrl(url) };
const title = typeof this.props.title === 'string' ? this.props.title : getHost(url);
this.setState({ title: title.substring(0, 1).toUpperCase(), apiLogoUrl });
Copy link
Contributor

Choose a reason for hiding this comment

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

I oppose to this change because it's gonna cause an extra re-render in every website icon, no matter if it was problematic or not.

My suggestion is to can keep those in the render method and use the this prop to capitalize: https://facebook.github.io/react-native/docs/text-style-props#texttransform

}

/**
* Get image url from favicon api
*/
Expand All @@ -74,15 +83,14 @@ export default class WebsiteIcon extends PureComponent {
};

render = () => {
const { renderIconUrlError } = this.state;
const { url, viewStyle, style, title, textStyle, transparent } = this.props;
const apiLogoUrl = { uri: this.getIconUrl(url) };
const { renderIconUrlError, title, apiLogoUrl } = this.state;
const { viewStyle, style, textStyle, transparent } = this.props;

if (renderIconUrlError && title) {
return (
<View style={viewStyle}>
<View style={[styles.fallback, style]}>
<Text style={[styles.fallbackText, textStyle]}>{title.substring(0, 1).toUpperCase()}</Text>
<Text style={[styles.fallbackText, textStyle]}>{title}</Text>
</View>
</View>
);
Expand Down
13 changes: 3 additions & 10 deletions app/components/Views/Entry/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,9 +115,9 @@ class Entry extends PureComponent {
this.setState({ viewToGo: view }, () => {
if (Platform.OS === 'android') {
setTimeout(() => {
this.animation.play(0, 25);
this.animation ? this.animation.play(0, 25) : this.onAnimationFinished();
setTimeout(() => {
this.animationName.play();
this.animationName && this.animationName.play();
}, 1);
}, 50);
} else {
Expand Down Expand Up @@ -179,14 +179,7 @@ class Entry extends PureComponent {

renderAnimations() {
if (!this.state.viewToGo) {
return (
<LottieView
// eslint-disable-next-line react/jsx-no-bind
style={styles.animation}
autoPlay
source={require('../../../animations/bounce.json')}
/>
);
return <LottieView style={styles.animation} autoPlay source={require('../../../animations/bounce.json')} />;
}

return (
Expand Down
4 changes: 2 additions & 2 deletions app/components/Views/LockScreen/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,9 +119,9 @@ export default class LockScreen extends PureComponent {
Logger.log('Lockscreen::unlockKeychain - state: ready');
if (Platform.OS === 'android') {
setTimeout(() => {
this.secondAnimation.play(0, 25);
this.secondAnimation ? this.secondAnimation.play(0, 25) : this.onAnimationFinished();
setTimeout(() => {
this.animationName.play();
this.animationName && this.animationName.play();
}, 1);
}, 50);
} else {
Expand Down