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

(Android only) onVisibleIndexesChanged callback not always firing #103

Closed
SudoPlz opened this issue Jan 10, 2018 · 12 comments · May be fixed by #104
Closed

(Android only) onVisibleIndexesChanged callback not always firing #103

SudoPlz opened this issue Jan 10, 2018 · 12 comments · May be fixed by #104

Comments

@SudoPlz
Copy link

SudoPlz commented Jan 10, 2018

Hey there I have no idea what causes this but even in 1.2.6:

  • there are times even I change the month no events are being fired.

I debugged a little bit and found that the reason is that when that recycler calculates the visible indices, the old visible indices array is the same with the new indices array, so nothing gets dispatched.

  • ALSO when the recyclerlistview is first mounted, I receive 2 onVisibleIndexesChanged events. The first one is right (now only contains 1 item), and the second one is wrong (contains 2 items).

Ever seen that?

By the way both of those issues only happen on Android (I think).

@SudoPlz SudoPlz changed the title Value changed callback not always firing (Android only) Value changed callback not always firing Jan 10, 2018
@SudoPlz SudoPlz changed the title (Android only) Value changed callback not always firing (Android only) onVisibleIndexesChanged callback not always firing Jan 10, 2018
@SudoPlz
Copy link
Author

SudoPlz commented Jan 10, 2018

Ok I did some digging and found that the second issue is because of decimal points here:

return (window.start <= itemBound && window.end >= itemBound);

In my case the window.end was 100.71469 and the itemBound was 100.71428 and therefore for 0.00041 pixels the next screen was considered to be visible. Rounding to 1 (or even 0) decimal value should fix the issue, I'll open a PR.

About the first bullet, I still don't know I'll keep on digging.

SudoPlz added a commit to SudoPlz/recyclerlistview that referenced this issue Jan 10, 2018
- No longer receiving 2 onVisibleIndexesChanged callbacks
- No longer accidentally skipping onVisibleIndexesChanged callbacks
@naqvitalha
Copy link
Contributor

Are you using non deterministic rendering? Rounding off will be a double edged sword because it might fix your use case and break others. Can you share your layout provider implementation?

@SudoPlz
Copy link
Author

SudoPlz commented Jan 11, 2018

@naqvitalha The issue is clearly visible on this snack https://snack.expo.io/S1OeHVAQM (if you run it on an Android device).

  • First of all the first month visible, is February, when it should in fact be January.
  • Second, if you try to go from February to March, the title will be stuck on February.

Also you can check the layout provider on that link too, but here is the actual code:

this.layoutProvider = new LayoutProvider(
      (index) => {
        const date = this.indexToDate(index);
        if (date) {
          const miniCalMonthMoment = momentFromDateWithoutTimezone(date);
          if (miniCalMonthMoment) {
            const monthKey = miniCalMonthMoment.format(TimeFormats.MINICAL_KEY_FORMAT);
            if (props.miniCalData) {
              const dayData = props.miniCalData.getIn([monthKey, 'data']);
              if (dayData) {
                const weekCnt = Math.ceil(dayData.size / 7);
                if (weekCnt >= 6) {
                  this.minicalHeight = props.h * (props.showMonthTitles ? 0.42 : 0.37);
                  return VIEW_TYPES.LARGE_SIZE;
                }
              }
            }
          }
        }
        return VIEW_TYPES.NORMAL_SIZE;
      },

      // Providing heights and widths for given types, doing deterministic way for perf
      (type, dim) => {
        switch (type) {
          case VIEW_TYPES.LARGE_SIZE: {
            // eslint-disable-next-line
            dim.height = props.h * (props.showMonthTitles ? 0.4 : 0.35);
            break;
          }
          case VIEW_TYPES.NORMAL_SIZE:
          default: {
            // eslint-disable-next-line
            dim.height = props.h * (props.showMonthTitles ? 0.48 : 0.42);
            break;
          }
        }
        // eslint-disable-next-line
        dim.width = this.minicalWidth;
      },
    );

@naqvitalha
Copy link
Contributor

I tried the expo. First visible month was January. And I don't even get the second issue. What is happening? Device specific issue?

@SudoPlz
Copy link
Author

SudoPlz commented Jan 12, 2018

I'm running this on a Nexus 5X @ 8.1.0, and I don't think it's device specific issue (we had lot's of users reporting the same behaviour on various devices) as we're using that code on production.

  • First issue is that the first month is January (because there are 2 onVisibleIndexesChanged being dispatched upon mount (the first one is the right one and the second one is wrong).

  • The second issue is that on February, if you try to scroll from February to March, the title will still be February even though March is been shown on the minical. Under the hood what happens is that both the old and new visible indices are the same, thus:

if (now.length > 0 || notNow.length > 0) {

both now and notNow are empty.

Truncating the number after the decimal point fixes both issues for us. What side issues do you think that might create btw?

@naqvitalha
Copy link
Contributor

Check slack once. I've posted a video of my experience

@naqvitalha
Copy link
Contributor

Truncating is a problem in non deterministic rendering where item widths and heights can be very irregular. It would be very hard to figure out if that rounding off will lead to right or wrong. E.g in our search page a lot of time our rendered item heights are like 120.56003... Assuming anything is very hard there

@SudoPlz
Copy link
Author

SudoPlz commented Jan 12, 2018

I'm not in front of slack atm, I'll join you in ~40 mins.

@naqvitalha
Copy link
Contributor

Any update?

@SudoPlz
Copy link
Author

SudoPlz commented Feb 15, 2018

Not really, I still get that issue. It's a real pain, and I'm using hacks to determine what's visible, and that doesn't always return the right index.
Not sure what I can do. Is there anything I can do on my end to help get to the bottom of this?

@SudoPlz
Copy link
Author

SudoPlz commented Apr 7, 2018

So the repo I've created for #145 is the same project I've been having trouble with onVisibleIndexesChanged.

@naqvitalha
Copy link
Contributor

Closing due to inactivity.

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 a pull request may close this issue.

2 participants