Hero: Redesign `heroModifiers` to not be a string API

[kylebshr]

I think the API would be far clearer and easy to use if heroModifers wasn't a string. The benefits of a non "stringly-typed" API are pretty clear, but to outline a few:

• Improved safety
• Compiler checking
• Easier discoverability and documentation

I would love to take on this task, but I wanted to ask which of two directions I should go, and if you might merge a PR like this before I start working on it.

Option 1

Introduce a new Enum with associated values for each Modifier - something like

enum Modifier {
    case scale(CGFloat)
    case fade
    case ...
}

Views would retain the heroModifiers property, but it would become an array of Modifier, used like this: foo.heroModifiers = [.fade, .scale(0.5)].

This design is nice because an enum encapsulates the possible modifiers well, especially with associated values. One drawback is that it's unclear at usage that you can only use one of each Modifier. However, the current String API also has this problem.

Option 2

Introduce a new Modifiers struct with optional properties for each Modifier, something like:

struct Modifiers {
    var scale: CGFloat?
    var fade = false
    var transform: (CGFloat, CGFloat)?
    ...
}

This design is nice because it ensures one of each Modifier is used at the most. Views would also have a heroModifiers property, but would be the type of the struct. This design might be used like this:

foo.heroModifiers.scale = 0.5
foo.heroModifiers.fade = true

I'd love your opinion on this, or would love to know if you're already working on something similar. I'd love to help implement it!

[alexzielenski]

+1 I had this same thought while reviewing this repository and was glad I was not alone when I went to file an issue! Personally I prefer the syntax/feel of using the enum but it seems to me that the struct is the way to go to enforce one of each type of modifier.

One issue I see with both; however, is the inability for users of this library to add their own modifiers, especially since structs and enums in swift are final types. Perhaps have an extra enum case .custom(CustomModifier) where CustomModifier is some protocol which describes how to perform the transition?

[kylebshr]

Yeah, very tough to decide between enum or struct. Does the current API allow custom modifiers? Or does that deserve a new/separate issue? Haven't had too much time to go in depth.

[lkzhao]

That is a good point. Actually, I previously have a branch that uses swift enum for HeroModifiers. But I dropped that because I wanted storyboard and plugin support.

One thing about what you are saying is incorrect. When applying transforms to views we might need to use multiple of the same modifiers and the orders are important.
For example: translate(50,50) rotate(1.57) behave differently than rotate(1.57) translate(50,50)
One rotates first, one translates first.
Something like translate(50,50) rotate(1.57) translate(50,50) is also supported currently.

I do feel that what you are proposing is far cleaner than the string API. It is more easy to discover and better to document them and probably faster too. But we need some more discussions about this. There are some immediate drawbacks too:

• Breaks storyboard support, IBInspectable does not support enum type at all.
• Plugins won't be able to introduce their own modifiers
• Same modifier would not be able to support multiple parameters

If we are going to implement it. I would prefer the 1st option. The second one, regards to what I have said about multiple modifiers, wouldn't work.

[kylebshr]

In that case, an array seems perfect - it'll retain order and allow multiple of the same. RE: storyboard support - could there be an IBInspectable property that allows you to use a string in IB? But isn't exposed programmatically? And when set, would convert to the array? Haven't played with IBInspectable much.

[alexzielenski]

@kylebshr I feel like that conversion would need to be done at runtime

[kylebshr]

Yes, that's what I meant :)

[lkzhao]

The current behaviour of HeroModifiers will recognize the order of modifiers that needs them (all the transform modifiers). For other modifiers like fade, arc... where only one is needed, the last one will override the options of the previous ones.

e.g cascade(0.02), cascade(0.05) will be equivalent to just cascade(0.05)

We can still do this with an array of enums. So I think it is the better way to go.

For storyboard support, It could be done at runtime using a setter.
But we will have two different attribute, one is of string type, the other one is a [Modifier] type.
If we change the array, do we expect the string attribute to return the modified version? And how do we name those attributes? one heroModifiersString? the other one heroModifiers? It might get ugly.

[kylebshr]

I agree it could get ugly... ideally there would be a storyboard only string property. And perhaps it could be set only?

[alexzielenski]

That would be ideal but in swift you can't do something along the lines of public private(get) heroModifierString: String. Besides, I'm guessing the storyboard needs access to a getter too. The getter for the string should probably be best computed by converting the underlying array when asked and its usage should be discouraged.

Also, it should be noted that retaining some semblance of the string API will allow backwards compatibility for whomever is currently using the library.

[lkzhao]

I have also tried this fun thing with swift. I am not saying that we should do this.

extension String: ExpressibleByArrayLiteral{
    typealias Element = Modifier
    init(arrayLiteral: Element...) {
        // returns a string from [Modifers]
    }
}

That makes this work! which is what we want. heroModifers is still a string type

view.heroModifers = [.fade, .scale(0.5)]

[lkzhao]

But I couldn't find a way to make this extension private with swift 3. So that is a huge pollution to the String type.

[alexzielenski]

@lkzhao You may be able to make an intermediate type thats both ExpressibleByArrayLiteral and ExpressibleByStringLiteral. I'm unsure of if IBInspectable works with ExpressibleByStringLiteral vs plain string types, though.

[lkzhao]

@alexzielenski You bet. But storyboard's IBInspectable doesn't support ExpressibleByStringLiteral. They won't show up if it is not a String type. sad! 😭😭

[lkzhao]

May be we can use conditionally compile preprocessor macro.

#if TARGET_INTERFACE_BUILDER
    public var heroModifierString:String
#else
    internal var heroModifierString:String{
       set{
          // change heroModifiers
       }
    }
    public var heroModifiers:[Modifier]
#endif

[kylebshr]

I think we're kind of discovering why a lot of UIKit APIs aren't usable in storyboards.. many issues with a well designed programmatic API compatibility.

On Jan 3, 2017, at 9:03 PM, Luke Zhao [e-mail] wrote:

May be we can use conditionally compile preprocessor macro.

if TARGET_INTERFACE_BUILDER

public var heroModiferString:String

else

internal var heroModiferString:String{
   set{
      // change heroModifers
   }
}
public var heroModifers:[Modifer]

endif

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

[lkzhao]

@kylebshr The conditionally compile macro may just work. If you are interested, can you take a look a it?

Another option that maybe better is something like this.

struct Modifier{
  var duration:TimeInterval?
  var delay:TimeInterval?
  var timingFunction:CAMediaTimingFunction?
  required init(fromString:String){}
}

struct Fade:Modifier{}
struct Scale:Modifier{

}

This have some benefits over the enum one:

• support custom modifier
• individual delay, duration, timingFunction is supported

What do you guys think?

[kylebshr]

I assume you mean a protocol or class, since you can't inherit from a struct? If a protocol works, that's a probably pretty good idea, but I wouldn't want to use subclassing. It doesn't really help with the storyboard issue either as far as I can tell.

I'll take a look at the macro!

On Jan 3, 2017, 10:20 PM -0500, Luke Zhao [e-mail], wrote:

@kylebshr The conditionally compile macro may just work. If you are interested, can you take a look a it?
Another option that maybe better is something like this.

struct Modifier{
var duration:TimeInterval?
var delay:TimeInterval?
var timingFunction:CAMediaTimingFunction?
required init(fromString:String){}
}

struct Fade:Modifier{}
struct Scale:Modifier{

}
This have some benefits over the enum one:

• support custom modifier
• individual delay, duration, timingFunction is supported

What do you guys think?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

[kylebshr]

Also — this framework seems to be picking up a lot of steam! Could we get some labels for issues and PRs? Maybe "discussion" "WIP" etc.? Thanks! 😀

[kylebshr]

Looks like the macro is a no-go — it compiles the var away when built for device/sim. The TARGET_INTERFACE_BUILDER macro is literally only for when it's compiled for IB within Xcode.

[lkzhao]

Yea, but we can have the same var in the else clause right? We just want to hide it from being accessed in code. i.e. change it to internal

#else
    internal var heroModifierString:String{
       set{
          // change heroModifiers
       }
    }
    public var heroModifiers:[Modifier]
#endif

[kylebshr]

But IB properties are set at run time, and it can't set it because it has become internal at that point — you get this:

2017-01-03 22:37:21.789 Locator[34050:1110487] Failed to set (heroModifierString) user defined inspected property on (UILabel): [<UILabel 0x7fb1f6d03b20> setValue:forUndefinedKey:]: this class is not key value coding-compliant for the key heroModifierString.

[lkzhao]

Really? I thought setValue:forKey: works even when the variable is internal in swift.

[kylebshr]

I can double check with a more real test, but I don't think that'll work across a framework. Sure, it can set an internal var within the same framework/project, but then we can't use cocoapods.

[lkzhao]

@kylebshr LOL. I got this working even through cocoapods:

#if TARGET_INTERFACE_BUILDER
  @IBInspectable public var heroModifiersString: String? {
    get { return nil }
    set {}
  }
#else
  internal var heroModifiersString: String? {
    get { fatalError() }
    set {
      heroModifiers = newValue
    }
  }
#endif

I uploaded it on the swift-internal-test branch.
You can install via:

pod "Hero", :git => 'https://github.com/lkzhao/Hero.git', :branch=>"swift-internal-test"

And you will see that storyboard can access the variable even though it is marked internal.

[kylebshr]

That's very interesting... can't help but wonder if it's a bug. I guess now we just have to decide the rules around its use? What happens if a string is set and programmatic options are set? I'm thinking raise an exception, but we could just pick a priority.

On Jan 3, 2017, at 11:21 PM, Luke Zhao [e-mail] wrote:

@kylebshr LOL. I got this working even through cocoapods:

if TARGET_INTERFACE_BUILDER

@IBInspectable public var heroModifiersString: String? {
get { return nil }
set {}
}

else

internal var heroModifiersString: String? {
get { fatalError() }
set {
heroModifiers = newValue
}
}

endif

I uploaded it on the swift-internal-test branch.
You can use to install it.

pod "Hero", :git => 'https://github.com/lkzhao/Hero.git', :branch=>"swift-internal-test"
And you will see that storyboard can access the variable even though it is marked internal.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

[lkzhao]

The string one should just modify the programmatic options when set (only springboard can do this). Since developers cannot access the string one (because it is marked as internal), it is not a problem. The programmatic options should always be the source of truth.

[krzysztofzablocki]

Maybe you could adapt the pattern I've used for Traits, e.g. leverage hero identifier in both code and IB, and then have a central repository that contains modifier configurations for it?

Also take a look at a script there that generates typed interface for Storyboard identifiers, so once someone sets them in Storyboard, it would generate enumeration containing all identifiers, that way you can avoid strings in code, that could easily be adapted to Hero.

[mRs-]

Enums would be the best option here imho

[evermeer]

For an easy enum solution you can use something like:

public enum Modifier: EnumString {
    case fade,
    translate(Int, Int),
    rotate(Int, Int, Int),
    scale(Float),
    zPosition(Int),
    zPositionIfMatched(Int)
}

protocol EnumString { }

extension EnumString {
    func toString() -> String {
        let mirror = Mirror(reflecting: self)
        if let associated = mirror.children.first {
            let valuesMirror = Mirror(reflecting: associated.value)
            if valuesMirror.children.count > 0 {
                let parameters = valuesMirror.children.map { "\($0.value)" }.joined(separator: ",")
                return "\(associated.label ?? "")(\(parameters))"
            }
            return "\(associated.label ?? "")(\(associated.value))"
        }
        return "\(self)"
    }
}

extension Array where Element: EnumString {
    func toModifierString() -> String {
        return self.map { $0.toString() }.joined(separator: ", ")
    }
}

extension String: ExpressibleByArrayLiteral {
    public init(arrayLiteral: Modifier...) {
        self = arrayLiteral.toModifierString()
    }
}

let x: String = [.fade, .translate(1, 2), .scale(1.45)]

here the x field will contain:
fade, translate(1,2), scale(1.45)

The only thing you have to do is extend the enum.

But then again... Writing out a switch case for building up the string would perform better than doing a Mirror

[lkzhao]

@evermeer The 'Mirror' class is pretty cool. Never knew that they existed.

One thing that concerns me with this implementation is the pollution to the default String type. Is your solution able to extended String again to support another array literal type?

I still feel like we should convert it to a pure swift enum API. With the ability to set it in the storyboard with a string. That will give us the excellent syntax when matching these internally.

switch incomingModifier {
case .scale(let x, let y, let z):
//...
}

[kylebshr]

I agree Luke, there's not much reason to go from enum -> String, just the other way around. I'll take a stab at implementing today, using the macro trick.

Sent from my iPhone

On Jan 4, 2017, at 10:47 AM, Luke Zhao [e-mail] wrote:

@evermeer The 'Mirror' class is pretty cool. Never knew that they existed.

One thing that concerns me with this implementation is the pollution to the default String type. Is your solution able to extended String again to support another array literal type?

I still feel like we should convert it to a pure swift enum API. With the ability to set it in the storyboard with a string. That will give us the excellent syntax when matching these internally.
'''swift
switch incomingModifier {
case .scale(let x, let y, let z):
//...
}
'''

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

[evermeer]

Well, you could also leave out the String extension and just use the .toModifierString() on the array. But I agree that It would be better to have an enum api.

Using 'Mirror' is kind of my thing. It's about the only reflection option available in Swift. It's the base for my https://github.com/evermeer/EVReflection library.

[cnstoll]

Sorry for joining the conversation late. I started tinkering with ideas to add a composable API yesterday, before I realized this conversation was taking place. By the time I had a PR ready it looks like the conversation already made a lot of progress.

I posted a PR here for a modifier string composition API using swift method chaining. I didn't mean to suggest that the PR should be taken as-is without discussion. Rather, it was just easier to convey my thoughts in code, so I decided to write it and put it up for review.

