Skip to content

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

Closed
terrysahaidak wants to merge 16 commits into
react:masterfrom
terrysahaidak:onChangeVisibleRows-android-support
Closed

[Android][ListView] Add support ListView's onChangeVisibleRows for android#13532
terrysahaidak wants to merge 16 commits into
react:masterfrom
terrysahaidak:onChangeVisibleRows-android-support

Conversation

@terrysahaidak

@terrysahaidak terrysahaidak commented Apr 16, 2017

Copy link
Copy Markdown

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

Copy link
Copy Markdown
Contributor

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 facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Apr 16, 2017
@facebook-github-bot

Copy link
Copy Markdown
Contributor

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

@terrysahaidak

Copy link
Copy Markdown
Author

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

@terrysahaidak

Copy link
Copy Markdown
Author

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

image

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

@terrysahaidak terrysahaidak mentioned this pull request Apr 17, 2017
21 tasks
@henrikra

Copy link
Copy Markdown

I would like to have this merged too ❤️

@terrysahaidak

Copy link
Copy Markdown
Author

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

hramos commented Jun 1, 2017

Copy link
Copy Markdown
Contributor

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

@terrysahaidak

Copy link
Copy Markdown
Author

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

Copy link
Copy Markdown
Author

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

Copy link
Copy Markdown

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

@hramos

hramos commented Jul 6, 2017

Copy link
Copy Markdown
Contributor

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.

@hramos hramos closed this Jul 6, 2017
@terrysahaidak

Copy link
Copy Markdown
Author

Thank you guys for your help 😢

@vvavepacket

vvavepacket commented Jul 6, 2017

Copy link
Copy Markdown

@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

Copy link
Copy Markdown

@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

Copy link
Copy Markdown

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

@terrysahaidak

Copy link
Copy Markdown
Author

@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

Copy link
Copy Markdown

@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

terrysahaidak commented Jul 7, 2017

Copy link
Copy Markdown
Author

@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

Copy link
Copy Markdown
Author

@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

Copy link
Copy Markdown

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

image

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

terrysahaidak commented Jul 7, 2017

Copy link
Copy Markdown
Author

@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

Copy link
Copy Markdown

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.