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

[TC-1101] Added Ti.App.Properties 'change' event for iOS #2635

Merged
merged 2 commits into from Jul 31, 2012

Conversation

farfromrefug
Copy link
Contributor

I added a "change" event to Ti.App.Properties to listen for changes of Properties

@negupta
Copy link
Contributor

negupta commented Jul 26, 2012

Signed CLA is in place.

@negupta
Copy link
Contributor

negupta commented Jul 26, 2012

@farfromrefug - We need a JIRA ticket and a test case to process this PR.

@farfromrefug
Copy link
Contributor Author

Will do thanks

@farfromrefug
Copy link
Contributor Author

JIRA ticket + test case
https://jira.appcelerator.org/browse/TC-1101

@WhichKatieDid
Copy link
Contributor

Forwarded questions internally about if there'd be any issue with Android or Mobile Web implementing this, too. CR edge case: It's highly unlikely, but could you add:

TiThreadPerformOnMainThread(^{
    [[NSNotificationCenter defaultCenter] removeObserver:self];
}, YES);

in the beginning of TiAppPropertiesProxy's dealloc? There is a possibility that the proxy is removed before a listener is.

Similarly, you will need to have

TiThreadPerformOnMainThread(^{<YOUR CODE>}, YES);

around any access to nsnotificationcenter, since it's not threadsafe and the _listenerAdded/removed happens in a background thread.

@farfromrefug
Copy link
Contributor Author

yes you are right!

@farfromrefug
Copy link
Contributor Author

About android implementation i didnt see any way to change properties outside application so i didnt see the need for such implementation

@WhichKatieDid
Copy link
Contributor

CR approved, generated TIMOB-10260 and TIMOB-10262 for parity, in case you want to add to the pull request on other OSes.

@WhichKatieDid
Copy link
Contributor

FR passed. Merging pull.

WhichKatieDid added a commit that referenced this pull request Jul 31, 2012
[TC-1101] Added Ti.App.Properties 'change' event for iOS
@WhichKatieDid WhichKatieDid merged commit e773a40 into tidev:master Jul 31, 2012
@farfromrefug
Copy link
Contributor Author

I will look at it. By the way shouldnt we add documentation for this ? (that would be my first so a good thing ;))

@farfromrefug farfromrefug deleted the properties_change_event branch December 27, 2012 16:54
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

Successfully merging this pull request may close these issues.

None yet

3 participants