Skip to content
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

Using Tactile with UIControl works inconsistently #1

Closed
pteasima opened this issue Nov 29, 2015 · 6 comments
Closed

Using Tactile with UIControl works inconsistently #1

pteasima opened this issue Nov 29, 2015 · 6 comments

Comments

@pteasima
Copy link

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

@delba
Copy link
Owner

delba commented Dec 3, 2015

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 😄

@pteasima
Copy link
Author

pteasima commented Dec 7, 2015

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.

@delba
Copy link
Owner

delba commented Dec 7, 2015

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!

@delba delba closed this as completed Dec 7, 2015
@pteasima
Copy link
Author

pteasima commented Dec 7, 2015

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.

@delba
Copy link
Owner

delba commented Dec 7, 2015

Thanks, I'll definitely take a look a it!

@delba
Copy link
Owner

delba commented Dec 14, 2015

@pteasima aeb8864 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants