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

IMPORTANT: Here are some of the changes in version 7+. It will cause some code breaks. #553

Closed
patchthecode opened this issue Sep 6, 2017 · 18 comments
Assignees

Comments

@patchthecode
Copy link
Owner

patchthecode commented Sep 6, 2017

TLDR

  1. Please implement the willDisplayCell function as stated below.
  2. Do not use cell.isSelected I will be removing it. Instead use cellState.isSelected

.

Problem 1: UICollectionView cell pre-fetching enables your calendar to scroll smoothly. But some problems arise.

A case can arise where a cell is already pre-fetched, but is still not yet visible on the screen.
screen shot 2017-09-08 at 2 32 29 pm

In a case like this, lets say a user selects the Feb1 outDate.
screen shot 2017-09-08 at 2 32 29 pm

Then the Feb1st inDate on the other month has to also be updated to show a selection.
screen shot 2017-09-08 at 2 34 23 pm

Now If the cell is already prefetched, but still invisible, there is no way for JTAppleCalendar library to send you an instance of the already prefetched cell so that it can be updated.
The way I handled this in version 7.0.6 was to call reloadIndexPaths, but this invalidates the layout. In order to save the layout much code was done which caused subtle bugs to form.

According to Apple documentation ,

To avoid inconsistencies in the visual appearance, use the
collectionView:willDisplayCell:forItemAtIndexPath:
delegate method to update the cell to reflect visual state such as selection.

Therefore, in the 7.1.0 version of JTAppleCalendar, i'll be forcing you developers to implement this willDisplay function sadly. I did not want to do this because I think implementing 2 functions with the same code is silly, but it seems that I was coding against the grain. The contents of this function will be exactly the same as the code located in the cellForItemAtIndex function (except the cell dequeuing part). Therefore in order to avoid code duplication, you can create a common function that is run by both those functions.

If a developer can show me how to invalidate an already prefetched, but still hidden cell in a UICollectionView, then we would not have to do this.

Here is an example of what you developers should do when implementing this with a shared function to reduce code:

    func calendar(_ calendar: JTAppleCalendarView, willDisplay cell: JTAppleCell, forItemAt date: Date, cellState: CellState, indexPath: IndexPath) {
        // This function should have the same code as the cellForItemAt function
        let myCustomCell = cell as! CellView
        sharedFunctionToConfigureCell(myCustomCell: myCustomCell, cellState: cellState, date: date)
    }
    
    func calendar(_ calendar: JTAppleCalendarView, cellForItemAt date: Date, cellState: CellState, indexPath: IndexPath) -> JTAppleCell {
        let myCustomCell = calendar.dequeueReusableCell(withReuseIdentifier: "CellView", for: indexPath) as! CellView
        sharedFunctionToConfigureCell(myCustomCell: myCustomCell, cellState: cellState, date: date)
        return myCustomCell
    }

    func sharedFunctionToConfigureCell(myCustomCell: CellView, cellState: CellState, date: Date) {
        myCustomCell.dayLabel.text = cellState.text
        if testCalendar.isDateInToday(date) {
            myCustomCell.backgroundColor = red
        } else {
            myCustomCell.backgroundColor = white
        }
        // more code configurations
        // ...
        // ...
        // ...
    }

OR as developer @boborbt mentioned below, you can just do all you setup inside the willDisplayCell function. And for your cellForIndexPath function, simply do this

func calendar(_ calendar: JTAppleCalendarView, cellForItemAt date: Date, cellState: CellState, indexPath: IndexPath) -> JTAppleCell {
    let myCustomCell = calendar.dequeueReusableCell(withReuseIdentifier: "CellView", for: indexPath) as! CellView
    self.calendar(calendar, willDisplay: myCustomCell, forItemAt: date, cellState: cellState, indexPath: indexPath)
    return myCustomCell
}

Problem 2: UICollectionViewCell.isSelected may be out of sync.

I am thinking on removing the property of isSelected on a cell as it can incorrect.
In the library if the calendar is in single selection mode, if a user selects say Feb1, then I also cause its outDate counter part to be selected.

screen shot 2017-09-08 at 2 34 23 pm

Here 2 cells will be "selected" even though it is on single selection. I track this magic with the CellState struct. Therefore cellState.isSelected will always give you a correct value. But developers are used to using the Cell.isSelected property. And this causes confusion when a cell that is supposed to be selected gives an incorrect value.

Therefore I am going to make the Cell.isSelected property unavailable unless a developer can come up with a good alternative solution. Else, Users should only use the cellState inorder to determine if a cell is selected or not.

@ahmed-sal
Copy link

For what it's worth, I personally like to create my own isSelected variable. I get to call it whatever I want, I play around with it's getters and setters. And it's just part of my UI design. Up to you tho.

@patchthecode
Copy link
Owner Author

@ahmed-sal thanks for the input. I put it up here so that people can let me know if they are using the isSelected property on a cell. If many people complain, then I might try to consider another solution.

I am removing it because of synchronization issues. in some edge cases.
cellState.isSelected however, is managed by myself, and therefore it is always synchronized.

@patchthecode patchthecode changed the title Debate: Should developers use cellState.isSelected or the customCell.isSelected? Developer Input is needed: Here are some of the changes in the upcoming version. It may cause some very minor code breaks. Sep 8, 2017
@santiagoprieto
Copy link

I would rather keep it clean and always use cellState.isSelected for JTAppleCalendar instead of apple's presets. This way there's less chance of interference with other stuff as swift and apple kits change.

You're doing a great job at building the cellState for calendar-focused info.

@seanrice
Copy link

seanrice commented Sep 14, 2017

another vote for ♥︎ cellState.isSelected and ditto on the great job.

@patchthecode patchthecode changed the title Developer Input is needed: Here are some of the changes in the upcoming version. It may cause some very minor code breaks. IMPORTANT: Here are some of the changes in the upcoming version. It may cause some very minor code breaks. Sep 19, 2017
@boborbt
Copy link

boborbt commented Sep 26, 2017

@patchthecode I was just noting that, regarding the first problem, in most cases it is not really necessary to create the sharedFunctionToConfigureCell function. IMHO it is cleaner and easiest to just call the calendar:willDisplay delegate from the calendar:cellForItemAt delegate.

Thanks again for the library and your awesome support.

Repository owner deleted a comment from boborbt Sep 26, 2017
@patchthecode
Copy link
Owner Author

@boborbt you know.. that's actually a good idea haha. I wasnt thinking that route, but yes, this will be cleaner.

@patchthecode
Copy link
Owner Author

Version 7.1 released. closing this issue

@arjunmadgavkar
Copy link

arjunmadgavkar commented Apr 19, 2018

@patchthecode - thanks for this update, spent a few hours trying to figure out what was going wrong in my code, but this solved everything.

I have a question though, why can't you use dequeueReusableCell in the willDisplay function?

This was causing me an error as an unselected cell would appear to be selected, but I don't understand why this is the case.

Thank you 💯

@patchthecode
Copy link
Owner Author

@arjunmadgavkar This lib is built from a UICollectionView. It behaves the same way.
If you dequeue in that function, you be reusing an old cell that was already used and therefore you'd need to clean up the dequeued cell just like you would for a UICollectionView. But why would you dequeue a cell in there? WillDisplayCell is for displaying a cell that is already dequeued.

@patchthecode patchthecode changed the title IMPORTANT: Here are some of the changes in the upcoming version. It may cause some very minor code breaks. IMPORTANT: Here are some of the changes in version 7+. It will cause some code breaks. May 18, 2018
@patchthecode
Copy link
Owner Author

@PrincessKimchi theres a sample project attached to this repository.
check it out for an eample

@nickdnk
Copy link

nickdnk commented Jul 21, 2018

willDisplayCell does not get called for me. Delegate and data source are both connected, and the dequeue-function works and is called as it should. willDisplayCell is simply ignored. Any idea why?

@patchthecode
Copy link
Owner Author

willDisplay gets called.
it is not called as frequently as a regular UICollectionView or UITableView.
It gets called in certain situations.

@lukescott
Copy link

