[WIP] Custom event views support

[RareScrap]

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.

[richardtop]

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:

• CalendarKit is a modular framework and we sholud move it into that direction further
• However, all of the modules are tightly coupled now (e.g. event editing requires coordination of 4-5 classes together)
• To avoid further coupling of the modules, I propose a simple API that would solve the issue

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:

DayView

This method registers the EventView class you want

func registerEventViewClass(_ eventViewClass: EventView.Type) // Should be a subclass of EventView

dayView.registerEventViewClass(MyCustomEventView.self)

DayViewDelegate

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)
	}
}

What do you think of this approach?

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.

[RareScrap]

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

Way to provide a custom EventView

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 EventDescriptors and thus make it so that there are different types of custom views in same timeline.

Place to register views

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.

Your API for decoupling modules

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?

[richardtop]

Sorry for brevity of my reply, on mobile.

1. The reason is not to be similar with the table view interface or performance. The reason is to keep the data source/delegate interfaces clean and simple.
2. Yes, the approach I've suggested wouldn't work for multiple cell classes
3. How about a different approach? Adding an eventViewClass property to the descriptor. This would allow Timeline to instantiate the EventView.

Also, the change could be put in place without introduction of new protocols, that would keep the library simple.

[RareScrap]

@Rich86man

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

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

[RareScrap]

@richardtop Here's a sketch of how I see support for custom views.

Key points:

1. 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()
      }

      // ...
  }

2. In the 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()
    }
  }

3. EventViews (including custom ones) are saved by the timeline for further reuse
4. Data bind occurs in the overridden updateWithDescriptor(event:) method inside the custom view:

override open func updateWithDescriptor(event: EventDescriptor) {
    super.updateWithDescriptor(event: event)
    subviews[2].backgroundColor = event.color // for example
}

[richardtop]

Yes, these properties should be read-only, as the CalendarKit doesn't modify them.

[RareScrap]

But in this case, we cannot use EventDescriptor as a generic type for custom descriptors. I find it quite convenient:

var event: EventDescriptor
if (Bool.random()) {
    event = Event()
} else {
    event = MyEvent()
}
// imposible with read-only properties
event.text = info.reduce("", {$0 + $1 + "\n"})
event.color = colors[Int(arc4random_uniform(UInt32(colors.count)))]

[richardtop]

Yes, but that's not a requirement by the library.
You're free to create own, stricter protocol requirement and use it. Meanwhile, it would inherit from EventDescriptor, so it would be perfectly compatible.

[richardtop]

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.

[RareScrap]

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

Click to expand

[RareScrap]

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

[richardtop]

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: #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

[richardtop]

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:

CalendarKit/CalendarKitDemo/CalendarApp/ExampleController.swift

Line 127 in 3eb1a83

print("Event has been selected: \(descriptor) \(String(describing: descriptor.userInfo))")

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.

[richardtop]

Btw, feel free to email me at [email protected] or reach via Telegram https://t.me/richardtop to discuss this feature more in detail.