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.

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

Closed
wants to merge 0 commits into from

Conversation

WoLewicki
Copy link

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 to onDetachedFromWindow, which is the moment the view is not present in the window of the app. It required passing the requestManager to the FastImageViewWithUrl instance.

@ku8ar
Copy link

ku8ar commented Mar 1, 2021

@WoLewicki Are you sure this solution is OK? I have in app:

react-navigation v4
react-native-screens 2.16

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?

@WoLewicki
Copy link
Author

Can you post a reproduction here? I am not 100% sure if this solution is 100% correct.

@ku8ar
Copy link

ku8ar commented Mar 1, 2021

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

ff_broken

@WoLewicki
Copy link
Author

I changed the implementation to handle loading/clearing of the images in onAttachedToWindow and onDetachedFromWindow. Can you check if it fixes the issues? Or maybe I am missing something here too?

@ku8ar
Copy link

ku8ar commented Mar 4, 2021

When image is generated in onAttachedToWindow, it is not possible to dynamically change source. We need to recreate a new fast-image object. Right?

@WoLewicki
Copy link
Author

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.

@scvgoe
Copy link

scvgoe commented Jun 7, 2021

any progress on this?

@WoLewicki
Copy link
Author

I added loading the image on setting the source, can you test if all is ok now @ku8ar @scvgoe?

@ku8ar
Copy link

ku8ar commented Jun 10, 2021

@WoLewicki checked. Looks ok. Thanks!

@byteab
Copy link

byteab commented Jun 19, 2021

can you please merge it @DylanVann

@chr4ss12
Copy link

bump, would be good to have this merged

@moulie415
Copy link

moulie415 commented Aug 17, 2021

@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

@WoLewicki
Copy link
Author

@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 createMaterialTopTabNavigator which does not use react-native-screens underneath, so there may be something connected to this package that does not work well with this PR.

@moulie415
Copy link

moulie415 commented Aug 18, 2021

@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 createMaterialTopTabNavigator which does not use react-native-screens underneath, so there may be something connected to this package that does not work well with this PR.

@WoLewicki I've managed to repro with both bottom and material top tabs

21-08-18-10-56-47.mp4
21-08-18-10-56-13.mp4

Here'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"
  }
}

@WoLewicki
Copy link
Author

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 react-native-screens enabled. Can you spot flickering when you don't use the react-native-screens in this navigator (set https://reactnavigation.org/docs/bottom-tab-navigator/#detachinactivescreens to false)?

@moulie415
Copy link

@WoLewicki yeh that works however "react-native-fast-image": "^8.3.7" is fine regardless of what you use.

@WoLewicki
Copy link
Author

Yeah, I don't know an easy way to fix the bug and make it work correctly. I am open to suggestions.

@chr4ss12
Copy link

chr4ss12 commented Aug 30, 2021

@WoLewicki

What about this? I know too little about how things work but this seemed to fix the original issue without breaking the other one

    @Override
    public void onDropViewInstance(final FastImageViewWithUrl view) {
        // This will cancel existing requests.
        if (requestManager != null) {
            runOnUiThread(
                new Runnable() {
                @Override
                public void run() {
                    requestManager.clear(view);
                }
            });
        }


      ....

essentially running the requestManager.clear() on UI thread

I verified that

  1. requestManager.clear() is getting called by just logging to adb
  2. changed animation time to a 3 seconds and requestManager.clear() gets called after the whole animation is done

@WoLewicki
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants