-
Notifications
You must be signed in to change notification settings - Fork 3
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
Ordering styles based on type #6
Comments
Sorting the styles by target and marker types. See issue werediver#6
Here's an implementation of the above: [75ad58...] The unit test In order to make this work several breaking changes were made:
@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. |
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: 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. |
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. |
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. |
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 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. |
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. |
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:
One would hope that all views would be styled
.red
except for those that that areMyCustomView
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
andas
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
orUIViewController
so we can take advantage of objective-c's runtime introspection to sort styles usingclass_getSuperclass
on theTarget
type.The
Marker
is another story though since it can be anything. Swift introspection is much more limited, and again, without an instance evenMirror
is unhelpful. If all markers are annotated with@objc
then we can make use ofprotocol_conformsToProtocol
.The methods
protocol_conformsToProtocol
andclass_getSuperclass
wont work withAny
. This can be worked around by first checking if the metatype equalsAny.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?
The text was updated successfully, but these errors were encountered: