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

Jump to bottom

Fix: Blurred images in attachment view on mobile devices #6442

Merged
merged 10 commits into from
Nov 26, 2021
Merged

Fix: Blurred images in attachment view on mobile devices #6442

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
da8b7ee
fix: aspect ratio calculation
aswin-s Nov 19, 2021
8543f59
Add react-native-fast-image & react-native-image-size library depende…
aswin-s Nov 23, 2021
7a72f8b
fix: Update component to use fast-image & react-native-fast-image
aswin-s Nov 23, 2021
a0e5b03
Merge branch 'main' into aswin-s/issue_5193
aswin-s Nov 23, 2021
45abbae
Merge branch 'main' into aswin-s/issue_5193
aswin-s Nov 23, 2021
733a114
Update lock file
aswin-s Nov 23, 2021
db9e2c7
Merge branch 'main' into aswin-s/issue_5193
aswin-s Nov 25, 2021
24a5536
Cancel promises on unmount
aswin-s Nov 25, 2021
cf3b7af
Merge branch 'main' into aswin-s/issue_5193
aswin-s Nov 26, 2021
d6b6055
Fix code style error
aswin-s Nov 26, 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
1 change: 1 addition & 0 deletions android/app/src/main/java/com/expensify/chat/MainApplication.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import androidx.multidex.MultiDexApplication;
import com.facebook.react.PackageList;
import com.facebook.react.ReactApplication;
import com.existfragger.rnimagesize.RNImageSizePackage;
import com.google.firebase.crashlytics.FirebaseCrashlytics;
import com.facebook.react.ReactInstanceManager;
import com.facebook.react.ReactNativeHost;
Expand Down
2 changes: 2 additions & 0 deletions android/settings.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
rootProject.name = 'NewExpensify'
include ':react-native-image-size'
project(':react-native-image-size').projectDir = new File(rootProject.projectDir, '../node_modules/react-native-image-size/android')
include ':react-native-config'
project(':react-native-config').projectDir = new File(rootProject.projectDir, '../node_modules/react-native-config/android')
apply from: file("../node_modules/@react-native-community/cli-platform-android/native_modules.gradle"); applyNativeModulesSettingsGradle(settings)
Expand Down
31 changes: 30 additions & 1 deletion ios/Podfile.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,15 @@ PODS:
- GoogleUtilities/Logger
- hermes-engine (0.7.2)
- libevent (2.1.12)
- libwebp (1.2.1):
- libwebp/demux (= 1.2.1)
- libwebp/mux (= 1.2.1)
- libwebp/webp (= 1.2.1)
- libwebp/demux (1.2.1):
- libwebp/webp
- libwebp/mux (1.2.1):
- libwebp/demux
- libwebp/webp (1.2.1)
- nanopb (2.30908.0):
- nanopb/decode (= 2.30908.0)
- nanopb/encode (= 2.30908.0)
Expand Down Expand Up @@ -509,6 +518,10 @@ PODS:
- React-Core
- RNDateTimePicker (3.5.2):
- React-Core
- RNFastImage (8.5.11):
- React-Core
- SDWebImage (~> 5.11.1)
- SDWebImageWebPCoder (~> 0.8.4)
- RNFBAnalytics (12.3.0):
- Firebase/Analytics (= 8.4.0)
- React-Core
Expand Down Expand Up @@ -563,6 +576,12 @@ PODS:
- React-Core
- RNSVG (12.1.0):
- React
- SDWebImage (5.11.1):
- SDWebImage/Core (= 5.11.1)
- SDWebImage/Core (5.11.1)
- SDWebImageWebPCoder (0.8.4):
- libwebp (~> 1.0)
- SDWebImage/Core (~> 5.10)
- urbanairship-react-native (11.0.2):
- Airship (= 14.4.2)
- React-Core
Expand Down Expand Up @@ -646,6 +665,7 @@ DEPENDENCIES:
- "RNCMaskedView (from `../node_modules/@react-native-masked-view/masked-view`)"
- "RNCPicker (from `../node_modules/@react-native-picker/picker`)"
- "RNDateTimePicker (from `../node_modules/@react-native-community/datetimepicker`)"
- RNFastImage (from `../node_modules/react-native-fast-image`)
- "RNFBAnalytics (from `../node_modules/@react-native-firebase/analytics`)"
- "RNFBApp (from `../node_modules/@react-native-firebase/app`)"
- "RNFBCrashlytics (from `../node_modules/@react-native-firebase/crashlytics`)"
Expand Down Expand Up @@ -685,12 +705,15 @@ SPEC REPOS:
- GoogleUtilities
- hermes-engine
- libevent
- libwebp
- nanopb
- Onfido
- OpenSSL-Universal
- Plaid
- PromisesObjC
- Protobuf
- SDWebImage
- SDWebImageWebPCoder
- YogaKit

EXTERNAL SOURCES:
Expand Down Expand Up @@ -794,6 +817,8 @@ EXTERNAL SOURCES:
:path: "../node_modules/@react-native-picker/picker"
RNDateTimePicker:
:path: "../node_modules/@react-native-community/datetimepicker"
RNFastImage:
:path: "../node_modules/react-native-fast-image"
RNFBAnalytics:
:path: "../node_modules/@react-native-firebase/analytics"
RNFBApp:
Expand Down Expand Up @@ -825,7 +850,7 @@ SPEC CHECKSUMS:
CocoaAsyncSocket: 065fd1e645c7abab64f7a6a2007a48038fdc6a99
DoubleConversion: cf9b38bf0b2d048436d9a82ad2abe1404f11e7de
FBLazyVector: 7b423f9e248eae65987838148c36eec1dbfe0b53
FBReactNativeSpec: 1d564cbdef3e1546843d1f1ceb0e4463b7993e3a
FBReactNativeSpec: 3930028959767285f556bd187e13b896deee31fe
Firebase: 54cdc8bc9c9b3de54f43dab86e62f5a76b47034f
FirebaseABTesting: c3e48ebf5e7e5c674c5a131c68e941d7921d83dc
FirebaseAnalytics: 4751d6a49598a2b58da678cc07df696bcd809ab9
Expand All @@ -849,6 +874,7 @@ SPEC CHECKSUMS:
GoogleUtilities: 3df19e3c24f7bbc291d8b5809aa6b0d41e642437
hermes-engine: 7d97ba46a1e29bacf3e3c61ecb2804a5ddd02d4f
libevent: 4049cae6c81cdb3654a443be001fb9bdceff7913
libwebp: 98a37e597e40bfdb4c911fc98f2c53d0b12d05fc
nanopb: a0ba3315591a9ae0a16a309ee504766e90db0c96
Onfido: 116a268e4cb8b767c15285e8071c2e8304673cdf
onfido-react-native-sdk: b8f1b7cbe1adab6479d735275772390161630dcd
Expand Down Expand Up @@ -900,6 +926,7 @@ SPEC CHECKSUMS:
RNCMaskedView: 138134c4d8a9421b4f2bf39055a79aa05c2d47b1
RNCPicker: 6780c753e9e674065db90d9c965920516402579d
RNDateTimePicker: c9911be59b1f8670b9f244b85af3a7c295e175ed
RNFastImage: 1f2cab428712a4baaf78d6169eaec7f622556dd7
RNFBAnalytics: 8ba84c2d31c64374d054c8621b998f25145ffddc
RNFBApp: 64c90ab78b6010ed5c3ade026dfe5ff6442c21fd
RNFBCrashlytics: 1de18b8cc36d9bcf86407c4a354399228cc84a61
Expand All @@ -910,6 +937,8 @@ SPEC CHECKSUMS:
RNReanimated: 833ebd229b31e18a8933ebd0cd744a0f47d88c42
RNScreens: e8e8dd0588b5da0ab57dcca76ab9b2d8987757e0
RNSVG: ce9d996113475209013317e48b05c21ee988d42e
SDWebImage: a7f831e1a65eb5e285e3fb046a23fcfbf08e696d
SDWebImageWebPCoder: f93010f3f6c031e2f8fb3081ca4ee6966c539815
urbanairship-react-native: 60b4b4235838ff109a2639b639e2ef01d54ad455
Yoga: a7de31c64fe738607e7a3803e3f591a4b1df7393
YogaKit: f782866e155069a2cca2517aafea43200b01fd5a
Expand Down
10 changes: 10 additions & 0 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,11 +79,13 @@
"react-native-collapsible": "^1.6.0",
"react-native-config": "^1.4.0",
"react-native-document-picker": "^5.1.0",
"react-native-fast-image": "^8.5.11",
"react-native-gesture-handler": "1.9.0",
"react-native-google-places-autocomplete": "^2.4.1",
"react-native-haptic-feedback": "^1.13.0",
"react-native-image-pan-zoom": "^2.1.12",
"react-native-image-picker": "^4.0.3",
"react-native-image-size": "^1.1.3",
"react-native-keyboard-spacer": "^0.4.1",
"react-native-modal": "^11.10.0",
"react-native-onyx": "git+https://github.com/Expensify/react-native-onyx.git#00bcc1520cf6cf7846d8aa40b5161bd1f407341f",
Expand Down
89 changes: 69 additions & 20 deletions src/components/ImageView/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
import React, {PureComponent} from 'react';
import PropTypes from 'prop-types';
import {View, PanResponder} from 'react-native';
import {View, InteractionManager, PanResponder} from 'react-native';
import Image from 'react-native-fast-image';
import ImageZoom from 'react-native-image-pan-zoom';
import ImageSize from 'react-native-image-size';
import _ from 'underscore';
import ImageWithSizeCalculation from '../ImageWithSizeCalculation';
import styles from '../../styles/styles';
import * as StyleUtils from '../../styles/styles';
import * as StyleUtils from '../../styles/StyleUtils';
import variables from '../../styles/variables';
import withWindowDimensions, {windowDimensionsPropTypes} from '../withWindowDimensions';

Expand All @@ -24,8 +25,11 @@ class ImageView extends PureComponent {
super(props);

this.state = {
imageWidth: 100,
imageHeight: 100,
thumbnailWidth: 100,
thumbnailHeight: 100,
imageWidth: undefined,
imageHeight: undefined,
interactionPromise: undefined,
};

// Use the default double click interval from the ImageZoom library
Expand All @@ -41,6 +45,34 @@ class ImageView extends PureComponent {
});
}

componentDidMount() {
// Wait till animations are over to prevent stutter in navigation animation
this.state.interactionPromise = InteractionManager.runAfterInteractions(() => this.calculateImageSize());
Comment on lines +49 to +50
Copy link
Contributor

Choose a reason for hiding this comment

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

@aswin-s Do you recall the production steps for the animation bug here? We are looking to remove InteractionManager.runAfterInteractions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wow this was over 2 years ago. If I remember correctly trying to calculate image size while animating the modal was causing frozen frames on android device. But this component has gone through multiple refactors after this change and even react-native-fast-image was forked to improve load performance. So this optimisation might not be required anymore.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you!

}

componentWillUnmount() {
if (!this.state.interactionPromise) {
return;
}
this.state.interactionPromise.cancel();
}

calculateImageSize() {
if (!this.props.url) {
return;
}
ImageSize.getSize(this.props.url).then(({width, height}) => {
let imageWidth = width;
let imageHeight = height;
const aspectRatio = (imageHeight / imageWidth);
if (imageWidth > this.props.windowWidth) {
imageWidth = Math.round(this.props.windowWidth);
imageHeight = Math.round(imageWidth * aspectRatio);
}
this.setState({imageHeight, imageWidth});
});
}

/**
* Updates the amount of active touches on the PanResponder on our ImageZoom overlay View
*
Expand All @@ -60,6 +92,30 @@ class ImageView extends PureComponent {
render() {
// Default windowHeight accounts for the modal header height
const windowHeight = this.props.windowHeight - variables.contentHeaderHeight;

// Display thumbnail until Image size calculation is complete
if (!this.state.imageWidth || !this.state.imageHeight) {
return (
<View
style={[
styles.w100,
styles.h100,
styles.alignItemsCenter,
styles.justifyContentCenter,
styles.overflowHidden,
styles.errorOutline,
]}
>
<Image
source={{uri: this.props.url}}
style={StyleUtils.getWidthAndHeightStyle(this.state.thumbnailWidth, this.state.thumbnailHeight)}
resizeMode={Image.resizeMode.contain}
/>
</View>
);
}

// Zoom view should be loaded only after measuring actual image dimensions, otherwise it causes blurred images on Android
return (
<View
style={[
Expand Down Expand Up @@ -103,21 +159,14 @@ class ImageView extends PureComponent {
this.imageZoomScale = scale;
}}
>
<ImageWithSizeCalculation
Copy link
Contributor

Choose a reason for hiding this comment

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

hmmm I wonder if we should move all this logic to this component instead... what's the value of keeping both?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ImageWithSizeCalculation component is used in Web & Desktop platforms as well and current implementation works just fine for those. Keeping the logic contained in ImageView native implementation, we basically keep the unnecessary dependencies out of web bundle & avoid webpack errors. If you still prefer to keep it in ImageSizeWithCalculation then it needs to have a native implementation which uses FastImage & RNImageSize.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh ok ok, no, this is good.

style={StyleUtils.getWidthAndHeightStyle(this.state.imageWidth, this.state.imageHeight)}
url={this.props.url}
onMeasure={({width, height}) => {
let imageWidth = width;
let imageHeight = height;

if (width > this.props.windowWidth || height > windowHeight) {
const scaleFactor = Math.max(width / this.props.windowWidth, height / windowHeight);
imageHeight = height / scaleFactor;
imageWidth = width / scaleFactor;
}

this.setState({imageHeight, imageWidth});
}}
<Image
style={[
styles.w100,
styles.h100,
this.props.style,
]}
source={{uri: this.props.url}}
resizeMode={Image.resizeMode.contain}
/>
{/**
Create an invisible view on top of the image so we can capture and set the amount of touches before
Expand Down