Skip to content
This repository has been archived by the owner on Feb 2, 2023. It is now read-only.

Improve our batch fetching cases #1681

Closed
nickvelloff opened this issue May 19, 2016 · 6 comments
Closed

Improve our batch fetching cases #1681

nickvelloff opened this issue May 19, 2016 · 6 comments
Milestone

Comments

@nickvelloff
Copy link
Contributor

nickvelloff commented May 19, 2016

There is a use cases that might make sense to support a batch fetching update/enhancement:

If all loaded items are within visible bounds, fetching never triggers because this is triggered by a scroll event. If I have 10 of 100 items loaded, then pinch to zoom a collection view where all 10 fit nicely the trigger to fetch additional items will never get called.

This would commonly occur on pinch/zoom out with a collection view.

cc /@maicki

@flovouin
Copy link
Contributor

I mentioned something similar a few days ago on the ASDK Slack, so I would be interested in any comment from the team. :)
Batch fetching also isn't triggered when the contentOffset of the scroll view is changed programmatically, for example simply by setting the contentOffset property, or when calling scrollRectToVisible. At the moment only item manipulations (insert, remove, etc) seem to trigger a batch fetch.

@flovouin
Copy link
Contributor

Hi guys, sorry for uping this issue once again, but I'd rather do this than create a new one. ;-) My use case is slightly different compared to nickvelloff, but it fits under "batch fetching cases". Please tell me if you'd like me to create a separate issue.

Basically I have an ASTableNode (or ASCollectionView) with scrollEnabled = false on the view. The displayed content location is changed programatically by setting the contentOffset of the scroll view, or by calling scrollRectToVisible. If batch fetching could also work in these cases, it would really help me out. Right now I would have to reimplement some kind of batch fetching mechanism to obtain the same behaviour, which (partly) defeats the purpose of using the ASTableNode in the first place.

I do realise that this would imply some changes to the current batch fetching mechanism, so my request is more of a "would that be doable / reasonable changes, or should I look for an alternative way of achieving this?".

As always, thanks for the awesome framework :-)
Cheers

@appleguy
Copy link
Contributor

@flovouin This is a completely valid use case, and we should support it. Until seeing your note, I simply didn't realize this wasn't working correctly. Thank you for pinging the issue (it is clear you are constructive and respectful, please don't hesitate to report or ping issues if we are slow to respond!)

The team is quite busy right now preparing for our Tuesday event (WWDC-week gathering for ASDK users to discuss the future of the framework and profile apps). We do have two new full time framework developers starting at Pinterest this week.

It will probably take a few weeks for us to get to this issue, but, it should not be extremely complex if you are open to looking at the implementation and finding the right override points to trigger a "re-check" of the batch parameters. There should already be methods that make this a lot easier, things like "batch fetch if necessary" that are not too expensive to call if a batch doesn't need to be triggered. The task is probably as simple as calling the appropriate method more frequently, specifically in the cases of a programmatic scroll.

@flovouin
Copy link
Contributor

Thank you for your reply, appleguy!
No hurry, this is more of a "preemptive" issue, as I know I will need this in a few weeks. ;-) Have fun at the WWDC.
I did look at the code and I indeed found a simple solution, see #1744. If there's anything else I can do to help concerning this issue, please tell me.

@hannahmbanana
Copy link
Contributor

@nickvelloff: Was this issue resolved in PR #1697?

@nickvelloff
Copy link
Contributor Author

@hannahmbanana yes I didn't remember to close it. Thanks for the reminder!

aimalygin pushed a commit to aimalygin/AsyncDisplayKit that referenced this issue Oct 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants