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
257 stars 64 forks source link

Fixes onMonthChange called to late on scrolling to the past and implements a PrefetchingWeekViewLoader class #89

Open ln-12 opened 6 years ago

ln-12 commented 6 years ago

Same as #86, but using separate branch instead of develop branch to avoid adding more commits. Sorry for that mistake!

AndroidDeveloperLB commented 6 years ago

Does this fix this issue too: https://github.com/Quivr/Android-Week-View/issues/105 ? Maybe they are the same? I've created a fork of this repository, and changed it to Kotlin: https://github.com/AndroidDeveloperLB/Android-Week-View I hope it won't be too hard to fix it.

ln-12 commented 6 years ago

Oh, I love Kotlin! ❤️

Yep, I think that's exactly the issue I fixed with this PR. The problem is that they just removed the prefetching stuff from the original code without considering all the consequences (e.g. the bug you mentioned). I also created a fork which already includes this fix as the mentainers of this repo don't seem to be very active. But please recheck it for your situation to see if it works!

AndroidDeveloperLB commented 6 years ago

Seems it fixes it. But which are the changes that I need to do? Should I look at this: https://github.com/Quivr/Android-Week-View/pull/86 Or just here?

AndroidDeveloperLB commented 6 years ago

Can you please do the needed changes on my fork? Seems quite hard to find the needed changes...

ln-12 commented 6 years ago

Please don't care about #86, I forgot to do all the relevant commits in a new branch to avoid that new commits are also added there and just did a new PR.

Basically you just need to let Android Studio convert the PrefetchingWeekViewLoader to Kotlin and adjust the bounds stuff. Here is everything I changed.

AndroidDeveloperLB commented 6 years ago

OK, I think I got it to work, but why did you call it "PrefetchingWeekViewLoader" ? Why did you wrap it around WeekViewLoader ? And what's the mPrefetchingPeriod ?

AndroidDeveloperLB commented 6 years ago

I think the issue still exists (even in your repository), but in a smaller scale. See this video:

device-2018-05-27-134603.zip

Not as bad as the original issue, but still... It doesn't show the events of the partially-displayed day till you show more of it.

ln-12 commented 6 years ago

I implemented it that way as @entropitor suggested it, if I want to contribute that to the lib.

As described in the class file mPrefetchingPeriod is "The amount of periods to be fetched before and after the current period. Must be 1 or greater." That means if you wanna provide data for 1 month, it also requests data for the N months before and after if mPrefetchingPeriod is N.

How did you implement it? Of course you need a prefetching period longer than the amount of visible days as shown here.

ln-12 commented 6 years ago

Wait a second, now I see what the actual problem is. The all day events gap is hidden when there are no more all day events in the visible amount of days. It has nothing to do with this issue.

For that please refer to this commit and set the property to true.

AndroidDeveloperLB commented 6 years ago

Hmmmm... I wanted to avoid forcing the developer to set yet another field for this, so I had it set within the class, reducing the hassle. Probably a bad choice, because you say it should be configurable to the developer. What happens if I use the default one (with 1 as parameter), always?

About the fix, you can clone my Kotlin fork here : https://github.com/AndroidDeveloperLB/Android-Week-View

If you know how to improve it , a PR is welcomed :)

Currently there are 2 issues that I still see on this library:

  1. No support for RTL in terms of mirroring the UI (meaning order of days will be from right to left, for example
  2. Feels a bit slow on old Android devices, compared to Google Calendar app.
AndroidDeveloperLB commented 6 years ago

About your fix of the all-day, I don't think it's right. What you wrote will set the all-day row to always appear, even if there aren't any events there.

Also, the issue I've presented is that the event itself isn't shown till you've scrolled enough.

ln-12 commented 6 years ago

Please try out my suggested fix an set mWeekView.alwaysShowAllDayEventGap(true); I think the event is not shown, because it is hidden inside the collapsed all day header. I just tried it out and it works perfectly well.

AndroidDeveloperLB commented 6 years ago

How can I clone this commit? Visiting the repository (here: https://github.com/mc-12/Android-Week-View ) , it says latest commit was 9 days ago

ln-12 commented 6 years ago

Yeah, that's fine. The relevant changes were commited back in April.

AndroidDeveloperLB commented 6 years ago

Well, as I wrote, it always shows the row, even without events: device-2018-05-27-151127.zip

and if I don't set the flag, the issue I wrote occurs (that events won't be shown till I scroll a bit more).

ln-12 commented 6 years ago

That's exactly how I think this property is supposed to perform but it definitely does not belong to this PR anymore. The prefetching works fine and has nothing to do with all day events. I really don't get what the behaviour you want would look like, sorry.

AndroidDeveloperLB commented 6 years ago

The issue is that for example:

  1. I have an all-day even today (2018-5-28), I scroll so that the left day is tomorrow:

image

This is ok. None of those have all day event.

  1. If I scroll so that I see entire day of today, I see the event :

image

  1. However, if I scroll only partially, I don't see it:

image

ln-12 commented 6 years ago

Ok now i get it, thanks. You shoudl probably take a look at the lines around here to ix this. But as I said, I think is not related to the prefetching issue and this PR.

AndroidDeveloperLB commented 6 years ago

Yes that's true. I wrote that it's not the same issue.

AndroidDeveloperLB commented 6 years ago

Have you looked at my repo?

ln-12 commented 6 years ago

I don't have time to take a deeper look at it right now. If it works, it's fine I guess. But as I see that you are excessively using the "!!" operator which might be dangerous as it could cause null pointer exceptions. Kotlin provides some syntactical ways to avoid it most of the time.

AndroidDeveloperLB commented 6 years ago

I converted the previous code. If in the previous code there is no null check, it doesn't need to be here either. Of course, since the code is huge, it can be shortened and become nicer...

AndroidDeveloperLB commented 6 years ago

I think I've found an issue in your fork (and the others) :

https://github.com/Quivr/Android-Week-View/issues/106

Can you please check it out?

AndroidDeveloperLB commented 6 years ago

ok never mind. I've fixed it :)