lukescott commented Dec 30, 2018

I think the root of the issue is updating the appearance of the cell within the controller violates MVC. The UICollectionViewDataSource.cellForItemAt call is meant to create the cell (view) represented by the data being passed in. Its job is to pass that data into the cell. Examples of cellForItemAt usually have you update the label - this is fine because you are passing data into the cell. But as soon as you start changing the appearance of the cell, you start violating that principle.

The UICollectionView manages the selection state internally. The isSelected property on the cell cannot be trusted because in that moment of time, that is how the view is being rendered. It isn't supposed to be your application state (or model). Since UICollectionView can update this state at any time, the appearance can get out of sync if you update the appearance in the controller.

What I typically do is update the appearance in the view itself:

override var isSelected: Bool {
    didSet {
        updateCellAppearance()
    }
}

updateCellAppearance() {
    if isSelected {
        // ...
    } else {
        // ...
    }
}

updateCell(info) {
    // ...
    updateCellAppearance()
}

That way it doesn't matter if the cell gets updated from cellForItemAt or by the UICollectionView itself - it always stays in sync. The cell also has a prepareForReuse method which allows you to cleanup the appearance of the cell before it is passed into the dequeue method.

The way I see it JTAppleCalendarView manages the state of the calendar for you. So I think CellState should probably be DateInfo or something along those lines. It should also probably not be able to return the cell itself, but rather have indexPath on it as an optional (if it is being rendered). You can call UICollectionView.cellForItem on the collection view itself if you need the cell. This makes the "state" (or data) not tightly coupled to the view and separates concerns.

@patchthecode
Copy link
Owner Author

@lukescott
i didnt understand the last part. What are you suggesting should be changed?

The way I see it JTAppleCalendarView manages the state of the calendar for you. So I think CellState should probably be DateInfo or something along those lines. It should also probably not be able to return the cell itself, but rather have indexPath on it as an optional (if it is being rendered). You can call UICollectionView.cellForItem on the collection view itself if you need the cell. This makes the "state" (or data) not tightly coupled to the view and separates concerns.

@lukescott
Copy link

lukescott commented Dec 30, 2018

@patchthecode Sorry! I'll see if I can be clearer. The way CellState is designed it mixes concerns. A cell is part of the UI. A date is part of the model (state). It should be "DateState" or "DateInfo". CellState also can return cell. It should have indexPath on it instead.

The problem with "CellState", and having to update the appearance in"willDisplay" and "cellForItemAt" has to do with mixing concerns in MVC. You have view code in the controller, and in the case of CellState, binding the view to the model. If "CellState" was "DateState" or "DateInfo" and had indexPath on it you could lookup the cell in the controller using indexPath (if you needed it).

Fixing the willDisplay problem doesn't require any code changes. It's just a documentation / education thing. Update appearance code in the cell view class. Anyone can do that today.

The "CellState" thing is not related. Just wanted to point out that the model is being tightly coupled to the view layer.

I'm not really a fan of MVC, but Apple uses it, so I have to follow it. I prefer Component based architecture as I find it easier to understand.

@Aaron-Spence
Copy link

myCustomCell.dayLabel.text = cellState.text
        if testCalendar.isDateInToday(date) {
            myCustomCell.backgroundColor = red
        } else {
            myCustomCell.backgroundColor = white
        }

Thank you for this fix, it helped a lot.

To hopefully save someone else the confusion that I went through with the above code snippet provided in the original post:

testCalender is actually just an instance of Calendar.current, and red is UIColor.red (and the white).

zeitschlag added a commit to zeitschlag/calendarpoc that referenced this issue Feb 7, 2020
please note, that configure is invoked in two places for a reason, at
least according to the [docs](https://patchthecode.com/jtapplecalendar-home/calendar-from-scratch/):

> These 2 functions should contain the same code, therefore it is wise
> to have a shared function to reduce code duplication. The only
> difference between these two functions should be the first line of code
> (the dequeuing code). Reasons for the 2 functions having the same code
> are found [here under problem#1](patchthecode/JTAppleCalendar#553).
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

10 participants