[Android][ListView] Add support ListView's onChangeVisibleRows for android

[terrysahaidak]

Motivation (required)

I'm building GitterMobile (Unofficial client for Gitter.im).
Because it's a chat application, there is an ability to mark visible messages as read. But because of onChangeVisibleRows work only in iOS, I've used some in-js check to assume if a view is visible. This is a poor performance approach. So I need it to be done in a native side.

There was a PR #11945 opened by @ptomasroos, but he decided to close it because of migration to brand new FlatList. It isn't a way I can go with because of bad virtualized list performance (you can check it out here by building and running my project).

Also, I'm using react-native-invertible-scroll-view, so it isn't compatible with flat list component. And ListView component just works. So, I think, we still need full featured ListView.

Test Plan (required)

Just use onChangeVisible rows on Android.

      <ListView
        ref="listView"
        childSizes={this.childHeights}
        onChangeVisibleRows={(a, b) => onChangeVisibleRows(a, b)}
        renderScrollComponent={props => <InvertibleScrollView {...props} inverted />}
        dataSource={listViewData.dataSource}
        onEndReached={this.props.onEndReached}
        scrollRenderAheadDistance={1000}
        onEndReachedThreshold={500}
        pageSize={30}
        initialListSize={30}
        renderRow={(rowData, _, rowId) => this.renderRow(rowData, rowId)} />

You can also try to build my GitterMobile where I'm using ListView and onChangeVisibleRows which work on iOS.

I can't really build react-native from source because of poor internet connection, so if anybody can test it please ping me.

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact cla@fb.com if you have any questions.

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[terrysahaidak]

Don't know why tests failed. It says package com.facebook.react.views.swiperefresh does not exist but it does exist.

[terrysahaidak]

Just tested it by myself in my Xiaomi mi5. Seems everything works well:

Green messages not marked as read yet, but messages with a white background have just marked as well.

[henrikra]

I would like to have this merged too ❤️

[terrysahaidak]

Since it's quite old pull request in general, it would be great if someone will look at it at all.

Sorry for spam, guys, but, please, give some feedback :)

/cc @janicduplessis @mkonicek @emilsjolander @astreet

[hramos]

As we've moved on to FlatList, I am not sure who can jump on this and get it reviewed.

[terrysahaidak]

As I've said before, FlatList's performance is not acceptable for my use case, so I still need to use legacy ListView component. I don't really want to manage and build my own fork of ReactNative for every release, moreover, this functionality is missed from the core of ReactNative, it isn't some sort of custom thing.

So I will really appreciate if someone will look at that PR. Thanks.

[terrysahaidak]

Sorry for spamming, guys, again. Can someone look at that PR?
It's already half-year-old PR 😢

/cc @javache @sahrens @ericvicenti @ide @janicduplessis @brentvatne @satya164 @mkonicek @emilsjolander @astreet

[vvavepacket]

@terrysahaidak while waiting for this to be merged,,, for the mean time, will copying the files you changed directly to my project work?

[hramos]

I'm going to go ahead and close this as no one has stepped up to review this. FlatList has supplanted ListView in core. You may consider shipping an improved ListView as a third party package instead.

[terrysahaidak]

Thank you guys for your help 😢

[vvavepacket]

@terrysahaidak i see that it works on your setup but when i checked the compile logs while building, some package is missing, i would love to try your changes on my machine and verify if it will work... im quite new here, will copying your changes on the java files to my machine resolves the issue?

[vvavepacket]

@terrysahaidak i tried to apply your java files changes to my machine, and then i tried running react-native run-android and it seems the onChangeVisibleRows is not triggered. are we missing any other prerequisites?

[vvavepacket]

i forgot one step, i need to rebuild my react native, will let u know since i have slow internet connection.

[terrysahaidak]

@vvavepacket you need to build react-native from source. Follow this instruction https://facebook.github.io/react-native/docs/android-building-from-source.html
It should work.

[vvavepacket]

@terrysahaidak thanks will do. whats the possibility of doing this as a 3rd party library or component since no one has expressed interest in reviewing and merging this into core?

[terrysahaidak]

@vvavepacket i don't think so. because of many core files of RN was edited in this PR. also, i'm not an android-guy. and it will be really hard to me to support it as a 3rd party library.

[terrysahaidak]

@vvavepacket you can use https://github.com/JSSolutions/GitterMobile/blob/master/libs/ListView/ExtendedListView.js as a workaround. it does checks in js side. bad performance, some lags and bugs. but at least it works.

[vvavepacket]

@terrysahaidak thanks, i have use it, but noticed that the onChangeVisibleRows() is triggered only once on startup... When i try to browse the list below, the callback is not called each time a row is rendered.

I tried inserting 20 rows with the following properties:

            <ListView
                dataSource={this.dataSource}
                renderRow={this.renderRow}
                onChangeVisibleRows={this.onChangeVisibleRows}
                initialListSize={1}
                pageSize={1}
            />

yet the onChangeVisibleRows() is not triggered everytime a row becomes out of view. Please advise. Thank you.

[terrysahaidak]

@vvavepacket sorry. forgot about some extra configuration.

Example:

class MyList extends Component {
  constructor(props) {
    // .. your constructor
    this.childSizes = {} // add this
  }

  handleOnLayout(e, rowId) {
    this.childSizes[+rowId] = e.nativeEvent.layout.height
  }

  onChangeVisibleRows(a, b) {
    console.log(a, b)
  }

  renderRow(rowData, rowId) {
    return (
      <View
        onLayout={e => this.handleOnLayout(e, rowId)}
        // other props
        >
        // other stuff
      </View>
    )
  }

  render() {
    return (
      <ListView
        dataSource={this.dataSource}
        renderRow={this.renderRow}
        childSizes={this.childSizes}
        onChangeVisibleRows={this.onChangeVisibleRows}
        initialListSize={30}
        pageSize={30} />
    )
  }
}
  

[vvavepacket]

@terrysahaidak Hi Terry, thanks it almost work but its kind of flaky.. When we say flaky, because my ListView is not fullscreen, (only half of screen), the onChangeVisibleRows() is triggered but the visible rows object from onChangeVisibleRows() emitted are incorrect. If i only see 2 rows on my listview, the onChangeVisibleRows() returns 4 rows which I assume is like correct for a fullscreen list view. Does it work perfectly for you?