-
Notifications
You must be signed in to change notification settings - Fork 64
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
Fixes onMonthChange called to late on scrolling to the past and implements a PrefetchingWeekViewLoader class #89
base: develop
Are you sure you want to change the base?
Conversation
Gradle mvn push script local with small changes to fix javadocs for annotations
corrects name of interface in read me instructions
Conflicts: library/src/main/java/com/alamkanak/weekview/WeekView.java library/src/main/res/values/attrs.xml
Don't prefetch, leave it up to the WeekViewLoader
Fix gradle compile errors
Does this fix this issue too: |
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. |
Seems it fixes it. But which are the changes that I need to do? |
Can you please do the needed changes on my fork? Seems quite hard to find the needed changes... |
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. |
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 |
I think the issue still exists (even in your repository), but in a smaller scale. 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. |
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. |
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. |
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. About the fix, you can clone my Kotlin fork here : If you know how to improve it , a PR is welcomed :) Currently there are 2 issues that I still see on this library:
|
About your fix of the all-day, I don't think it's right. Also, the issue I've presented is that the event itself isn't shown till you've scrolled enough. |
Please try out my suggested fix an set |
How can I clone this commit? |
Yeah, that's fine. The relevant changes were commited back in April. |
Well, as I wrote, it always shows the row, even without events: 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). |
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. |
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. |
Yes that's true. I wrote that it's not the same issue. |
Have you looked at my repo? |
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. |
I converted the previous code. If in the previous code there is no null check, it doesn't need to be here either. |
I think I've found an issue in your fork (and the others) : Can you please check it out? |
ok never mind. I've fixed it :) |
54ca15f
to
f8e42cf
Compare
Same as #86, but using separate branch instead of develop branch to avoid adding more commits. Sorry for that mistake!