Skip to content

implement tableNode:nodeForRowAtIndexPath: execution thread#14

Merged
fumito-ito merged 5 commits into
masterfrom
feature-implement-row-at-delegate
May 12, 2019
Merged

implement tableNode:nodeForRowAtIndexPath: execution thread#14
fumito-ito merged 5 commits into
masterfrom
feature-implement-row-at-delegate

Conversation

@fumito-ito
Copy link
Copy Markdown
Contributor

According to API doc, tableNode:nodeForRowAtIndexPath: should be called on main thread.

@dangthaison91
Copy link
Copy Markdown
Collaborator

Hi @fumito-ito , AFAIK RxSwift Binder will help to ensure your code run on mainThread.

@fumito-ito
Copy link
Copy Markdown
Contributor Author

@dangthaison91 thanks ! I will check it later !

@fumito-ito fumito-ito force-pushed the feature-implement-row-at-delegate branch from a2d26a5 to 306c939 Compare April 3, 2018 06:25
@fumito-ito fumito-ito changed the title [WIP] implement tableNode:nodeForRowAtIndexPath: execution thread implement tableNode:nodeForRowAtIndexPath: execution thread Apr 3, 2018
@fumito-ito
Copy link
Copy Markdown
Contributor Author

fumito-ito commented Apr 3, 2018

I used DispatchQueue not Binder to get result of closure.

@fumito-ito fumito-ito requested a review from dangthaison91 April 3, 2018 06:38
@dangthaison91
Copy link
Copy Markdown
Collaborator

Sorry @fumito-ito , my health has gone bad for weeks. Will be back this weekend.

@fumito-ito
Copy link
Copy Markdown
Contributor Author

@dangthaison91 I understand. Get well soon !

fileprivate static func configureCellBlockNotSet(dataSource: ASTableSectionedDataSource<S>, node: ASTableNode, indexPath: IndexPath, model: I) -> ASCellNodeBlock {
return { dataSource.tableNode(node, nodeForRowAt: indexPath) }
// Users expect taleNode(_:nodeForRowAt:) will be executed in main thread according to api doc http://texturegroup.org/appledoc/Protocols/ASTableDataSource.html#//api/name/tableNode:nodeForRowAtIndexPath: .
return { DispatchQueue.main.sync(execute: { dataSource.tableNode(node, nodeForRowAt: indexPath) }) }
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

IMHO, Binder using in RxASTableNodeReloadDataSource already guarantee that datasource is executed on Main Thread.

Could you help to verify that by using another DispatchQueue in Example app.

Copy link
Copy Markdown
Contributor Author

@fumito-ito fumito-ito Apr 20, 2018

Choose a reason for hiding this comment

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

@dangthaison91 Sorry for delay.

configureCellBlockNotSet may be executed in tableNode(_ tableNode:nodeBlockForRowA:) -> ASCellNodeBlock and this delegate is NOT called in main thread but called in background thread.

Following sentences are from Texture API Doc.

– tableNode:nodeBlockForRowAtIndexPath:

Asks the data source for a block to create a node to represent the row at the given index path.
The block will be run by the table node concurrently in the background before the row is inserted into the table view.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@fumito-ito @dangthaison91
I don't think this is best solution.

 tableNode:nodeForRowAt:
 tableNode:nodeBlockForRowAt:

will always execute in main thread.
because ASDK will check execution thread in ASDataController by ASDisplayNodeAssertMainThread().
but ASNodeBlock() will be execute concurrently in background .
So, see the Rx logic, if we not set configureCellBlock, it will fall back by embed tableNode:nodeForRowAt: in configureCellBlockNotSet, It will cause tableNode:nodeForRowAt: execute in background thread.
Actually, without RxASDataSOurce, if you want init node at main thread, you should just implement tableNode:nodeForRowAt: and ignore tableNode:nodeBlockForRowAt:, but both of them implement in Rx, I guess this is why we use configureCellBlockNotSet function.
in the end, this is my solution:

 let cellNode = dataSource.tableNode(node, nodeForRowAt: indexPath)
 return { cellNode }

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your comment @PatrickChow .

Your solution looks same as return { dataSource.tableNode(node, nodeForRowAt: indexPath) }. Do you mean this change is not needed ?

Copy link
Copy Markdown
Contributor Author

@fumito-ito fumito-ito Mar 22, 2019

Choose a reason for hiding this comment

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

Oh I found the difference. According to your solution, let cellNode = dataSource.tableNode(node, nodeForRowAt: indexPath) is called in main thread but { cellNode } will be executed in background.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@fumito-ito Yes, it is

@fumito-ito
Copy link
Copy Markdown
Contributor Author

@dangthaison91 add update for ASCollectionNode. Could you review it ?

@foxware00
Copy link
Copy Markdown
Contributor

@dangthaison91 add update for ASCollectionNode. Could you review it ?

Any update on this PR, i'm having issues with the threading of configureCell in 0.3.3

@dangthaison91
Copy link
Copy Markdown
Collaborator

@foxware00 Could you help direct your pod to @fumito-ito repo for this PR and help to verify if this PR will resolve your issues with configureCell is not called on Main Thread?
On my side, this PR is OK but don't have time to check it again.

@dangthaison91
Copy link
Copy Markdown
Collaborator

@fumito-ito Are you still working on this PR?

@fumito-ito
Copy link
Copy Markdown
Contributor Author

@dangthaison91 hi! I catch up the comment from @PatrickChow #14 (comment) .

@fumito-ito fumito-ito merged commit 5763d8f into master May 12, 2019
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 this pull request may close these issues.

4 participants