-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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.
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
feat: move clearing logic to onDetachedFromWindow #773
Conversation
@WoLewicki Are you sure this solution is OK? I have in app: react-navigation v4 and when I change screens plugged into bottomStackNavigator, then onDetachedFromWindow cleans images that are still on these screens. Maybe we should bind onDestroyView event here? |
Can you post a reproduction here? I am not 100% sure if this solution is 100% correct. |
I have created a repro here in Example on master: https://github.com/ku8ar/react-native-screens Commit with changes: ku8ar/react-native-screens@4e2114c |
I changed the implementation to handle loading/clearing of the images in |
When image is generated in |
Yeah, we should probably keep the logic of loading in setting the source too. I will change the implementation to an easier one and I will ping you then. |
any progress on this? |
@WoLewicki checked. Looks ok. Thanks! |
can you please merge it @DylanVann |
bump, would be good to have this merged |
@WoLewicki Having flickering issues with this fork, here's some example code import React from 'react';
import {NavigationContainer} from '@react-navigation/native';
import {createMaterialTopTabNavigator} from '@react-navigation/material-top-tabs';
import {ScrollView} from 'react-native';
import Image from 'react-native-fast-image';
const Tab1 = () => {
return null;
};
const Tab2 = () => {
return (
<ScrollView style={{margin: 10}}>
<Image
style={{
width: 600,
height: 600,
}}
source={{
uri: 'https://mht.wtf/post/content-aware-resize/sample-image.jpeg',
}}
/>
<Image
style={{
width: 600,
height: 600,
}}
source={{
uri: 'https://encrypted-tbn0.gstatic.com/images?q=tbn:ANd9GcT_12mZYw9c0QfCb875croxaX5vufbLRdrXLRhudSwVmHtuSKYOPise1ZTKQgl-_QzGdJc&usqp=CAU',
}}
/>
</ScrollView>
);
};
const Tab3 = () => {
return null;
};
const App = () => {
const Tab = createMaterialTopTabNavigator();
return (
<NavigationContainer>
<Tab.Navigator initialRouteName="Tab2">
<Tab.Screen name="Tab1" component={Tab1} />
<Tab.Screen name="Tab2" component={Tab2} />
<Tab.Screen name="Tab3" component={Tab3} />
</Tab.Navigator>
</NavigationContainer>
);
};
export default App; if you test going back and forth from tab1 and tab2 you'll notice that the first image rendered always flickers |
@moulie415 I tested the code and did not spot any flickering. Could you provide a video where it is clearly visible? Also, you are using |
@WoLewicki I've managed to repro with both bottom and material top tabs 21-08-18-10-56-47.mp421-08-18-10-56-13.mp4Here's my package.json {
"name": "test",
"version": "0.0.1",
"private": true,
"scripts": {
"android": "react-native run-android",
"ios": "react-native run-ios",
"start": "react-native start",
"test": "jest",
"lint": "eslint ."
},
"dependencies": {
"@react-navigation/bottom-tabs": "^6.0.5",
"@react-navigation/material-top-tabs": "^6.0.2",
"@react-navigation/native": "^6.0.2",
"react": "17.0.2",
"react-native": "0.65.0",
"react-native-fast-image": "git@github.com:WoLewicki/react-native-fast-image.git",
"react-native-pager-view": "^5.4.0",
"react-native-safe-area-context": "^3.3.0",
"react-native-screens": "^3.5.0",
"react-native-tab-view": "^3.1.1"
},
"devDependencies": {
"@babel/core": "^7.12.9",
"@babel/runtime": "^7.12.5",
"@react-native-community/eslint-config": "^2.0.0",
"babel-jest": "^26.6.3",
"eslint": "7.14.0",
"jest": "^26.6.3",
"metro-react-native-babel-preset": "^0.66.0",
"react-native-codegen": "^0.0.7",
"react-test-renderer": "17.0.1"
},
"jest": {
"preset": "react-native"
}
} |
Hmm it is probably caused by the fact that the whole tab is detached from the native view hierarchy when the tab is changed with |
@WoLewicki yeh that works however "react-native-fast-image": "^8.3.7" is fine regardless of what you use. |
Yeah, I don't know an easy way to fix the bug and make it work correctly. I am open to suggestions. |
What about this? I know too little about how things work but this seemed to fix the original issue without breaking the other one
essentially running the requestManager.clear() on UI thread I verified that
|
@chr4ss12 I also have not enough knowledge about this, but if it works, please make a PR with it and link it here so people can easily test it. Maybe it is a better solution to the problem. |
Moved the logic of clearing views from
onDropViewInstance
, which is not the proper place for it since the view can be dropped, but still visible for during the transition (see software-mansion/react-native-screens#797 and software-mansion/react-native-screens#773). They have been moved toonDetachedFromWindow
, which is the moment the view is not present in the window of the app. It required passing therequestManager
to theFastImageViewWithUrl
instance.