Style, naming and other minor nitpicking

[dbmrq]

You asked if I had any other suggestions… well, JTAppleCalendar is already awesome, and it works great, but here are a few thoughts, mostly just some tiny details that I think could be improved.

Take these methods, for instance:

func configureCalendar(calendar: JTAppleCalendarView)

func calendar(calendar : JTAppleCalendarView, isAboutToResetCell cell: JTAppleDayCellView){}

Why is the colon next to calendar in the first case but separated with a space in the second? There are many other similar inconsistencies. I suggest you give SwiftLint a try. I can set it up at the demo project if you're interested.

Also, why the name isAboutToResetCell instead of the standard willResetCell? Apple has some guidelines about that here.

And speaking of naming conventions, I didn't really take a deep look at the changes in swift 3 yet, but I think according to the new convention (check the second item, "omit needless words") that method should read isAboutToReset cell: JTAppleDayCellView instead of isAboutToResetCell cell: JTAppleDayCellView. There are many other examples like that, and since swift 3 already makes a lot of those breaking changes it's a great excuse to make them here too I think.

Also, since you already follow the way table views work, why use that calendarView.registerCellViewXib(fileName: "CellView") property instead of a delegate method like cellForDayAtDate (like you have cellForRowAtIndexPath for table views)? Then the user would be able to configure the cell right in the view controller where it will be displayed, instead of cramming the code in the cell class. (How many lines do your UITableViewCell classes have? A lot less than your JTAppleDayCellView classes, I bet.) Also that would allow the user to add the cell view straight to the view controller the calendar view is in (like it is with UITableViews), instead of having to add a dedicated xib. And the cell view class would be a lot easier to repurpose.

Lastly, this may by a little more tricky and it's not essential, but since you seem to be making some big changes already, here's another suggestion. I think it would be better if the calendar didn't have those startDate and endDate limits. It would just have an initial date that could default to today, and then the UICollectionView would be created with 3 months, the current month, the month before and the month after. When the user scrolled to the next month, one more month would be added after that one, and so on. And if the user wanted he could set a property like calendarView.maxDate.

As I said, these are all just suggestions, feel free to ignore them if you prefer. JTAppleCalendar is really great already. :)

[patchthecode]

1. Concerning swiftLint. you are right. I promoted it on reddit and there was a conversation with one guy mentioning the very same things here. The user name of the guy is called nhgrif, you can see his comments there. So yes we can do swift lint. The problem is man power. Any help is appreciated!
2. isAboutToResetCell <=> willResetCell
   The reddit guy mentioned the same thing. Problem again = man power.
3. Concering swift 3. I have a swift 3 branch. It did a good job of converting the majority of the code. But yes it can be improved. For this swift 2 version, yes the names can change as well. I just had this at the bottom of the list since the code on this last upgrade i'm working on is a head scratcher.
4. cellForDayAtDate. 😕 You know.. Looking at your suggestion and looking at the code I have done..
   You know when you do code that looks good to you one day, and then you re-look it another day and realize that another way could have been better? That's what i'm thinking now. Your suggestion is a much superior idea. The view can be grabbed from a cellForDayAtDate function and added to the subview of the cellView. And yes, it will indeed reduce the code in the cell.
5. The infinite scrolling calendar I have thought of. But you are right about it being tricky. Currently, i depend on an array being generated to keep track of the calendar offset days. This array is generated in the setupMonthInfoDataForStartAndEndDate() function. I use that array to do calculations to determine how to scrollToDate etc. Many changes will have to be made if this setup is changed.

Numbers 1, 2 & 3 should not take long to complete.
Also, I think Number 4 is the best idea here. It will simplify a lot of things and I can extend that same solution to registering for headers. Devs would be able to provide views on the fly. This can even allow for multiple cells, so that devs would not have to cram every view into an xib.

All good suggestions. I know you're probably busy but any help is appreciated.

[patchthecode]

Thoughts on rethinking Number#4

Ok I think i remember my thoughts which made me register using a property.

If the User provides the view in a cellForDayAtDate function, then i thought the User might do inefficient things in that function things which might cause the calendar to lag. Also, the user would maybe at most want just about 1 or 2 cell types. But that function will be called before every cell is displayed. (42 times if user has a full 6x7 rows and columns). So I think maybe that was why I opted to have it as a property. For performance reasons.

Currently how the code is, it basically remembers the view that the user wants, and caches it. Thereby limiting calls to the dev's code (which could include bad performing code). Since a dev will only want 1 or 2 different cells, I opted to just directly remembering those 2 cells, instead of making a delegate call every time the user scrolls the calendar.

But now that you brought it up, I am wondering if my quest for performance was unnecessary in this case. Currently, this calendar works well on an old iPhone 4s.

[dbmrq]

Ah, yes, he makes some good points. :)

I have to update my app to swift 3 soon and then I'll try to add SwiftLint here and clear its warnings on the swift 3 branch if I have the time. As for item 3, yes, I was actually referring to the swift 3 branch; I can still find examples of the naming conventions not being followed there. I'll try to fix that too and make a PR.

About your last message, I don't see how number 4 would reduce performance unless the user did something stupid (which he could already do, except that it would be in the JTAppleDayCellView class). From what I understood, right now each cell is instantiated by the calendar view itself and then something like cell.setupCellBeforeDisplay() is called at isAboutToDisplayCell to set it up. With my suggestion, on the other hand, the user would instantiate the cell and set it up however he wanted, and then just pass it to the the calendar with cellForDayAtDate. In both cases a cell has to be instantiated and configured for each day. The way it is now the cell is instantiated by the calendar (at setupView() I think?) and configured at isAboutToDisplayCell, with my suggestion this would all be done at cellForDayAtDate. Whatever the case, the same stuff has to be done. So it wouldn't make a difference, right? The only way I can think of for this to be improved would be to take a page from UITableView again and do something like its dequeueReusableCellWithIdentifier for the day views. Then instead of creating new views for each day you'd reuse the views that are now invisible. That would make even more sense along with suggestion 5. It's a whole other story though and I don't even know how to go about that. In any case I don't think suggestion 4 has any drawbacks (I could be wrong though).

Also, about suggestion 5, what do you mean by "offset days"?

[dbmrq]

Ah, I just realized you already use UICollectionReusableView. I don't know much about working with UICollectionViews (clearly), but I see they also have a cellForItemAtIndexPath method, so it would be just like that, except we'd have a cellForItemAtDate method and we'd get a cell there with dequeueReusableCell just like in table views and appartly collection views too.

[patchthecode]

hmm..

Inside the setupView function, I have this line of code:

if view != nil { return}

This means that although the dev's xib view is created for every cell (as you have correctly said), the xib view is only instantiated once.
So lets say we have a 6x7 calendar = 42 cells.
This will only be run 42 times, for the first 42 cells displayed on the screen.

For all other times, the developers xib view will not be instantiated again. It will be reused.

Currently, In the cellForItemAtIndexPath function in the JTAppleCalendarDelegates.swift file, I am doing the dequeueReusableCellWithReuseIdentifier you have mentioned. The Cells are being created/reused there. But I do not send the actual cell to the developer. I instead send the registered xib view to the developer. The registered xib view is the subview of the cell.

With the current setup (assuming a 6x7 grid), 42 cells will be instantiated, and reused. And 42 subviews will be attached to those cells. Since the 42 views are subviews to the 42 cells, they are also only created once and reused. This is what I was referring to with performance.

If i understand correctly however, you are suggesting something like this?

function cellForItemAt(calendar: JTAppleCalendar, date: NSDate) -> JTAppleDayCellView {
    let dateCell = myAlreadyCustomizedDayDell()
    someFunctionToConfigureTheCell(dateCell)
    return dateCell
}

[dbmrq]

Ah, ok, now I get it, sorry.

Yes, what I had in mind was something like that, but now I see it wouldn't be that easy to do that and use the native reusable cells at the same time. So I guess your current solution is probably the easiest and it works well enough.

It would be nice if JTAppleCalendarView were directly a subclass of UICollectionView, then the user could just drag a collection view to his storyboard and design its cell right there and then, he could give it an identifier himself and configure it in a function like the one I suggested using the native methods like dequeueReusableCell himself. That's a substantial change over how things are now, though, and I'm not even sure it's the best idea. I just thought I'd put it out there. I'll look into it some more when I have the time, but the way it is right now is already great. See, yours is the superior idea after all. :)

[patchthecode]

Hmm. you know. lol, that UICollectionView direct subclass sounds like a good idea haha. The code change would be very minimal, and the benefits would be incredible. I will look into it. I'm about 60% finished coding

[patchthecode]

wow.. thinking about it. A direct UICollectionView subclass is an even better idea than Number#4 yesterday! I will definitely look at it once i'm done. I see no conflicts with it in my mind right now. It will allow for multiple cells, and the user creating their own identifiers directly on the storyboard indeed. (if it works)

[dbmrq]

Yes, if it works out I think it would be pretty great. :)

One reason to put the UICollectionView in a UIView is dealing with the delegate, like this guy suggests here: http://stackoverflow.com/a/26083072/3539651

But I think we can work around that by using getters and setters like those other answers suggest, or if that doesn't work we can just declare a new property like calendarDelegate.

Then we could do something like this to expose some of the original delegate methods (like didScroll) and replace others (exposing cellForItemAtDate instead of cellForItemAtIndexPath).

That's the trickiest part, I guess. Other than that I don't think much else would have to be changed.

