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.

Sign up for GitHub

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

Jump to bottom

WIP: Replacing the calendar after DayView initializing #294

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

WIP: Replacing the calendar after DayView initializing #294

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b4fa5b9
A sketch of replacing the calendar after DayView initializing
RareScrap Dec 18, 2020
d78bfe5
Bugfixes and the first result that can be called stable
RareScrap Jan 20, 2021
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
21 changes: 18 additions & 3 deletions Source/DayView.swift
View file
Open in desktop
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,22 @@ public final class DayView: UIView, TimelinePagerViewDelegate {
}
}

public var calendar: Calendar = Calendar.autoupdatingCurrent
public var calendar: Calendar = Calendar.autoupdatingCurrent {
didSet {
// TODO: Should we observe calendar in state rather then change it manualy?
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.

let newState = DayViewState(date: date, calendar: calendar)
newState.move(to: date)
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.

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

}
}

private var style = CalendarStyle()

Expand Down
2 changes: 1 addition & 1 deletion Source/Header/DayHeaderView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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?


private var style = DayHeaderStyle()
private var currentSizeClass = UIUserInterfaceSizeClass.compact
Expand Down
9 changes: 8 additions & 1 deletion Source/Timeline/TimelinePagerView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,14 @@ 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 {
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.

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)

vc.timeline.calendar = calendar
}
}
}

public var timelineScrollOffset: CGPoint {
// Any view is fine as they are all synchronized
Expand Down