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

WIP: Replacing the calendar after DayView initializing #294

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 19 additions & 3 deletions Source/DayView.swift
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,8 @@ public final class DayView: UIView, TimelinePagerViewDelegate {
}
}

public let dayHeaderView: DayHeaderView
public let timelinePagerView: TimelinePagerView
public private(set) var dayHeaderView: DayHeaderView
public private(set) var timelinePagerView: TimelinePagerView

public var state: DayViewState? {
didSet {
Expand All @@ -60,7 +60,23 @@ public final class DayView: UIView, TimelinePagerViewDelegate {
}
}

public var calendar: Calendar = Calendar.autoupdatingCurrent
public var calendar: Calendar = Calendar.autoupdatingCurrent {
didSet { // TODO: willSet?
dayHeaderView.calendar = self.calendar
timelinePagerView.calendar = self.calendar

// recalculate the selectedDate taking into account
// the difference between new and old timezones
let oldCalendar = state!.calendar
let selectedDate = (self.state?.selectedDate ?? Date())
.dateOnly(calendar: calendar, oldCalendar: oldCalendar)

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

Choose a reason for hiding this comment

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

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

}
}

private var style = CalendarStyle()

Expand Down
2 changes: 2 additions & 0 deletions Source/DayViewState.swift
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ public protocol DayViewStateUpdating: AnyObject {

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

Choose a reason for hiding this comment

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

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

Copy link
Owner

Choose a reason for hiding this comment

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

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

public private(set) var selectedDate: Date
private var clients = [DayViewStateUpdating]()

Expand All @@ -17,6 +18,7 @@ public final class DayViewState {

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

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

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.

notify(clients: clients, moveTo: date)
selectedDate = date
}
Expand Down
8 changes: 8 additions & 0 deletions Source/Extensions/Date+DateOnly.swift
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,12 @@ extension Date {
let returnValue = calendar.date(from: newComponents)
return returnValue!
}

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

Choose a reason for hiding this comment

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

I suggest renaming to something like:

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

Copy link
Owner

Choose a reason for hiding this comment

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

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

var newDate = self.dateOnly(calendar: oldCalendar)
let diff = oldCalendar.timeZone.secondsFromGMT() - calendar.timeZone.secondsFromGMT()
newDate.addTimeInterval(TimeInterval(diff))
return newDate
}
}
16 changes: 15 additions & 1 deletion Source/Header/DayHeaderView.swift
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,13 @@ import DateToolsSwift

public final class DayHeaderView: UIView, DaySelectorDelegate, DayViewStateUpdating, UIPageViewControllerDataSource, UIPageViewControllerDelegate {
public private(set) var daysInWeek = 7
public let calendar: Calendar
public var calendar: Calendar {
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)?
Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

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.

}
}

private var style = DayHeaderStyle()
private var currentSizeClass = UIUserInterfaceSizeClass.compact
Expand All @@ -16,6 +22,14 @@ public final class DayHeaderView: UIView, DaySelectorDelegate, DayViewStateUpdat
didSet {
state?.subscribe(client: self)
swipeLabelView.state = state

// Fixes the "jump" of the pager from today to the selected date. This is noticeable
// when we are on a date outside of today's week, and try to change the time zone.
let previousSelectedDate = state!.selectedDate // day selected before timezone change
let vc = makeSelectorController(startDate: beginningOfWeek(previousSelectedDate))
vc.selectedDate = previousSelectedDate
currentWeekdayIndex = vc.selectedIndex
pagingViewController.setViewControllers([vc], direction: .forward, animated: false, completion: nil)
Copy link
Owner

Choose a reason for hiding this comment

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

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.

}
}

Expand Down
10 changes: 9 additions & 1 deletion Source/Header/DaySymbolsView.swift
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,15 @@ import UIKit

public final class DaySymbolsView: UIView {
public private(set) var daysInWeek = 7
private var calendar = Calendar.autoupdatingCurrent
public var calendar = Calendar.autoupdatingCurrent {
didSet {
// TODO: I honestly don't know if I should be doing this. Changes to
Copy link
Owner

Choose a reason for hiding this comment

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

I think, it's a good practice. The API suggest the possibility of changing the Calendar, what if e.g. 1st day of week has changed? In that case reconfiguration would be necessary.

Copy link
Owner

Choose a reason for hiding this comment

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

It's correct approach. API allows changing of the Calendar, therefore, anything could change, including the index of the first weekday.

// the timezone should not affect the displayed days of the week,
// however, just in case, I am reconfiguring this view
configure()
setNeedsLayout()
}
}
private var labels = [UILabel]()
private var style: DaySymbolsStyle = DaySymbolsStyle()

Expand Down
8 changes: 7 additions & 1 deletion Source/Header/SwipeLabelView.swift
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,13 @@ public final class SwipeLabelView: UIView, DayViewStateUpdating {
case Backward
}

public private(set) var calendar = Calendar.autoupdatingCurrent
public var calendar = Calendar.autoupdatingCurrent {
didSet {
// I doubt that someone will use this view without a state, but
Copy link
Owner

Choose a reason for hiding this comment

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

It's good to keep CK modular, as there are already use-cases when a few CK modules are used on their own or configured by the developer in a non-standard way. So, it's a great idea to keep the modularity going.

// I consider it necessary to update text label for such a situation
updateLabelText()
Copy link
Owner

Choose a reason for hiding this comment

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

Yes, it's correct approach. SwipeLabelView should allow to be used without other modules, e.g. when the developer would like to create a custom view.

}
}
public weak var state: DayViewState? {
willSet(newValue) {
state?.unsubscribe(client: self)
Expand Down
36 changes: 31 additions & 5 deletions Source/Timeline/TimelinePagerView.swift
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,17 @@ public final class TimelinePagerView: UIView, UIGestureRecognizerDelegate, UIScr
public weak var dataSource: EventDataSource?
public weak var delegate: TimelinePagerViewDelegate?

public private(set) var calendar: Calendar = Calendar.autoupdatingCurrent
public var calendar: Calendar = Calendar.autoupdatingCurrent {
didSet {
// changing timezone in loaded pages
pagingViewController.children.forEach {
let vc = $0 as! TimelineContainerController
Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

My main suggestion for this pull request would be instead of modifying the existing TimelineContainerController s , just create a set of new ones with the new calendar and reload with the new data.

It would be much simpler programmatically, as the newly created controllers would have a clean state, instead of a modified one, which would make it easier to reason about...

... And would remove the need to use hacks such as in updateTimeline and func pageViewController(_ pageViewController: UIPageViewController, didFinishAnimating finished: Bool, previousViewControllers: [UIViewController], transitionCompleted completed: Bool)

let oldCalendar = vc.timeline.calendar
vc.timeline.date = vc.timeline.date.dateOnly(calendar: calendar, oldCalendar: oldCalendar)
vc.timeline.calendar = calendar
}
}
}

public var timelineScrollOffset: CGPoint {
// Any view is fine as they are all synchronized
Expand Down Expand Up @@ -173,7 +183,11 @@ public final class TimelinePagerView: UIView, UIGestureRecognizerDelegate, UIScr

private func updateTimeline(_ timeline: TimelineView) {
guard let dataSource = dataSource else {return}
let date = timeline.date.dateOnly(calendar: calendar)
// I don't know under what circumstances, but I know for sure that
// a situation is possible when these calendars have different time
// zones. It is in this case that the call below will prevent a bug
// that leads to "jumps" after days when you swipe to the left
let date = timeline.date.dateOnly(calendar: calendar, oldCalendar: timeline.calendar)
let events = dataSource.eventsForDate(date)
let day = TimePeriod(beginning: date,
chunk: TimeChunk.dateComponents(days: 1))
Expand Down Expand Up @@ -437,7 +451,7 @@ public final class TimelinePagerView: UIView, UIGestureRecognizerDelegate, UIScr
// MARK: UIPageViewControllerDataSource

public func pageViewController(_ pageViewController: UIPageViewController, viewControllerBefore viewController: UIViewController) -> UIViewController? {
guard let containerController = viewController as? TimelineContainerController else {return nil}
guard let containerController = viewController as? TimelineContainerController else {return nil}
let previousDate = containerController.timeline.date.add(TimeChunk.dateComponents(days: -1), calendar: calendar)
let vc = configureTimelineController(date: previousDate)
let offset = (pageViewController.viewControllers?.first as? TimelineContainerController)?.container.contentOffset
Expand All @@ -446,7 +460,7 @@ public final class TimelinePagerView: UIView, UIGestureRecognizerDelegate, UIScr
}

public func pageViewController(_ pageViewController: UIPageViewController, viewControllerAfter viewController: UIViewController) -> UIViewController? {
guard let containerController = viewController as? TimelineContainerController else {return nil}
guard let containerController = viewController as? TimelineContainerController else {return nil}
let nextDate = containerController.timeline.date.add(TimeChunk.dateComponents(days: 1), calendar: calendar)
let vc = configureTimelineController(date: nextDate)
let offset = (pageViewController.viewControllers?.first as? TimelineContainerController)?.container.contentOffset
Expand All @@ -459,7 +473,19 @@ public final class TimelinePagerView: UIView, UIGestureRecognizerDelegate, UIScr
public func pageViewController(_ pageViewController: UIPageViewController, didFinishAnimating finished: Bool, previousViewControllers: [UIViewController], transitionCompleted completed: Bool) {
guard completed else {return}
if let timelineContainerController = pageViewController.viewControllers?.first as? TimelineContainerController {
let selectedDate = timelineContainerController.timeline.date
// Unfortunately, I cannot explain this change in detail. I fiddled
// with a bug in which some days were skipped when swiping, completely
// desperate and wrote this. To my surprise, this solved the problem.
// I'll do some research on the reasons later.

// Попытка пофиксить сбой хедера с перескоком через число
var selectedDate = timelineContainerController.timeline.date
selectedDate = selectedDate.dateOnly(calendar: self.state!.calendar, oldCalendar: timelineContainerController.container.timeline.calendar)

// попытка пофиксить баг с уменьшением и свайпом
timelineContainerController.timeline.date = selectedDate // Я хз как и какие последствия оно имеет, но это сработало
timelineContainerController.timeline.calendar = self.state!.calendar

delegate?.timelinePager(timelinePager: self, willMoveTo: selectedDate)
state?.client(client: self, didMoveTo: selectedDate)
scrollToFirstEventIfNeeded()
Expand Down