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

Conversation

levi
Copy link
Contributor

@levi levi commented Oct 23, 2015

This is a start on the concepts discussed in #761. The underlying data controller has been modified to allow synchronous construction of the underlying nodes on the main thread on reloadDataImmediately, while batch layout of the results still benefit from background thread parallelization.

Duplicated Kittens example here to better illustrate the change. Would recommend wrapping this into something like SynchronousConcurrency to reduce unnecessary duplication when we do decide to land this.

The current implementation loads all the cells in the data controller's data source, so not fit for large amounts of data. #761 discusses the idea of only preloading a given number of cells, possibly based upon a provided bounds—like screen size. Investigating this next.

@eanagel
Copy link
Contributor

eanagel commented Oct 25, 2015

I love here you're going with this, @levi! I have several use cases for this myself, including the initial load of data. One question I had - is it possible to mix and match synchronous and asynchronous operations safely? In my brief scan of the code, I didn't see how you were keeping transactions in the editing transaction queue from stomping on a synchronous operation or is that covered because we already do enough on the main thread to keep things safe?

@levi
Copy link
Contributor Author

levi commented Oct 25, 2015

@eanagel, any section or row modification method in the data controller first expects to be on the main thread in order to be called. Second, before the method body takes place, these methods call waitUntilAllOperationsAreFinished on the editing queue to ensure its transactions are consistent. Since the synchronous version of reloadData is performed entirely on main (except for parallelized node layout), we shouldn't run into threading issues with external clients using the data controller.

On Oct 24, 2015, at 23:55, Ethan Nagel notifications@github.com wrote:

I love here you're going with this, @levi! I have several use cases for this myself, including the initial load of data. One question I had - is it possible to mix and match synchronous and asynchronous operations safely? In my brief scan of the code, I didn't see how you were keeping transactions in the editing transaction queue from stomping on a synchronous operation or is that covered because we already do enough on the main thread to keep things safe?


Reply to this email directly or view it on GitHub.

@eanagel
Copy link
Contributor

eanagel commented Oct 25, 2015

@levi, that makes perfect sense! The waitUntilAllOperationsAreFinished is the rather important piece that I missed in this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

woah, wtf. I didn't even know about this feature. This looks like really terrible code, we should revisit the API / functionality here to see if it can be more automatic.

@appleguy
Copy link
Contributor

@levi this is so awesome. Thank you for doing this within hours of the community raising it as a need!

Would it make sense to add the synchronous mode to the Kittens app without duplicating the project? Maybe even a tab bar controller within the main Kittens app?

If so, you could do a later PR that deletes the new sample project and merges them. It could actually be a good environment to test ASViewControllers' ability to preload an ASTableView or ASCollectionView before the controller is shown; this is not implemented yet, but needs to be soon. We could also test neverShowsPlaceholders in such an app.

If you have a different idea for combining them, please do!

appleguy added a commit that referenced this pull request Oct 26, 2015
Add synchronous reloadData methods to ASTableView and ASCollectionView
@appleguy appleguy merged commit 8dddcaa into facebookarchive:master Oct 26, 2015
@zintus
Copy link

zintus commented Oct 26, 2015

Wow, thank you so much for this! ❤️

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants