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

func calendar(_ calendar: JTACMonthView, willDisplay cell: JTACDayCell ... not getting called #1100

Open
rup2701 opened this issue Jul 26, 2019 · 10 comments
Assignees

Comments

@rup2701
Copy link

rup2701 commented Jul 26, 2019

(Required) Version Number:
Version 8.0.0

Description

Implementing cleanup in willDisplay. But the delegate method is not invoked.

Steps To Reproduce

Implement:
JTACMonthViewDelegate#func calendar(_ calendar: JTACMonthView, willDisplay cell: JTACDayCell, forItemAt date: Date, cellState: CellState, indexPath: IndexPath)

Expected Behavior

Cell cleanup.

Additional Context

@patchthecode
Copy link
Owner

patchthecode commented Jul 26, 2019

Ok.

Have you moved from version 7.1.7 to version 8?
Or, did you start this new project on version 8 directly.

@rup2701
Copy link
Author

rup2701 commented Jul 26, 2019

Yep. Everything else is working after moving from 7.1.7 to 8.

I'm pretty sure this method was not working in 7.1.7 either.

@patchthecode
Copy link
Owner

patchthecode commented Jul 26, 2019

I'm pretty sure this method was not working in 7.1.7 either.

Yes, because since the beginning i coded it this way.
You can see from the lib function below that on the very first line, i do a return if the cell is not dirty. It is only called in very specific times when a cell is dirty.

    public func collectionView(_ collectionView: UICollectionView, willDisplay cell: UICollectionViewCell, forItemAt indexPath: IndexPath) {
        if !pathsToReload.contains(indexPath) { return } // I return here
        pathsToReload.remove(indexPath)
        let cellState: CellState
        if let validCachedCellState = selectedCellData[indexPath]?.cellState {
            cellState = validCachedCellState
        } else {
            cellState = cellStateFromIndexPath(indexPath)
        }
        calendarDelegate!.calendar(self, willDisplay: cell as! JTACDayCell, forItemAt: cellState.date, cellState: cellState, indexPath: indexPath)
    }

But since you are the 1000th person to ask me this question, i am now wondering if i should remove that restriction. I was originally thinking to make the function more efficient (2+ years ago).
But now i think that since developers are expecting it to behave exactly like a UICollectionView, maybe i should remove the restriction.

I will give it a re-look to see if efficiency is really impacted. Looking at it with new eyes and 2+ years experience on this. Added the enhancement tag.

But in the mean time, just do what is suggested here and have a shared function between these 2 functions -> #553

@patchthecode
Copy link
Owner

Also, are you using Xcode 11 Beta by any chance?

@rup2701
Copy link
Author

rup2701 commented Jul 26, 2019

Also, are you using Xcode 11 Beta by any chance?

Nope. 10.3 GA.

@rup2701
Copy link
Author

rup2701 commented Jul 26, 2019

I'm pretty sure this method was not working in 7.1.7 either.

Yes, because since the beginning i coded it this way.
You can see from the lib function below that on the very first line, i do a return if the cell is not dirty. It is only called in very specific times when a cell is dirty.

    public func collectionView(_ collectionView: UICollectionView, willDisplay cell: UICollectionViewCell, forItemAt indexPath: IndexPath) {
        if !pathsToReload.contains(indexPath) { return } // I return here
        pathsToReload.remove(indexPath)
        let cellState: CellState
        if let validCachedCellState = selectedCellData[indexPath]?.cellState {
            cellState = validCachedCellState
        } else {
            cellState = cellStateFromIndexPath(indexPath)
        }
        calendarDelegate!.calendar(self, willDisplay: cell as! JTACDayCell, forItemAt: cellState.date, cellState: cellState, indexPath: indexPath)
    }

But since you are the 1000th person to ask me this question, i am now wondering if i should remove that restriction. I was originally thinking to make the function more efficient (2+ years ago).
But now i think that since developers are expecting it to behave exactly like a UICollectionView, maybe i should remove the restriction.

I will give it a re-look to see if efficiency is really impacted. Looking at it with new eyes and 2+ years experience on this. Added the enhancement tag.

Fair question. I did not dip in your code. Because as a wrapper I expected it to behave like a normal collectionView.

If any efficiency I was thinking it should be in my impl. For example, I'm marking the current day label as red. But since cells are recycled I get many different days marked as red. Hence the check in willDisplay.

image

@rup2701
Copy link
Author

rup2701 commented Jul 26, 2019

For now I have just put the cell dequeue and config code in func cellForItemAt.

@rup2701
Copy link
Author

rup2701 commented Jul 30, 2019

Hi JT,

Am I reading #553 correctly that implementing the shared code will allow willDisplay to be called?

@patchthecode
Copy link
Owner

no. I t just means that the code that is in the willDisplay and the cellForItem functions should be the same. Therefore, to reduce code duplication, just make them call a shared function.

If you want willDisplay to work as a regular UICollectionView, then wait a bit on the update.

@rup2701
Copy link
Author

rup2701 commented Jul 30, 2019

Ok. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants