search

Quivr / Android-Week-View

Android Week View is an android library to display calendars (week view or day view) within the app. It supports custom styling.
Apache License 2.0
258 stars 64 forks source link

onMonthChangeListener not working as expected #83

Open ln-12 opened 6 years ago

ln-12 commented 6 years ago

Hey guys,

I'd really love to use your library as part of an app for students which includes a timetable.

After testing around a bit, I noticed that the behaviour of the onMonthChangeListener seems to be broken. When I clone your sample project and start the app (Basic Example) the debugger tells me that onMonthChange(int newYear, int newMonth) in the BasicActivity is called exactly once, in my case (Mar 29th 2018) newYear=2018 and newMonth=3. So far, so good. Now I would expect that it it also called for newMonth=2 and newMonth=4, but that is not happening. When I scroll to the right, the listener is called on April 3rd with newMonth=4 (3 days to late) and when I scroll to the right left it is called on Feb 16th with newMonth=2 (about 14 days to late).

The original library by alamkanak behaves the way I would expect it and how you described it in Issue #70. It calles the listener 3 times on startup and depending on the direction again when needed. Please have a look at this bug or tell me what I am doing wrong.

Thanks!

jhoobergs commented 6 years ago

We disabled prefetching in this fork because it shouldn't be the task of the library itself. (see https://github.com/Quivr/Android-Week-View/pull/42) . It's strange that he doesn't call it on the right days, did you test with multiday view or one day view ?

ln-12 commented 6 years ago

I just tested it again: in single day view mode it seems to work well, but as multiple days are visible it gets weird. In week view mode onMonthChange is called when the 8th of the next month becomes visible. So my guess would be that it's called N days to late when N is the number of visible days. is that the loader is only called, when the first day of the next month is the first day visible and it is about the be swiped out to the left. Instead it should be called when the first day is about to enter from the right.

The hint about the WeekViewLoader interface helped me understanding it, although I'm not really sure how to implement it to get prefetching behaviour. Could you give me a short explanation on that point? I assume the best idea is to provide events for the period of time from the 15th of (current month -1) untill the 15th of the current next month, right?

Btw I noticed the current implementation always devides by 30. Could that be a problem if the current month has a different amount of days (e.g. Feb)? (This could easily be done with instance.getActualMaximum(Calendar.DAY_OF_MONTH))

entropitor commented 6 years ago

When I scroll to the right, the listener is called on April 3rd with newMonth=4 (3 days to late) and when I scroll to the right it is called on Feb 16th with newMonth=2 (about 14 days to late) I just tested it again: in single day view mode it seems to work well, but as multiple days are visible it gets weird. In week view mode onMonthChange is called when the 8th of the next month becomes visible. So my guess...is that the loader is only called, when the first day of the next month is the first day visible and it is about the be swiped out to the left. Instead it should be called when the first day is about to enter from the right. Your guess is 100% correct. Unfortunately, the way it is set up right now, it's harder to do this with a normal MonthLoader, but I'll give a better solution below.

As to why it is only called on Feb 16th, I'm not 100% sure. Honestly, I think the library could really use some unit tests here. But I think it has to do with this line: https://github.com/Quivr/Android-Week-View/blob/develop/library/src/main/java/com/alamkanak/weekview/WeekView.java#L888 (which we might need to drop/adapt because of the PR 42 linked above)

The hint about the WeekViewLoader interface helped me understanding it, although I'm not really sure how to implement it to get prefetching behaviour. Could you give me a short explanation on that point? I assume the best idea is to provide events for the period of time from the 15th of (current month -1) untill the 15th of the current month, right?

Well, it's a very confusing interface, but basically, you can choose whatever period you want. You just need to be able to give every period a periodIndex. E.g. this could be a week, a day, hell, it could even been 1 year. In essence you can see this method as two methods: 1) what is the index of the period of which this day is part of. E.g. for a period of a year, this could be the year itself. 2) where in this period (more or less) does this day fall. 0.5 would mean middle of the period, e.g. for a month it would be the 15th day.

So the day with periodIndex 2018,5 would be a day, somewhere around July 1st, 2018. This 2nd part was mainly for the prefetching (when we were still fetching 3 months, it would only go fetch the 2nd next month once you were over half into the next month), but we might not need it anymore. This might be causing the problem listed above with the past only loading on Feb 16th

Btw I noticed the current implementation always devides by 30. Could that be a problem if the current month has a different amount of days (e.g. Feb)? (This could easily be done with instance.getActualMaximum(Calendar.DAY_OF_MONTH)) Because it wasn't necessary to be very precise, it doesn't matter that much. Because DAY_OF_MONTH is 1-based, (DAY_OF_MONTH-1) is 0-based and its maximum is 30, so if you divide by 30 again, it can never get into the next month and while the answer to question 2 might be off by a bit, it doesn't matter that much. But yes, that would have been a better implementation.

To fix this

I think there are two things we should fix:

  1. There is a bug with the loading if you scroll back. This check is probably not necessary
  2. We need to think about how we can take into account all days that are visible. There are two options: pass this along to the WeekViewLoader or use a prefetching kind of WeekViewLoader (see below)

A PrefetchWeekViewLoader could be a decorator class decorating another WeekViewLoader (So it can be used with any kind of WeekViewLoader). It has 3 lists + currentPeriodIndex as fields and basically calls the underlying WeekViewLoader 3 times on initialization and otherwise just shifts its lists to return the correct ones and calls the underlying WeekViewLoader to prefetch (cfr. the code that was removed with https://github.com/Quivr/Android-Week-View/pull/42/files). The onLoad method would then just return a list that is a concatenation of these 3 other lists.

Is that clear? If so, I'd be happy to review any PR's :smile:

ln-12 commented 6 years ago

Thanks a lot for your detailed explanation!

  1. There is a bug with the loading if you scroll back. This check is probably not necessary

With pull request #86 this bug should be gone, some testing would be appreciated.

  1. We need to think about how we can take into account all days that are visible.

The second problem is a bit harder to fix. In my opinion just saying that the library shouldn't be responsible for prefetching is a mistake. Let me try to explain: assumed your view start somewhere in the middle of month 4 with week view (7 days visible) enabled. When you scroll to the right (future) the onMonthChange listener will be triggered at the end of the 4th month/beginning of the 5th month, let's say when the 1st day of month 5 will become visible. The events stored by the library (for month number 4) are cleared and replaced be the ones for month number 5. The problem now is that you can see events for month 5, but any events for month 4 (of which the last few days are still visisible) are suddenly gone. The same happens when you scroll to the past.

For better understanding I recorded a short video: video

So if you have no prefetching at all there will always be problems like shown in the video, that's why it was included in the original libary. I would suggest the easiest thing is reverting pull request #42 and adjusting the lines around https://github.com/Quivr/Android-Week-View/blob/develop/library/src/main/java/com/alamkanak/weekview/WeekView.java#L888 so that getMoreEvents is called on the 15th of the last month or the 15th of the next month. (I think that's also the reason for "bug" 1, it was wanted behaviour)

entropitor commented 6 years ago

@mc-12 This PrefetchingWeekViewLoader decorator could be part of the library. And I think most people will/should probably use it, but it should not be part of the main WeekView class as then it's impossible to not have prefetching or to implement it yourself in another way (E.g. Maybe you want to prefetch up to 5 periods).

Do you see were I'm coming from?

ln-12 commented 6 years ago

Please have a look at #86 :wink:

ln-12 commented 6 years ago

Is there anything wrong with this PR which prevents your from accepting it?

crismaver1993 commented 6 years ago

Our team is using the library in the project, and doing a regression the QA team found that if we are in a week that shares two months (today is Tuesday October 2 and the week starts with Sunday = September 30) the items that I have today they are not loaded until the user swipes. Previously, the previous table was loaded, the current month and the next. I see that here they comment on the issue, which solution is the most recommended to follow? Did explain?

crismaver1993 commented 6 years ago

Thanks a lot for your detailed explanation!

  1. There is a bug with the loading if you scroll back. This check is probably not necessary

With pull request #86 this bug should be gone, some testing would be appreciated.

  1. We need to think about how we can take into account all days that are visible.

The second problem is a bit harder to fix. In my opinion just saying that the library shouldn't be responsible for prefetching is a mistake. Let me try to explain: assumed your view start somewhere in the middle of month 4 with week view (7 days visible) enabled. When you scroll to the right (future) the onMonthChange listener will be triggered at the end of the 4th month/beginning of the 5th month, let's say when the 1st day of month 5 will become visible. The events stored by the library (for month number 4) are cleared and replaced be the ones for month number 5. The problem now is that you can see events for month 5, but any events for month 4 (of which the last few days are still visisible) are suddenly gone. The same happens when you scroll to the past.

For better understanding I recorded a short video: video

So if you have no prefetching at all there will always be problems like shown in the video, that's why it was included in the original libary. I would suggest the easiest thing is reverting pull request #42 and adjusting the lines around https://github.com/Quivr/Android-Week-View/blob/develop/library/src/main/java/com/alamkanak/weekview/WeekView.java#L888 so that getMoreEvents is called on the 15th of the last month or the 15th of the next month. (I think that's also the reason for "bug" 1, it was wanted behaviour)

ln-12 commented 6 years ago

@crismaver1993 As the authors of this lib don't respond anymore you could either manually clone this repo and apply https://github.com/Quivr/Android-Week-View/pull/89 or simply clone my repo at https://github.com/mc-12/Android-Week-View which already has this fix (and some more) included.

crismaver1993 commented 6 years ago

@crismaver1993 As the authors of this lib don't respond anymore you could either manually clone this repo and apply #89 or simply clone my repo at https://github.com/mc-12/Android-Week-View which already has this fix (and some more) included.

Thanks ! I will do the changes and I will let you know how it works! . By the way, It is a cool library!

crismaver1993 commented 6 years ago

@crismaver1993 As the authors of this lib don't respond anymore you could either manually clone this repo and apply #89 or simply clone my repo at https://github.com/mc-12/Android-Week-View which already has this fix (and some more) included.

Do you add your repo to Maven Central?

ln-12 commented 6 years ago

Not yet. But its pretty simple to add the repo as submodule and add it to your gradle file. Just take a look at the sample project here.

crismaver1993 commented 6 years ago

Not yet. But its pretty simple to add the repo as submodule and add it to your gradle file. Just take a look at the sample project here.

Thanks a lot ! i really apprecciate!