Skip to content

accept nodeBlock* delegates for ASCollectionNode#23

Merged
dangthaison91 merged 3 commits into
masterfrom
feature/ascollection-block-delegates
Jul 26, 2018
Merged

accept nodeBlock* delegates for ASCollectionNode#23
dangthaison91 merged 3 commits into
masterfrom
feature/ascollection-block-delegates

Conversation

@fumito-ito
Copy link
Copy Markdown
Contributor

for #22 and nits for ASBatchContext delegate

@fumito-ito fumito-ito requested a review from dangthaison91 July 20, 2018 03:21
Copy link
Copy Markdown
Collaborator

@dangthaison91 dangthaison91 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @fumito-ito

@dangthaison91 dangthaison91 merged commit 47338d3 into master Jul 26, 2018
@fumito-ito fumito-ito deleted the feature/ascollection-block-delegates branch July 27, 2018 00:29
@vldalx
Copy link
Copy Markdown

vldalx commented Aug 6, 2018

@fumito-ito @dangthaison91
Had you tested this PR before you merged it?

According to ASTableDataSource docs tableNode:nodeBlockForRowAtIndexPath: method takes precedence over tableNode:nodeForRowAtIndexPath: if implemented.

According to the RP changes if configureCellBlock is not set then configureCell will be invoked from tableNode:nodeBlockForRowAtIndexPath: method. It means configureCell can be called on the main thread or a background queue. That is dead wrong.
Furthermore, two closures look redundant in that case.

@fumito-ito
Copy link
Copy Markdown
Contributor Author

@vldalx thanks for your feedback.

It means configureCell can be called on the main thread or a background queue. That is dead wrong.

Is #14 same as your feedback ?

Furthermore, two closures look redundant in that case.

I cannot what you mean. Can you describe me more details for it ?

@vldalx
Copy link
Copy Markdown

vldalx commented Aug 7, 2018

@fumito-ito
take a look at the code below. it has the similar logic to the RP changes.

class ASTableSectionedDataSource {
    var configureCell: ConfigureCell
    var configureCellBlock: ConfigureCellBlock?

    init(configureCell: @escaping ConfigureCell) {
        self.configureCell = configureCell
        self.configureCellBlock = nil
    }

    func tableNode(_ tableNode: ASTableNode, nodeForRowAt indexPath: IndexPath) -> ASCellNode {
        // never invoked.
        // configureCell is supposed to called here
    }

    func tableNode(_ tableNode: ASTableNode, nodeBlockForRowAt indexPath: IndexPath) -> ASCellNodeBlock {
        let closure = configureCellBlock ?? configureCell
        // configureCell can't be fallback here
        return closure(self, tableNode, indexPath, self[indexPath])
    }
}

taking into account that approach, why do we need two closures? That just shows something is wrong.

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.

3 participants