Since you're already working on some major changes I'll wait until you're finished with that and then I'll look into adding SwiftLint and changing my first 3 suggestions if you want me to, ok? Fixing the style and naming conventions will change the API, but this idea will too, so you could bundle those changes together in another major release.

[patchthecode]

Yea. sure man, you can go ahead. You sound like someone who does neat code.
I'll almost done the changes.

[patchthecode]

Changes are done.
Now I have to fix the userInteractionFunctions which should not take long at all.

Btw, do you think its a good idea to stop support for swift 2?
In my work we're now moving to swift3 so, I was wondering if pretty much every one else was doing the same.

[dbmrq]

Great!
I'm waiting until you're done with the bulk of your work and then I'll get started with the linting, so there won't be that many conflicts.

I think it's a good idea to have a stable swift 2.3 version for people who are still transitioning, but for the new stuff, sure, I'd leave swift 2 behind.

Is the DynamicGeneration branch already in swift 3? I'm on my phone now, I couldn't take a look yet. Anyway, it's a good idea to add SwiftLint when everything's already in swift 3, since a lot of the sintax changes.

[patchthecode]

DynamicGeneration branch should be in swift 3 by morning.
I'm converting it now.

Swift linting etc can start then.
Also i need to do some code cleanup.

[dbmrq]

I'll get started tonight. :)

[dbmrq]

Uhm, now I just forked it and the branch isn't in swift 3 yet, right?

[patchthecode]

I'm @ work. Making that last push in about 3 hrs.

[dbmrq]

Ah, no problem, just let me know. I've got a lot on my plate too. Take your time. :)

[patchthecode]

I have just pushed it.

The datein/dateout functionality should be working now. (not sure if there are bugs i did not find yet)

Massive code cleanup is needed. Will get to it once home. Maybe then we can split up stuff (if youre interested). Conversion is in swift3.

[patchthecode]

Other userInteractionFunctions are still broken. Will also get to it.

[dbmrq]

Sure, I don't have that much time, so it might take me a while to do my part, but I'm up for it.
You could make a checklist split in small tasks, like "fix function X", "add feature Y", and then we can take up tasks one by one when we have the time.
Let me know when the DynamicGeneration branch is relatively stable, preferably with the example app running, and then I'll add Swift lint and all. :)

[patchthecode]

ok. will let you know.
But just so you know, currently, im working on the file JTAppleCalendarView.swift

[patchthecode]

I think i'm finished.
Just bug testing now.
DynamicGeneration branch has the latest changes

[dbmrq]

Awesome, I'll take a look soon. 🎉

[dbmrq]

Ok, I just checked out the DynamicGeneration branch and started playing around with the demo app. I noticed a few weird things:

• When using the "PreOff" option the first day always shows up in the first column, while it should show up at the same place as before, right?
• When using the "HeadersOff" option there are many problems:
  • The dates get all messed up, sometimes first day shows in the middle of the collection view
  • If you also use the option "PostOff" sometimes a lot of stuff is hidden that shouldn't be, especially at the last month available
  • If you play around enough and keep turning the other options on and off the little arrows at the upper left corner just stop working
• If you use the arrows the month label doesn't update
• If you change to "VerticalCalendar" and then back to "HorizontalCalendar" everything gets messed up again

I think that's what I noticed so far.

So how do you want to go about it, do you want to fix this stuff before I start making changes? I can start anyway, but then we might have conflicts later when trying to merge.

[patchthecode]

I dont know how you see all these bugs.
When I return i'll look at them.
There should be no bugs in that demo app :/
Will be back in 30 mins

[dbmrq]

No problem, haha. Sometimes it's hard to see it when it's your own app, it happens to me all the time.

[dbmrq]

Now I tried to make a super simple app just to see how it goes. I added just the bare minimum, so no headers or nothing. And my cell views have just a single label.

This is all I have in my view controller:

    @IBOutlet weak var calendarView: JTAppleCalendarView!

    override func viewDidLoad() {
        super.viewDidLoad()
        calendarView.dataSource = self
        calendarView.delegate = self
        calendarView.registerCellViewXib(file: "CellView")
    }

    func configureCalendar(_ calendar: JTAppleCalendarView) -> ConfigurationParameters {
        let calendar = Calendar.current
        let startDate = calendar.date(byAdding: .month, value: -3, to: Date())!
        let endDate = calendar.date(byAdding: .month, value: 3, to: Date())!

        return ConfigurationParameters(
            startDate: startDate,
            endDate: endDate,
            numberOfRows: 6,
            calendar: calendar,
            generateInDates: true,
            generateOutDates: .tillEndOfRow,
            firstDayOfWeek: .sunday
        )
    }

    func calendar(_ calendar: JTAppleCalendarView, willDisplayCell cell: JTAppleDayCellView, date: Date, cellState: CellState) {
        let cell = cell as! CellView
        cell.dayLabel.text = cellState.text
    }

I run the app and at first it looks ok:

Scroll to the next month and…

Next…

[patchthecode]

just got back. Looking at it now

[patchthecode]

OK. What you see happening is expected to happen. haha.
I coded it with the wrong concept. One sec, i'll fix this.

[dbmrq]

Ok, let me know. :)

[patchthecode]

OK. Fixed. Should be working now.
I misunderstood the error your found.

And ended up "fixing" a bunch of stuff that didnt need fixing, thereby breaking every thing.
Woke up this morning, and then realized it should have only been a 5 line code fix xD
damn...

[dbmrq]

Haha, sorry for that.
I'll take a look soon, thanks. :)

[dbmrq]

Now we're talking, it's looking a lot better!

There are still a few problems, though (sorry 😳):

• There's still the problem when using "PreOff", it makes every month starts on a Sunday.
• When using both "PreOff" and "PostOff" it's even worse, the months get all messed up.
• Now the month label works when using the arrows, but not if you scroll the calendar. It also doesn't update when tapping the "ScrollToDate" button.
• When switching to "VerticalCalendar" and back to "HorizontalCalendar" again everything gets messed up (that's not so bad, though, since it's unlikely someone would use both those options in the same calendar).
• When using any other number of rows besides 6 there are many weird things:
  • Often the arrows won't change the date in the calendar.
  • If you play around with "PreOff", "PostOff" etc. things get weird too.
  • If you start selecting random dates or ranges of dates the selection gets a little crazy, often it's a lot larger than it should be and it covers other dates.
    

Sorry for keeping finding bugs, I understand if you can't or don't want to fix everything right now, I'm really busy myself, I just thought you'd want to know.

[patchthecode]

OK let me try to understand the problems. I've responded to them inline.

1. There's still the problem when using "PreOff", it makes every month starts on a Sunday.
   Yes. If you set pre-Dates to Off, they will not be generated. Therefore, all following dates after the pre-Dates will be shifted to the left to fill the gap of the missing predates. If you want the space to still be there, and for there to be no shifting, then what you want is to have pre-Dates set to on. But you want the visibility of the pre-Dates to be hidden.
2. When using both "PreOff" and "PostOff" it's even worse, the months get all messed up.
   Setting both to off will cause shifting on both sides. With post dates off, the next month's date will shift to the left to fill in the gaps.
3. Now the month label works when using the arrows, but not if you scroll the calendar. It also doesn't update when tapping the "ScrollToDate" button.
   That code was commented for testing purposes. I now un-commented it.
4. When switching to "VerticalCalendar" and back to "HorizontalCalendar" again everything gets messed up.
   Ok. This one is a real bug. I forgot to reset the size. It has now been fixed and pushed.
5. Not a bug. I just have bad constraints on the selectedView on the CellView.xib file. I have the selected view's constraints to be a ratio. Basically, in short, I made an ugly dateCell. But thats cool right? Since I'm responsible for making a perfect library, and not a good looking sample app. The developers will be the ones to create the cool looking calendars.

And man, finding bugs is what i wish more people do. I have no problem with it. I invite it.

Are you clear on point 1 & 2 above though?

[dbmrq]

Ah, I didn't get what the "PreOff" and "PostOff" options are for, then. I thought they were just a convenient way to set the pre and post dates do hidden. Why would any one wan to not even generate the pre dates?

Anyway, it's all good then. I'll get started on my changes soon and start making some PRs. :)

Thanks!

[patchthecode]

I have had many requests for that preOn/off and postOn/off. People have different calendar designs.

I know of one place the preOff/and postOff makes sense. In a single row setup, when pre is On and post is On, it looks really weird. As the user is scrolling side ways, pre-dates and postDates pop up giving the impression that numbers are repeated. In a 6-row calendar, it is clear that those are just pre and post dates, but in a single row view, it looks weird.

And cool yea. Will wait for the PRs. I know i have a lot of code cleanup to do.

[patchthecode]

Examples of guys who want this feature are:
#143
#129
#10

there were others, but i cant find them all.
The ability to not generate pre/post also allows for designs like this (without the confusing in/out dates)

[dbmrq]

Ah, I see! Ok then, thanks for the explanation.

[patchthecode]

@dbmrq thanks for the support man! :]

[dbmrq]

Don't mention it. Kee up the good work. :)

[patchthecode]

Ok i think i can finally close this :)
I think i did most on the list that is possible.
I even fixed the thing that i said wasnt a bug before "the shifting of the dates".
I made the behavior more standard. Thanks for all the help man.
I'm still here fighting to get more publicity 👍

[dbmrq]

Awesome, thanks for your amazing work!
I hoped to help more, but things have been pretty hectic, sorry. The library's really looking better and better though, way to go. :)

[patchthecode]

Awesome man. Well anytime you got more input just give a shout. Till then. Have a good one :)