werediver / StyleSheet

Reusable and composable UI styles
MIT License
23 stars 3 forks source link

Ordering styles based on type #6

Closed cfilipov closed 5 years ago

cfilipov commented 5 years ago

Since the styles are applied in the order they are defined it's possible for a more general style to override a more specific style. Consider the following:

let stylesheet = StyleSheet(styles: [
    Style<Any, MyCustomView> {
        $0.backgroundColor = .yellow
    },
    Style<Any, UIView> {
        $0.backgroundColor = .red
    }
])

One would hope that all views would be styled .red except for those that that are MyCustomView which should be styled as .yellow. However, since these are applied in the order defined we end up with all views being .red. As it stands today, it's up to the programmer to keep track of this and ensure the styles are in the proper order. This gets even harder when you have to track the order taking both the target and the marker into account.

I've been trying to solve this by sorting the styles based on the metatype. The goal is to sort the styles based on the target and then the marker so that the least specific styles are applied before the more specific styles. I've had limited success and I was wondering if you had any suggestions.

Unfortunately you can't use is and as operators on the metatype, at the point where styles are defined we don't have any instances to use those against.

Sorting based on the target is actually doable since targets are expected to be either UIView or UIViewController so we can take advantage of objective-c's runtime introspection to sort styles using class_getSuperclass on the Target type.

The Marker is another story though since it can be anything. Swift introspection is much more limited, and again, without an instance even Mirror is unhelpful. If all markers are annotated with @objc then we can make use of protocol_conformsToProtocol.

The methods protocol_conformsToProtocol and class_getSuperclass wont work with Any. This can be worked around by first checking if the metatype equals Any.self in the comparator.

Sorting happens only once after creating the styles so applying styles should not incure any additional overhead.

The biggest problem with this approach is that you have to remember to use @objc on all you markers. I haven't found a way to constrain markers to only objective c protocols so failure to do this would be a runtime error.

Thoughts?

cfilipov commented 5 years ago

Here's an implementation of the above: [75ad58...]

The unit test testOrderOfStylesRev would fail on master for the reasons mentioned above and passes on the sorted-styles branch.

In order to make this work several breaking changes were made:

  1. Styles are created by passing the types as parameters to init rather than specifying them as generic parameters.
  2. Marker protocols must now be annotated with @objc, the nice thing is that it's a compile error to forget to do so.
  3. Only NSObject subclasses can be passed as targets.
  4. Instead of using Any as a marker you simply pass nil or leave out that parameter.
  5. Since the marker is an optional parameter the order has been switched.
  6. Since the types are passed as arguments you have to use .self.
@objc protocol WarningStyle {}

let stylesheet = StyleSheet(styles: [
    Style(UILabel.self, WarningStyle.self) { 
        $0.backgroundColor = .yellow
    },
    Style(UIView.self) {
        $0.backgroundColor = .red
    }
])

Not having to worry about the order of styles and just being able to trust that the right style will be applied based on the types alone removes the possibility for several confusing bugs I encountered when order mattered.

werediver commented 5 years ago

Okay, you're trying to enforce some kind of a "natural" order for style application. It makes good sense for a flat style list, but we have a tree structure: Style is a StyleApplicator, we collect StyleApplicator instances in a StyleSheet, but a StyleSheet is a StyleApplicator too, so you can group your styles hierarchically (the top level style could consist of a group of label styles, button-styles, a group of screen-A-specific styles, screen-B-specific styles, etc.).

As I understand, your approach can enforce ordering in a flat group, but that's it. So it's only a partial ordering which, I'm afraid, could strengthen the confusion.

A more radical approach would be to flatten the style tree and enforce an ordering on the resulting flat structure. Well, it might be an option, but then I'd first investigate if there is a nice and reliable way to define that order.

cfilipov commented 5 years ago

I didn't realize at the time that you could define nested stylesheets, your example code hadn't demonstrated that. But now that you mention it, it's a good point. I think usage instructions in the README would be a good addition, I'd be happy to help with a PR on that as well if you're open to it.

That said, I don't understand the advantage of being able to define hierarchal stylesheets. In the absence of natural ordering they can help with the problem I outlined by allowing you to group styles by UIView subclass thus providing some consistency with how they are applied. However, it is up to the developer to remember to group them, otherwise an unexpected style may be applied.

I believe natural ordering by type makes stylesheet hierarchy unnecessary. There is already a hierarchy implicitly present in the style definitions, and one would naturally expect that styles be applied based on that. My suggestion is to enforce a flat list of styles in the stylesheet, much like CSS. Each style initializer acts as a limited CSS selector which can only match against class and/or protocol.

This is something I feel is very important to the usability of this library. Before implementing ordering I kept running into subtle bugs caused by applying the wrong style and it was tricky to reason about the order every time I added a new style.

werediver commented 5 years ago

So, I see a number of action points here.

First, for the current mechanism:

We need separate tickets for this.

Second, for the proposed flat style list:

It might be worth creating separate tickets for this as well. I'd tag this ticket as "idea" and close as "doesn't fit the current design".

The first part is clear. The second part is [perceived by me as] an interesting experiment.

I'm also open for collaboration on all of this, but excuse me if I am picky or rejecting something; this project started as a micro-framework and is still quite compact and [hopefully] clean, so I'd like to keep as lightweight [and clean] as possible.

As a side note, nowadays I also feel somehow uncomfortable having zero unit-tests in this project, and I do have an experimental overhaul branch with a more testable implementation, but it wasn't finalized in time and now is in a half-forgotten state.

cfilipov commented 5 years ago

I don't think the addition of sorting styles go against your goal of keeping this framework light weight and clean. It's a pretty small addition in terms on lines of code, and the added logic is cleanly inserted as an additional call to sort during initialization.

I don't take any offense to your rejection of any proposals. I'll continue to maintain my fork which works for my purposes and if anything looks interesting to you I'm happy to collaborate.

werediver commented 5 years ago

Because the current tree-like style registry is not suitable for style ordering and style ordering requires some investigation, I close this ticket as an accepted idea and propose to track further progress in this direction in the ticket #10.