WIP: Replacing the calendar after DayView initializing by RareScrap · Pull Request #294 · richardtop/CalendarKit

RareScrap · 2020-12-18T06:41:06Z

No description provided.

richardtop · 2020-12-18T07:09:11Z

Source/DayView.swift

+      state = newState
+
+      // TODO: Shound we redraw dayView and its subvies? Or we should do this when calendar in state was changed?
+//      setNeedsDisplay()


I think yes, as the 1st day of the week is based on the Calendar, which could change. So it there should be a call to setNeedsDisplay.
Also all of the subviews have to be redrawn.

What about dayview's state? Should we redrawing subvies in response to a changes in state?

I believe so, since that wouldn't happen too often. A change in state means a change in what should be displayed, so that should trigger a full relayout & redraw cycle.
I think, the best is to use setNeedsLayout, as the redraw will be triggered by the system when needed.

richardtop · 2020-12-18T07:10:38Z

Source/Timeline/TimelinePagerView.swift

+  public var calendar: Calendar = Calendar.autoupdatingCurrent {
+    didSet {
+      pagingViewController.viewControllers?.forEach {
+        let vc = $0 as! TimelineContainerController


Please use an if-let or guard-let construct here.

Do you think it's worth doing it? After all, TimelinePagerView only works with TimelineContainerControllers, and I think that it is better to have a crash in such a place than to continue working with a controller of a different type.

It would be simpler to modify, e.g. to use a protocol instead of a concrete class TimelineContainerController.

I'd go for a safe solution without a crash here, although I agree that it's highly unlikely that a crash will happen.

richardtop · 2020-12-18T07:15:09Z

Source/DayView.swift

+
+      // TODO: Shound we redraw dayView and its subvies? Or we should do this when calendar in state was changed?
+//      setNeedsDisplay()
+//      self.subviews.forEach { $0.setNeedsDisplay() }


Not sure whether the call should happen here, or at the respective subView. I'd recommend the latter. Think of using some of the subviews on their own, without being shown in the DayView.

I agree with the same opinion. It seems to me that subviews redrawing must be triggered by a change in its state.

Yeah, a state or a calendar property, depending on which way you decide to go.

richardtop · 2020-12-18T07:15:54Z

Source/Header/DayHeaderView.swift

 public final class DayHeaderView: UIView, DaySelectorDelegate, DayViewStateUpdating, UIPageViewControllerDataSource, UIPageViewControllerDelegate {
  public private(set) var daysInWeek = 7
-  public let calendar: Calendar
+  public var calendar: Calendar


Should there be some reaction to this event? e.g. re-drawing all of the subviews, or propagating the calendar change further?

richardtop · 2020-12-18T08:02:04Z

Source/DayView.swift

+      dayHeaderView.calendar = self.calendar
+      timelinePagerView.calendar = self.calendar
+
+      let date = self.state?.selectedDate ?? Date()


If possible, please omit self. I suggest quickly looking thru CONTRIBUTING.md with the code style examples. Not a requirement, I could fix those issues myself, after your pull request gets merged. Good to keep the library in a common style.

richardtop · 2020-12-20T11:50:33Z

Now I think, the best way to handle this is to simply propagate the Calendar change on a view-by-view basis (e. g. from DayView to Header and other sibling views) and not thru a common DayViewState.

The reason for this is simple: some views may need to have the access to the Calendar property, but don't need to subscribe to the State notifications:
DaySymbolsView is one of such views.

public init(daysInWeek: Int = 7, calendar: Calendar = Calendar.autoupdatingCurrent) {

Of course, we could propagate State to the DaySymbolsView also, but wouldn't that complicate the library?

However, not all TODOs have been explained and solved

RareScrap · 2021-01-20T04:46:48Z

After many weeks of work and bug fixing, I am very happy to present this solution to you (seriously, I think I’m one step closer to alcoholism). I can't say that the feature is fully implemented, but I think that a significant part of the work is over. I would love to hear your comments and ask you to help me with testing. To be honest, it is very difficult for me to come up with such a set of tests that would cover ALL possible options for the behavior of the CalendarKit when changing the timezone, but I checked everything I could and everything works well.

I will update the example app a little later so that you can test this feature more easily. I will also fix conflicts a little later.

richardtop · 2021-01-21T07:53:12Z

Hi, thanks for the update. I'll get back to you soon. I hope to check it this weekend.

richardtop · 2021-02-15T00:07:29Z

Source/DayViewState.swift


 public final class DayViewState {
  public private(set) var calendar: Calendar
+//    willSet {} // TODO: I think this needs to be implemented, cause timezone can change without creating a new state object


I suggest ensuring that changing the timezone will always create a new state object

richardtop · 2021-02-15T00:09:22Z

Source/DayViewState.swift


  public func move(to date: Date) {
    let date = date.dateOnly(calendar: calendar)
+//    if (date == selectedDate) { return } // TODO: If necessary? Just trying to make sure that willMoveTo not be called if we are already on this page


I think, adding if (date == selectedDate) { return } condition is not needed and would only lead to more complicated debugging. If there are some issues in the propagating the date to the modules (e.g. some methods are called twice), it should be visible as early as possible.

richardtop · 2021-02-15T00:10:36Z

Source/Extensions/Date+DateOnly.swift

  }
+
+  /// Cuts off the time, leaving the beginning of the day for the new time zone
+  func dateOnly(calendar: Calendar, oldCalendar: Calendar) -> Date {


I suggest renaming to something like:

func dateOnly(from: oldCalendar: Calendar, to: newCalendar: Calendar) -> Date {

richardtop · 2021-02-15T00:16:31Z

Source/Header/DayHeaderView.swift

+    didSet {
+      daySymbolsView.calendar = calendar
+      swipeLabelView.calendar = calendar
+      configure() // TODO: Should I do it that way or better just to set a new vc without adding subview (like in state#didSet)?


I think, the proper solution is to refactor the configure() function and extract the addSubview portion of it, so that it's called only when needed.
Otherwise, looks good.

richardtop · 2021-02-15T00:17:48Z

Source/Header/DayHeaderView.swift

+      let vc = makeSelectorController(startDate: beginningOfWeek(previousSelectedDate))
+      vc.selectedDate = previousSelectedDate
+      currentWeekdayIndex = vc.selectedIndex
+      pagingViewController.setViewControllers([vc], direction: .forward, animated: false, completion: nil)


I think, any glitches during the configuration process are OK, as it should happen "behind the scenes", i.e. when the DayViewController is not displayed or obscured by another view.

richardtop · 2021-02-15T00:44:00Z

Source/Header/DayHeaderView.swift

+    didSet {
+      daySymbolsView.calendar = calendar
+      swipeLabelView.calendar = calendar
+      configure() // TODO: Should I do it that way or better just to set a new vc without adding subview (like in state#didSet)?


I suggest refactoring the configure method and extracting all of the code that should run only once to a separate method (which also calls configure). The new configure method should be able to be run multiple times during the lifecycle of the view.

richardtop · 2021-02-15T00:44:49Z

Source/Extensions/Date+DateOnly.swift

  }
+
+  /// Cuts off the time, leaving the beginning of the day for the new time zone
+  func dateOnly(calendar: Calendar, oldCalendar: Calendar) -> Date {


I suggest renaming to something akin to func dateOnly(from: oldCalendar: Calendar, to: newCalendar: Calendar) -> Date {

richardtop · 2021-02-15T00:46:43Z

Source/DayViewState.swift


  public func move(to date: Date) {
    let date = date.dateOnly(calendar: calendar)
+//    if (date == selectedDate) { return } // TODO: If necessary? Just trying to make sure that willMoveTo not be called if we are already on this page


I suggest against including such statements. If willMoveTo is called multiple times, it might indicate an error. Errors should be visible as early as possible.

Returning early if date hasn't changed would only complicate debugging. I'm for very simple DayViewState with as little logic as possible.

richardtop · 2021-02-15T00:47:26Z

Source/DayViewState.swift


 public final class DayViewState {
  public private(set) var calendar: Calendar
+//    willSet {} // TODO: I think this needs to be implemented, cause timezone can change without creating a new state object


I suggest that changing TimeZone (or Calendar, in this case) should always trigger creation of a new DayViewState.

richardtop · 2021-02-15T00:47:53Z

Source/DayView.swift

+      // setting a new state
+      let newState = DayViewState(date: selectedDate, calendar: calendar)
+      state = newState
+      newState.move(to: selectedDate) // TODO: Why I added this :I?


.... To send the new date to all the clients?

A sketch of replacing the calendar after DayView initializing

b4fa5b9

RareScrap mentioned this pull request Dec 18, 2020

Set timezone for DayView after initialization #291

Open

3 tasks

richardtop reviewed Dec 18, 2020

View reviewed changes

Bugfixes and the first result that can be called stable��

d78bfe5

However, not all TODOs have been explained and solved

RareScrap force-pushed the post_init_calendar_changing branch from 119e96c to d78bfe5 Compare January 20, 2021 04:33

richardtop reviewed Feb 15, 2021

View reviewed changes

Conversation

RareScrap commented Dec 18, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richardtop commented Dec 20, 2020

Uh oh!

RareScrap commented Jan 20, 2021

Uh oh!

richardtop commented Jan 21, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants