Open RareScrap opened 3 years ago
Hi, thanks a lot for your pull request! Highly appreciate! I've checked out your request and found few issues that make it hard to fit to the existing CalendarKit architecture.
So, before going further with development, I'd like to clarify a use-case that you'd like to solve. Could you please share design mockups of how the end result should look like? That would make our discussion more concrete and grounded in the reality.
As for your pull request, here are few issues that need to be addressed:
From my understanding, you'd like to have a custom EventView with buttons and be able to react to those button events.
I suggest modifying the API the following way:
This method registers the EventView
class you want
func registerEventViewClass(_ eventViewClass: EventView.Type) // Should be a subclass of EventView
dayView.registerEventViewClass(MyCustomEventView.self)
This method will be called just after the DayView
updates the EventView
with the EventDescriptor
. Since it's possible to get an EventDescriptor
by having a reference to the EventView
, there is no need to have other parameters.
func dayView(_ dayView: DayView, didUpdateEventView: EventView)
In your example, to connect the button with some other target you'll need to implement something similar to this:
func dayView(_ dayView: DayView, didUpdateEventView eventView: EventView) {
if let myEventView = eventView as? MyCustomClass {
myEventView.button.addTarget(self, action: #selector(buttonDidPress:), for: .touchUpInside)
}
}
It won't work if you need to use 2 or more different kinds of EventView. But if that's the case, I could think of slightly modifying this architecture. The goal is to keep the API clean, simple and extensible.
@richardtop
Thanks for the quick response. I compared mine and your approach to the implementation of this feature and they are very similar:
1) We both want to use EventView
as the superclass for custom views
2) And we want to provide a convenient mechanism for using our own custom views
However, we see the last point in different ways.
I want the TimelineView
to take instances of custom event views from TimelineViewAppearance
, and you want to register EventView
subclasses to provide the ability to use different custom event views in the same timeline.
I do not fully understand why we need to implement EventView
subslasses registration. As I understand it, we are doing this so that working with CalendarKit is understandable for most iOS developers who are used to working with UITableView
. However, registering a UITableViewCell
is needed only in order to be able to receive new or existing cell instances by identifier. And this is justified in a situation where the UITableView
implies a long scrolling. But in TimelineView
the situation is different: the scrolling area is rather limited. Why bother with registering custom views at all when we can just make a function that returns a custom EventView
instance from a delegate?
I find that registering views can be useful in views with a large scrolling area. And such a situation may arise if, for example, there is an opportunity to enlarge TimelineView
with a pinch gesture.
Now I'm not sure that registering views is the right thing to do. Moreover, we can return different EventView
subclass instances for different EventDescriptor
s and thus make it so that there are different types of custom views in same timeline.
And why should we register views in DayView
? DayView
combines TimelivePagerView
, DayViewHeader
and AllDayView
, although there may be a situation when the screen should only display TimelineView
. I believe that we should transfer the registration of event view subclasses to TimeliveView
instead of DayView
and implement a function in TimelivePagerView
will allow you to set the custom EventView
for all timelines.
In addition, we have another problem - we need to give the opportunity to create different event views for the TimelineView
and for AllEventView
, because what fits into the EventView
on the TimelineView
may not fit into the EventView
on the AllEventView
.
To avoid further coupling of the modules, I propose a simple API that would solve the issue
Could you tell us more about this API?
Sorry for brevity of my reply, on mobile.
Also, the change could be put in place without introduction of new protocols, that would keep the library simple.
@Rich86man
How will registering view classes make lib's code cleaner and easier? At the moment, I see only one reason to implement registration of views - performance (when the scroll area becomes very large due to the pinch gesture). In addition, I do not think that registering a viewclass is a good idea, because in this case we oblige the CalendarKit to create custom event views on our own. I believe that we need a method that will return an already created instance of a custom view. I think that the logic of creating a view is best left to the user of the library.
I see no reason to encapsulate the logic for creating a custom event view instance inside the TimelineView
. As I wrote in the paragraph above, it is better to delegate the creation of the view instance to the user. Just like the UITableView
does. I also doubt that storing a viewClass into a EventDescriptor
is a good idea. I think that we should be given the opportunity to create our own EventDescriptor`, the type of which will be used to determine what type of custom event views to create.
And although my approach suggests a new protocol, it removes the task of creating a custom view instance from TimelineView
. I believe this is a plus of the approach I proposed
I will present the implementation of my approach a little later, as soon as I fix the bugs.
@richardtop Here's a sketch of how I see support for custom views.
Key points:
The type of EventDescription
instance returned to the eventsForDate(date:) -> [EventDescriptor]
is used to determine the type of the custom event view and store additional information specific to the custom view:
private func generateEventsForDate(_ date: Date) -> [EventDescriptor] {
// ...
let event: EventDescriptor
if (Bool.random()) {
event = Event()
} else {
event = MyEvent()
}
// ...
}
timelineView(timeloneView:viewFor:) -> EventView
method the event view is instantiated based on the type of the descriptor:
func timelineView(_ timeloneView: TimelineView, viewFor event: EventDescriptor) -> EventView {
if (event is MyEvent) {
return MyEventView()
} else {
return DefaultEventView()
}
}
updateWithDescriptor(event:)
method inside the custom view:
override open func updateWithDescriptor(event: EventDescriptor) {
super.updateWithDescriptor(event: event)
subviews[2].backgroundColor = event.color // for example
}
Hi, commenting on your thoughts:
we oblige the CalendarKit to create custom event views on our own.
That's the point, event creation and configuration is left for the CalendarKit. The client just provides the appropriate Descriptor
that fully describes the event. It's up to the CalendarKit to create and configure it.
I think that we should be given the opportunity to create our own EventDescriptor`, the type of which will be used to determine what type of custom event views to create.
This is currently supported. In your case, having 2 completely different event types could be managed with having multiple EventDescriptor
classes.
What I suggest would work best here is to add a parameter to the EventDescriptor
protocol:
public protocol EventDescriptor: AnyObject {
var startDate: Date {get set}
var endDate: Date {get set}
var isAllDay: Bool {get}
var text: String {get}
var attributedText: NSAttributedString? {get}
var font : UIFont {get}
var color: UIColor {get}
var textColor: UIColor {get}
var backgroundColor: UIColor {get}
var viewClass: EventView.Type {get}
var editedEvent: EventDescriptor? {get set}
func makeEditable() -> Self
func commitEditing()
}
and then, use that information to create the event in the Timeline.
The default descriptor (Event
) would have that property set to EventView.Self
, while if you'd like to have a custom class, you'll have to either create an own EventDescriptor
or set the viewClass
to your desired class.
Also, with the proposed approach it's possible to add any information thru the EventDescriptor
class, which later would be used by the custom EventView
.
func timelineView(_ timeloneView: TimelineView, viewFor event: EventDescriptor) -> EventView {
if (event is MyEvent) {
return MyEventView()
} else {
return DefaultEventView()
}
}
What if there is 3 kinds of descriptors? Or even 5? Then we'll need to add an if-else statement for all of them. Clearly, this information (EventViewType
) could go to the EventDescriptor
itself.
Also, there is a risk of returning an incorrect view for the descriptor, which might result in obscure bugs that are really hard to trace. Keeping all of the relevant information at hand in the EventDescriptor
itself, so that it's all together.
Please, let me know, if the approach I propose is clear, as well as the limitations of using a custom protocol just to provide a type.
@richardtop Thx for you comment.
That's the point, event creation and configuration is left for the CalendarKit. The client just provides the appropriate
Descriptor
that fully describes the event.
I believe that by this step we artificially limit the flexibility of the CalendarKit. A situation may arise when the logic of initializing a custom event view may differ from the usual call of the constructor and setting the frame. I believe that obliging the TimelineView
not only to manage event views, but also to initialize them is a direct violation of the principle of separation of concerns.
This is currently supported. In your case, having 2 completely different event types could be managed with having multiple
EventDescriptor
classes.
This nice because any event can be fully described only using the descriptor subclass. I'm glad that this has already been implemented.
What if there is 3 kinds of descriptors? Or even 5?
I have not tested more than two types of views, because this is not a priority for my organization. If possible, I will take the time to do this testing. But I'm not sure if I have time to do this.
Then we'll need to add an if-else statement for all of them. Clearly, this information (EventViewType) could go to the EventDescriptor itself. Also, there is a risk of returning an incorrect view for the descriptor, which might result in obscure bugs that are really hard to trace. Keeping all of the relevant information at hand in the EventDescriptor itself, so that it's all together.
The descriptor should store information about the event, not a view-type of the event! Because in this case we establish a strong relation between the model layer and the view layer, and this is a wrong approach at the root. If you are so confused about the if-else expression chain, how about using a factory to create custom views?
We also have two other problems: 1) Ability to create custom views for All-Day events 2) Use different layouts of custom views depending on their size. What looks good without overlapping events will look bad otherwise:
I ran into a bug in my typed queue that poses too complex questions for me:
1) For what purposes is there EventView#descriptor
? Why does the view layer keep a reference to the model object?
2) Why is this field optional?
In order for the TimelineView
's reuse queue to be able to work with different types of event views, I began to use the already existing EventView#descriptor
field so that the TimelineViewAppearance
could determine the type of custom view. The descriptors for all event views are taken from the eventsForDate(_ date:)
method of the data source.
I have a suspicion that the EventView#descriptor
field shouldn't really be non optional, because we set it at each place where the event view is created: AllDayView#reloadData()
, TimelinePagerView#create(event:animated:)
and TimelineViewAppearance
(I call updateWithDescriptor()
in dequeue()
so as not to burden the user with the initialization of the descriptor)
Now I cannot answer these two questions. I will be very grateful if you can help me with them
I believe that by this step we artificially limit the flexibility of the CalendarKit. A situation may arise when the logic of initializing a custom event view may differ from the usual call of the constructor and setting the frame. I believe that obliging the TimelineView not only to manage event views, but also to initialize them is a direct violation of the principle of separation of concerns.
No, it's just a general philosophy of the library. I'd prefer to keep it simple. If you need something more complex, then I recommend just forking the library and making changes in your fork.
I have not tested more than two types of views, because this is not a priority for my organization. If possible, I will take the time to do this testing. But I'm not sure if I have time to do this.
It's clear that having more than 2 views would make implementing interface for the CK support cumbersome. Instead of containing every piece of the logic in an individual EventDescriptor
-compliant class, it would have to be in two places (a view creation code and a descriptor code).
To me it seems that your project requires a highly customized solution, so it's best to fork the library and use it as it is.
The descriptor should store information about the event, not a view-type of the event! Because in this case we establish a strong relation between the model layer and the view layer, and this is a wrong approach at the root. If you are so confused about the if-else expression chain, how about using a factory to create custom views?
It's fine, CalendarKit doesn't follow MVC strictly. EventDescriptor
is used as a messaging object and really should be the center of connection of the CK & the client code.
Ability to create custom views for All-Day events
Please, implement this feature either the way I suggested or just fork the library if this approach doesn't fit your project.
Use different layouts of custom views depending on their size. What looks good without overlapping events will look bad otherwise:
It's a known issue: https://github.com/richardtop/CalendarKit/issues/113
The solution is to allow to plug in a custom layout algorithm that would do the layout based on the information provided in the EventDescriptor
array.
The idea could be similar to this:
https://github.com/richardtop/CalendarKit/blob/master/Source/Timeline/SnapTo15MinuteIntervals.swift
https://github.com/richardtop/CalendarKit/blob/master/Source/Timeline/EventEditingSnappingBehavior.swift
For what purposes is there EventView#descriptor? Why does the view layer keep a reference to the model object?
This is in order to figure out which event has been selected / pressed:
Why is this field optional?
I think, it's most likely due to the lifecycle issues. E.g. EventView
in a reuse pool will have that property set to nil.
Btw, feel free to email me at topchiy@protonmail.ch or reach via Telegram https://t.me/richardtop to discuss this feature more in detail.
This is the first draft of the planned feature. I created a WIP pull request as you asked in #175. So @richardtop please give me a feedback while I implementing this feature.