Skip to content
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

Open
wants to merge 64 commits into
base: develop
Choose a base branch
from

Conversation

ln-12
Copy link

@ln-12 ln-12 commented May 1, 2018

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

entropitor and others added 30 commits September 2, 2016 16:08
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
@AndroidDeveloperLB
Copy link

Does this fix this issue too:
#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
Copy link
Author

ln-12 commented May 27, 2018

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
Copy link

Seems it fixes it. But which are the changes that I need to do?
Should I look at this:
#86
Or just here?

@AndroidDeveloperLB
Copy link

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

@ln-12
Copy link
Author

ln-12 commented May 27, 2018

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
Copy link

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
Copy link

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
Copy link
Author

ln-12 commented May 27, 2018

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.

AndroidDeveloperLB added a commit to AndroidDeveloperLB/Android-Week-View that referenced this pull request May 27, 2018
@ln-12
Copy link
Author

ln-12 commented May 27, 2018

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
Copy link

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
Copy link

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
Copy link
Author

ln-12 commented May 27, 2018

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
Copy link

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
Copy link
Author

ln-12 commented May 27, 2018

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

@AndroidDeveloperLB
Copy link

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
Copy link
Author

ln-12 commented May 27, 2018

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
Copy link

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
Copy link
Author

ln-12 commented May 27, 2018

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
Copy link

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

@AndroidDeveloperLB
Copy link

Have you looked at my repo?

@ln-12
Copy link
Author

ln-12 commented May 27, 2018

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
Copy link

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
Copy link

AndroidDeveloperLB commented May 28, 2018

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

#106

Can you please check it out?

@AndroidDeveloperLB
Copy link

ok never mind. I've fixed it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants