Using Tactile with UIControl works inconsistently
pteasima opened this issue · 6 comments
I like the concept very much, unfortunately Tactile isnt reliable. When using it as so:
let button = UIButton()
button.on(.TouchUpInside, functionToCall)
it only works about 95% of the time for me and my colleagues on different projects.
I had a quick look and would like to point you to this piece of code:
116 private let proxies = NSMapTable.weakToStrongObjectsMapTable()
117
118 private class Proxy: NSObject {
119 var actor: Triggerable!
120
121 init(actor: Triggerable, control: UIControl, event: UIControlEvents) {
122 super.init()
123
124 self.actor = actor
125
126 proxies.setObject(self, forKey: "\(control, event.rawValue)")
127 }
128
129 @objc func recognized(control: UIControl) {
130 actor.trigger(control)
131 }
132 }
You are using a weakToStrongObjectsMapTable(), which has the property that: "Use of weak-to-strong map tables is not recommended. The strong values for weak keys which get zeroed out continue to be maintained until the map table resizes itself."
I think that the key "\(control, event.rawValue)"
goes out of scope immediately, then, whenever the mapTable is resized, the values are released and stop working.
Also suspicious to me is the cycle Actor<--owns-->Proxy.
Can you please take a look at this and explain? Thanks
Hi @pteasima,
Thank you very much for the feedback.
I'm using NSMapTable.weakToStrongObjectsMapTable
in order to zero out the proxies when the UIView
/UIControl
is deallocated. It has the limitation of maintaining the objects until the table resizes itself but it is better than nothing. Another approach would be to swizzle the deinit
method and manually remove the corresponding proxy. It is something I consider doing and would like to hear your thoughts about it.
Your are absolutely right about the problem of the UIControl
proxies. It's a rude mistake on my part and I don't know what I was thinking 😳 Thanks for pointing it out!
I pushed a new branch https://github.com/delba/Tactile/tree/map-table that fixes it. I tested it thoughtfully and it seems to work. I would love to have your feedback before merging it in master.
You can install it like this:
// CocoaPods
pod "Tactile", :git => "https://github.com/delba/Tactile.git", :branch => "map-table"
// Carthage
github "delba/Tactile" "map-table"
Even though it isn't mentioned in the documentation, I observed that NSMapTable.strongToWeakObjectsMapTable()
has the same limitations as NSMapTable.weakToStrongObjectsMapTable()
and some objects will be maintained until the table resizes itself.
Thanks again 😄
Hi, sorry for taking so long. The change you made seems to make sense, however I dont have time to properly test it. Still, it feels like using a global hash table is not the ideal solution from a design perspective (storing object specific data in a global collection). Is there a reason why you chose not to use associated objects? I liked you solution for its simple swift syntax, but other than that its nothing new, there have been similar solutions using associated objects since objc.
No problem @pteasima !
I merged the map-table
branch into master. Using associated objects is definitely part of a biggest update and I want to take a stab a it soon. I'll ping you when it's done (maybe as soon as this week). In the meanwhile, could you point me to similar objc libs please?
Thanks for the feedback!
This is the one I had in mind, see addEventHandler: forControlEvents: method
https://github.com/kevinoneill/Useful-Bits/blob/master/UsefulBits/UIKit/UIControl%2BBlocks.m
It doesnt allow unregistering the eventHandler (your off method), I dont think that should be a problem to implement but I dont wanna think about it right now.
Thanks, I'll definitely take a look a it!