https://github.com/lkzhao/Hero/pull/15

I tried a few approaches before settling on the method chaining interface. The inspiration for method chaining actually came from the Hero usage docs. The examples for modifiers take the form of "fade() position(0,100) arc(1)" etc., and I thought a method chaining seemed most similar to the documentation.

I looked at String extensions too and had similar concerns around API pollution.

I looked at Swift enums too. Enum cases with associated values seem to make a ton of sense to encapsulate a modifier. But the trouble I ran into here was constructing a simple and intuitive API to actually create and add these modifiers to build the animation. The array example above seems like the closest, but ultimately still leaves something to be desired to me.

I settled on method chaining because of the readability, simplicity, and excellent discoverability aspect. Autocomplete on the HeroComposition makes it dead simple to see what options are available to add additional attributes to the modifier composition. Using optional method parameters, you can also construct very complex modifiers that still have simple implementations and public interfaces.

Here's an example of what a modifier string composition looks like with method chaining:

HeroComposition().fade().translate(0,150).rotate(-1,0,0).modifier

The HeroComposition with method chaining builds the following modifier string:

"fade translate(0,150) rotate(-1,0.0)"

It also supports modifiers like cascade using swift enums for those cases:

HeroComposition().fade().translate(0,150).rotate(-1,0,0).cascade(1,.bottomToTop).modifier

The HeroComposition with method chaining builds the following modifier string:

"fade translate(0,150) rotate(-1,0.0) cascade(1,"bottomToTop",0,false)"

(Note the optional parameters 0 and false added to the cascade modifier)

I'm going to go ahead and close the PR to let the conversation continue further without any pressure. If ya'll like this idea feel free to incorporate it in the evolution of the API, or let me know and I'll be happy to modify based on feedback.

The other benefit to this approach is it keeps the flexible String API intact, and builds around it as an extension. Both have benefits, and this lets them be used interchangeably.

[mRs-]

IMHO is a String API very hard to discover and error prone. Just a typo can you cost a day of work and you don't figure it out ;)

Otherwise here the current enum approach from myself, maybe this is the right direction?

//: Playground - noun: a place where people can play

import UIKit

enum Modifier {
    case fade
    case position(x: CGFloat, y: CGFloat)
    case size(width: CGFloat, height: CGFloat)
}

class Composition {

    var modifiers = [Modifier]()

    subscript(_ modifiers: Modifier...) -> Composition {
        self.modifiers = modifiers
        return self
    }
}


Composition()[.fade, .position(x: 12, y: 4), .size(width: 200, height: 300)]

or something like this:

//: Playground - noun: a place where people can play

import UIKit

enum Modifier {
    case fade
    case position(x: CGFloat, y: CGFloat)
    case size(width: CGFloat, height: CGFloat)
}

struct Composition {

    fileprivate var modifiers = [Modifier]()
}

extension Composition: ExpressibleByArrayLiteral {

    public init(arrayLiteral elements: Modifier...) {
        self.init()

        for element in elements {
            self.modifiers.append(element)
        }
    }
}

let composition: Composition = [.fade, .position(x: 100, y: 200), .size(width: 200, height: 200)]

or number 3

//: Playground - noun: a place where people can play

import UIKit

enum Modifier {
    case fade
    case position(x: CGFloat, y: CGFloat)
    case size(width: CGFloat, height: CGFloat)
}

extension Array {

    init(arrayLiteral elements: Modifier...) {
        self.init()

        for element in elements {
            self.append(element as! Element)
        }
    }
}

let composition = [Modifier.fade, Modifier.position(x: 100, y: 200), Modifier.size(width: 200, height: 1337)]

// or

let composition2: [Modifier] = [.fade, .position(x: 100, y: 200), .size(width: 200, height: 1337)]

[kylebshr]

This has been a great discussion, but other stuff has come up and I likely won't be able to help implement this any time soon. Feel free to carry on discussion and implementation without me!

[lkzhao]

I can take this on. Will have a PR up in a day or two. Lets discuss before I merge.  😀

[mRs-]

Sounds awesome. Let's form a rock solid solution here

[lkzhao]

Finished my initial implementation. Lets discuss over at #20.

[lkzhao]

Hey guys, checkout
https://github.com/lkzhao/Hero/blob/master/Examples/HeroExamples/Examples/LabelTransform/LabelTransformViewController.swift
for the new usage cases with custom HeroModifier and HeroPlugin.
I think it looks pretty damn good.

[BilalReffas]

Is this already done or should I create a pull request @lkzhao ? :)

[augusteo]

looks like its already done

[lkzhao]

Yep this is done. In master already. Thank you all!