-
Notifications
You must be signed in to change notification settings - Fork 810
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
Crash when switching numberOfRows to 1, then back to 6 and modifying the calendars height #304
Comments
ok i'll have a look |
I checked it on both regular and beta versions of Xcode and on the device also, I have this crash everywhere, it looks for me like some issue with threading because the only solution to avoid this crash that I found it to call this switch like so: func calendar(_ calendar: JTAppleCalendarView, didSelectDate date: Date, cell: JTAppleDayCellView?, cellState: CellState) {
//temp crash fix
DispatchQueue.main.asyncAfter(deadline: .now() + 0.1) {
self.weekMode = !self.weekMode
}
} |
ok. One thing, can you use the latest master branch code and let me know if you have the issue there? I just tested on master branch code and i cannot reproduce it. If you have the error there as well, then I will have to debug this over the weekend. |
Also, can you let me know if you are seeing this iOS 10.2 ? |
In the code you gave me, i see this --> func calendar(_ calendar: JTAppleCalendarView, didSelectDate date: Date, cell: JTAppleDayCellView?, cellState: CellState) {
weekMode = !weekMode
self.calendarView.scrollToDate(date, triggerScrollToDateDelegate: false, animateScroll: false, preferredScrollPosition: nil, completionHandler: nil)
} can you comment out this line? self.calendarView.scrollToDate(date, triggerScrollToDateDelegate: false, animateScroll: false, preferredScrollPosition: nil, completionHandler: nil) I want to isolate which line of code is causing the threading issue. |
Alright. Thanks. |
One last thing can be of help. |
|
Thanks. I have now replicated the issue |
Thank you for the help. That zip file has the latest master branch code. |
The bug was caused by this --> f368553 which is now fixed. I also made another change to the master code: Calendar reload should not be done automatically when a user changes the frameSize. This should be done by the developer. |
OK, you're using the same code i gave you? |
Yes, the code is unchanged, just clicking some same dates, I had this crash by clicking 1 and 31 also |
Alright. Its something i probably missed. Let me look into it very closely. |
Hey, here is an update code. Let me know if it is crashing for you. |
Ok. Looking ino it. By the way.
This is expected. The new code that will be released will have this logic. The Let me now look into Error(2) and Error(3) |
As an end user I would really expect complete consistency across the app, so if an application has some behavior to do something when you click on a day then this should happen no matter what. The issue here is that developers don't have control over this behavior without making some calculation to specify this start date correctly, taking in count InDates. This is the actual question for the apps which is relying on the week view because just hiding unused days will look not really good. Of course, this is up to you to decide such things. |
@numen31337 I see what you mean. Thanks for the input. I will remove this in next version then. What you said makes sense. Will look into those 2 other errors in the morning. Have a goodnight man. |
@numen31337 Your issue(3) is expected. Here is your updated zip file. I have changed your scrollingMode to Also, I have fixed some more threading issues. Can you let me know if you are still experiencing the crash? Because the crash is hard for me to reproduce, I have to fix this by looking at the logic. So I hope you don't mind if this is taking a while 😄 Edit [ Removed File ] fixing one last part |
Yes, that is the last part that i have to fix. I see why the issue is happening. Working on it now. |
Try this new build. |
There is one more issue i need to fix with it. You will get an update in about 3-4 hours. |
Because you changed the size of your calendar and reloaded at the same time, this caused your project to reveal many issues. Here is the latest build. I have tested and could not find any other issue. If the build is good, I will include these changes to the master branch. Thanks for helping reveal these issues. If there is still an issue somewhere, then let me know. And I will fix. |
After investigating, it seems that I checked the math and every thing is correct. I have now changed the This seems to work as expected. The only thing left to fix with this build is an existing scrolling issue. Let me know your findings. |
This version looks much better, I unable to reproduce the crash and even scrolling is working, at least I can't reproduce miss scroll for now. |
Awesome. I'll begin moving the changes over to master branch. |
Fixed on latest master branch. |
When I trying to switch calendar to the week mode and back by setting numberOfRows to 1 and then back to 6, simultaneously modifying the calendars height I have the crash:
I've attached the sample project where this crash can be reproduced by clicking Jan 31 for 3 times
Sample project: https://www.dropbox.com/s/lle6kuf93n8ptbi/CrashTest.zip?dl=0
The text was updated successfully, but these errors were encountered: