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

feat: Available dates range support #37

Closed
wants to merge 14 commits into from

Conversation

TatsuUkraine
Copy link
Contributor

@TatsuUkraine TatsuUkraine commented Jul 14, 2020

Closes: #25, #38

Features

  • ability to define min and max dates
  • ability to define swipeRange for a VisibleRange.days
  • added jumpTo, jumpToToday, jumpToInitialDate, animateToInitialDate methods to controller

Features for discussion

  • support of min/max date for WeekVisibleRange

Checklist

  • remove redundant code
  • fix lint issues
  • update existing tests
  • update example App
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

@TatsuUkraine
Copy link
Contributor Author

@JonasWanke one stuff that I don't know if it worth to support is min/max date for WeekVisibleRange. Question is: what min/max values should be eventually if they're not the first days of the week. Should the package extend it based on the first day of the week value? Or it should work with exact values, even if it means that at the start or at the end of the available range full week potentially could be truncated.

@TatsuUkraine
Copy link
Contributor Author

the case here, I added swipe range support to Days Visible range, which means that dev can create a custom week range period, with 7 days swiping interval

@TatsuUkraine
Copy link
Contributor Author

The biggest change here is scroll area rendering. I replaced how package defines center for it, by adding 2 opposite sliver delegates. One for dates that goes before initial date, and second - for dates in future

@TatsuUkraine TatsuUkraine changed the title Available dates range support feat: Available dates range support Jul 14, 2020
@TatsuUkraine
Copy link
Contributor Author

@JonasWanke do you have any thoughts regarding approach that I described in last comment? Just curious if I should continue to work on this PR, since if you think that it can lead to any problems, or it's something that breaks any of your upcoming features, I can think about different solution here)

lib/src/visible_range.dart Outdated Show resolved Hide resolved
lib/src/visible_range.dart Outdated Show resolved Hide resolved
lib/src/visible_range.dart Outdated Show resolved Hide resolved
lib/src/visible_range.dart Outdated Show resolved Hide resolved
lib/src/controller.dart Outdated Show resolved Hide resolved
_currentlyVisibleDatesListenable = scrollControllers.pageListenable
.map((page) {
return DateInterval(
LocalDate.fromEpochDay(page.floor()),
LocalDate.fromEpochDay(page.ceil() + visibleRange.visibleDays - 1),
this.initialDate.addDays(page.floor()),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the meaning of page now: offset from initialDate? That would be a breaking change and hence something I'd like to avoid

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

basically yes, page (which now can be infinite in both positive and negative directions) is offset for the initial date. It means that the initial date can be changed as well if needed, obviously, it will require some additional work for all-day event Widget)) but overall - having page as an offset of initialDate simplifies some calculations.

One particular stuff that I don't like is the initial date that the user passed into the controller, and the initial date that will be actually defined can be different when passed initialDate can't be actually initial date. For example:

  • it's Week visible range, and initial date, that user passed in, not the first day of the week
  • the visible range was defined for example as 5 days with some end date, and the initial date - not the start date of this range

Like max date 10th, range is 5 days, and the initial date was specified as 8th - so it can't be initial date, since range should start from 6th date

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as for breaking change - doubt that anyone was relying on page indexes like on epochDays value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw, having page as an offset against the initial page potentially should simplify modularization, since in that case, dev won't need to take into account all this internal specific of scroll list building. Instead - he can build his own Viewport layout as he wants to

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if we don't know for sure if someone is using these indices to calculate dates, this is a behavioral change of the public API and hence requires a breaking change. I don't think it's worth the hassle for all users of this package to manually update the package due to this relatively minor modification. Could you please work with the existing page values?

Regarding initialDate: This also seems to be a breaking change if someone is using a WeekVisibleRange

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please work with the existing page values?

You mean return back approach where Page is actually days since the epoch? Or add shift to pageListenable object?

lib/src/date_page_view.dart Outdated Show resolved Hide resolved
lib/src/date_page_view.dart Outdated Show resolved Hide resolved
),
],
center: _centerKey,
slivers: _slivers,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the benefit of two separate slivers? Couldn't we just adjust the offset and limit of the existing sliver?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the benefit is that it is actually infinite in both directions. Also, it simplifies dates calculation since the initial date becomes a center date in that case.

An additional plus with this approach is that - range can be changed dynamically, that potentially should simplify animation feature implementation (I saw it in your todo list). Count values are changeable which means that it will be easier to update min/max values without the need to recalculate offset of the visible date. And overall calculation on scroll become simpler - you can notice in that I basically kept 1 implementation of offset calculation for both Days and Week ranges

lib/src/visible_range.dart Outdated Show resolved Hide resolved
@JonasWanke
Copy link
Owner

@JonasWanke one stuff that I don't know if it worth to support is min/max date for WeekVisibleRange. Question is: what min/max values should be eventually if they're not the first days of the week. Should the package extend it based on the first day of the week value? Or it should work with exact values, even if it means that at the start or at the end of the available range full week potentially could be truncated.

@TatsuUkraine I think the same problem can already occur with DaysVisibleRange: If it's configured with swipeRange = 5, and min and max dates are 1 and 7, respectively, the first view would show dates 1 – 5 and the second view would show dates out of range (6 – 10 in theory). Lets only allow min/max values that align with the swipe range for now, so assert something like (maxDate + 1 - minDate) % swipeRange == 0 if all three properties are set. Similar for WeekVisibleRange: allow only min values whose day of week is the same as firstDayOfWeek, and only allow max values that are the week day before.

@JonasWanke JonasWanke added C: Timetable Component: The actual timetable package T: Feat Type: :tada: New Features labels Jul 15, 2020
@TatsuUkraine
Copy link
Contributor Author

Lets only allow min/max values that align with the swipe range for now, so assert something like (maxDate + 1 - minDate) % swipeRange == 0 if all three properties are set.

Do you think it worth to put such kind of limitation on days range? Like, date range doesn't suffer if last swipe range will be less then provided swipe range. and if I as a dev, put such kind of config, I should be ready for consistences of such decision) in same time, if dev wants to, he can always make rules for min/max date stricter. Like I can imagine that most common case will be render specific month, and have swipe per week, which means that in most common case such limitation will be more a problem than a solution.

But I agree that having this kind of assert in week range is a good idea, since week range should be more stricter than date range

@TatsuUkraine
Copy link
Contributor Author

@JonasWanke I'm just trying to think of most common use cases) and probably most common for days range with limit - limit on one specific month or couple of months, or similar to it, with week range swiping interval. Which means, that dev won't be able to meet this kind of rules in 99% cases. Which means that dev will need to add own visible range implementation very often.

For the week visible range - agree that this kind of check is necessary to meet rule with first days of the week.

In the result - if dev wants to, he can use week visible range, if he wants to be sure that full week should be visible, and if it doesn't meet min/max criteria - he get the error. But if dev wants to have less strict rules - he can use date visible range, calculate first day of the week on his own, and init timetable with these params. Or maybe he doesn't care about first day of the week, and want always render Today as a start date and make available only current month. So he will be able to do both out of the box, without custom Visible range implementation

@JonasWanke
Copy link
Owner

@TatsuUkraine If your render only a specific month, and have swipes per week, wouldn't you use a WeekVisibleRange?

But yeah, if you start with today or something similar, it might make sense to not force a strict alignment of the maximum date. In that case, the final range would contain some days after the maximum date? (So with the example from above, with swipeRange = 5, and min and max dates as 1 and 7, respectively, the first view would show 1 – 5 and the second view would show 6 – 10?) Either way, this should be described in the class comment of DaysVisibleRange

@TatsuUkraine
Copy link
Contributor Author

TatsuUkraine commented Jul 15, 2020

If your render only a specific month, and have swipes per week, wouldn't you use a WeekVisibleRange?

@JonasWanke well yes, unless I want to render just a month, with week interval swipe, that starts at 1st and ends at 30th. And also that tight to the initial date dev specified rather than to the first day of the week

final range would contain some days after the maximum date

yep, WeekRange in that context fits perfectly. DaysRange, on the other hand, allows dev to decide on his own - if he wants to see anything outside restricted month range

Either way, this should be described in the class comment of DaysVisibleRange

For sure, I didn't get to the docs stage yet, since wasn't sure what kind of API and behavior you want to see in the result in this feature)

@TatsuUkraine
Copy link
Contributor Author

@JonasWanke basically, the reason why I didn't put a lot of restriction to DateRange is that in this context DateRange allows dev to decide a lot regarding visible range, swipe range etc. Which should be something, that fits for 99% cases dev needed. WeekRange - is more stricter variant, that has more limitations because of First Day of the Week.

@TatsuUkraine
Copy link
Contributor Author

@JonasWanke so I was looking for a solution against current implementation, but there will be some unwanted side effect when min date is specified because of the way how the date range is rendered. Another option - try to figure out with scroll controller, maybe register controller that will be tight to the actual custom scroll view and hide it behind another controller that will emit days since epoch values. But I'm not sure if it worth to use such hook.

So the question is: what you think if this feature will go in your next release where you're planning to perform global refactoring?

@TatsuUkraine
Copy link
Contributor Author

TatsuUkraine commented Sep 22, 2020

@JonasWanke ping) curious about your thoughts regarding my last comment here)

@JonasWanke
Copy link
Owner

@TatsuUkraine Sorry, I somehow missed that. When integrating this into the refactor, I think the cleanest way would be to keep track of the absolute (epoch) day in TimetableController, and use the relative offset (to the initial date) only in the MultiDateLayout, as other layouts may have different requirements/preferred data. So, basically, your suggestion of a hidden, second controller applied to the new architecture. This hides the implementation details of that layout and should be achievable by using, e.g., a ValueListenable for the TimetableController and a LinkedScrollControllerGroup for the layout, keeping both in sync.

Due to the large structural changes of the code during the refactor, I'd like to directly integrate this into the new code, without you having to adjust large parts of the PR if that's fine with you.

@TatsuUkraine
Copy link
Contributor Author

@JonasWanke I'm totally fine with closing this PR, so you could add it directly into your refactor.

As for keeping epoch day as a starting point, not sure if it's the best way to tell the truth) it will lead to more complicated solution, especially when it comes to screen rotation or resize, since you will need manually adjust position when this happens. Which means that you will need to track media query. But problem is that Media query can be changed not just because of rotation, but also because of various reasons, like keyboard opening etc. And with huge amount of data it potentially can lead to the frame drop, since timetable will rerender and repaint even when it shouldn't

@TatsuUkraine
Copy link
Contributor Author

@JonasWanke do you want me to close this PR?

@JonasWanke
Copy link
Owner

it will lead to more complicated solution, especially when it comes to screen rotation or resize, since you will need manually adjust position when this happens.

@TatsuUkraine Can you explain why screen rotation or resize events affect the position — wouldn't it just stay the same as we're not using absolute pixel values but only a day and relative progress to the next day? And would that be solved by using an offset relative to the initial date?

Yes, I think we can close this PR. Thanks for the ideas and the code, which will be really handy for the new version :)

@JonasWanke JonasWanke closed this Sep 23, 2020
@TatsuUkraine
Copy link
Contributor Author

Can you explain why screen rotation or resize events affect the position — wouldn't it just stay the same as we're not using absolute pixel values but only a day and relative progress to the next day

@JonasWanke Fix me if I'm wrong, but I can imagine that with epoch day and min/max day range, you will be limiting min and max scroll position so user won't be able to scroll/swipe to the date which is outside of provided range.

If so, it means that as soon as you rotate the screen, day items size will be changed. So difference between 0 (epoch day) and min date position will be changed as well. So on screen size change you will need to update scroll to the valid position as soon as it happens.

With using start date as scroll start position and limiting number of items in past and in future - scroll view will handle it for you.

Overall, I can imagine that even with epoch day approach, there might some workarounds. Question only - which solution (epoch date vs start date as a scroll center day) will bring less complexity in terms of min/max range limit feature implementation)

@TatsuUkraine
Copy link
Contributor Author

@JonasWanke but, with epoch day approach it will be easier to handle dynamic min/max date range update

@AurelianTimu
Copy link

Definition of min and max dates is a must have feature.
There are many use cases that could use it.
I hope it will land in the new version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: Timetable Component: The actual timetable package T: Feat Type: :tada: New Features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Select weeks of a specific month only
3 participants