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.

Sign up for GitHub

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

Jump to bottom

Fix KVO that breaks if a target object overrides 'isEqual' and 'hash' #361

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Fix KVO that breaks if a target object overrides 'isEqual' and 'hash' #361

wants to merge 12 commits into from

Conversation

tutuming
Copy link

@tutuming tutuming commented Apr 1, 2014

NSDictionary's key is compared by key object's hash.
In BW::KVO targets are held as dictionary's key.
So if target objects' class override 'hash' (and 'isEqual'), KVO doesn't work collectly.

@tutuming tutuming changed the title Fix KVO that breaks if a target object overrides 'isEuql' and 'hash' Fix KVO that breaks if a target object overrides 'isEqual' and 'hash' Apr 1, 2014
@clayallsopp
Copy link
Contributor

This makes sense and I believe it shouldn't cause any trouble (tests fail for an unrelated reason, #359).

But I would like to get someone else to look at it - @colinta or @markrickert? I believe it will remove a strong reference to the target, which may have consequences in some apps?

@colinta
Copy link
Contributor

colinta commented Apr 3, 2014

The strong reference to target does make me pause, but it's probably not good to have that strong reference anyway; does that array get reset at some point, or is that a possible memory leak?

I think it should be merged, with a good explanatory email to the group about the change.

@colinta
Copy link
Contributor

colinta commented Apr 3, 2014

So yeah it looks like @targets never gets reset/reassigned, so whatever gets put in there stays in there. We should do some cleanup in unobserve/unobserve_all.

@tutuming
Copy link
Author

tutuming commented Apr 4, 2014

@clayallsopp @colinta

Thanks for your reviews!
I added cleanup code for @targets (and a little refactoring), is that right?

@clayallsopp
Copy link
Contributor

@tutuming can you merge with the master branch again and ensure that the tests pass on travis? thanks!

@clayallsopp @tutuming
Extract BW::HTTP to the bubble-wrap-http gem. fixes rubymotion-commun…
d7db918 
…ity#308
@clayallsopp @tutuming
Switch to the proper travis format
b7a641b
@clayallsopp @tutuming
UI now depends on constants.rb
dfda4dc
@clayallsopp @tutuming
update warning messages about bw http
203c599
@clayallsopp @tutuming
1.6.0.rc1
7ad7c1a 
- 'bubble-wrap/http' is now deprecated and will be removed in 2.0, see rubymotion-community#308

+ `BW::UIActivityViewController` wrapper added, see rubymotion-community#335

+ `BW::NetworkIndicator` added, see rubymotion-community#349

 + `BW::Location.get_compass` & `BW::Location.get_compass_once`see rubymotion-community#348

+ `NSString#to_color` ARGB support, see rubymotion-community#350

+ `Object#method` support for `BW::Reactor`, see rubymotion-community#359

* Prevented a possible exception when stopping `BW::Location`, see rubymotion-community#358

* Fixed a bug when requiring just 'bubble-wrap/ui'
@clayallsopp @tutuming
Switch to SVG status
8e356e6
@clayallsopp @tutuming
rc1
83aeedb
@clayallsopp @tutuming
update travis instructions
b0499d1
This was referenced Jul 21, 2014
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tutuming @clayallsopp @colinta @MarkVillacampa