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

Conversation

RareScrap
Copy link
Contributor

No description provided.

state = newState

// TODO: Shound we redraw dayView and its subvies? Or we should do this when calendar in state was changed?
// setNeedsDisplay()
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 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Owner

Choose a reason for hiding this comment

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

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.

public var calendar: Calendar = Calendar.autoupdatingCurrent {
didSet {
pagingViewController.viewControllers?.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.


// 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() }
Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Owner

Choose a reason for hiding this comment

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

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

@@ -4,7 +4,7 @@ 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
Copy link
Owner

Choose a reason for hiding this comment

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

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

dayHeaderView.calendar = self.calendar
timelinePagerView.calendar = self.calendar

let date = self.state?.selectedDate ?? Date()
Copy link
Owner

Choose a reason for hiding this comment

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

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

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

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

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

@@ -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

@@ -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.

@@ -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 {

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.

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.

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 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.

@@ -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 akin to func dateOnly(from: oldCalendar: Calendar, to: newCalendar: Calendar) -> Date {

@@ -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 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.

@@ -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 that changing TimeZone (or Calendar, in this case) should always trigger creation of a new DayViewState.

// 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?

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.

2 participants