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

Adding basic i18n support #15

Merged
merged 1 commit into from
Apr 17, 2014
Merged

Adding basic i18n support #15

merged 1 commit into from
Apr 17, 2014

Conversation

hectr
Copy link
Contributor

@hectr hectr commented Dec 22, 2013

No description provided.

@billymeltdown
Copy link
Owner

This is excellent, @hectr, thanks very much! Sorry for the delay, but I've merged your pull request into the develop branch as of SHA: d723b62 Want to give it a shot? NSDate+Helper has been updated with major performance improvements.

@billymeltdown billymeltdown merged commit 5f5420e into billymeltdown:master Apr 17, 2014
@hectr
Copy link
Contributor Author

hectr commented May 10, 2014

Hi @billymeltdown, today I've been rebasing #14. I believe the string method is mostly useless if its result depends on the user locale. But anyway, I don't think #14 can be merged right now, because it's not using sharedDateFormatter.
I can't actually understand how the _displayFormatter's date format could be changed after its initialisation without running into concurrency issues in multi-threaded environments.
Could you please share your thoughts about that? Does it mean that NSDate+Helper is not thread-safe anymore?

@hectr hectr deleted the i18n branch May 14, 2014 07:23
@billymeltdown
Copy link
Owner

To be honest I haven't thought real hard about thread concurrency with these helpers methods, although I'm not running into any issues. If you'd like to point out what's problematic with the current setup and suggest some fixes that would be great!

I believe the string method is mostly useless if its result depends on the user locale.

Could you expand on that some more?

All in all, this is a krufty category. I recently came across Erica Sadun's NSDate-Utilities category and it looks quite cleanly implemented, using a shared calendar but not a shared formatter:

https://github.com/erica/NSDate-Extensions/blob/master/NSDate%2BUtilities.m

@billymeltdown
Copy link
Owner

Did a little reading and you are right, NSDateFormatter has not been thread-safe. However, it is in the very latest APIs (iOS 7 and OS X 10.9 Mavericks):

https://developer.apple.com/library/mac/documentation/Cocoa/Reference/Foundation/Classes/NSDateFormatter_Class/Reference/Reference.html

So we'll want to fix this to be thread-safe for older deployment targets, as not everyone is targeting only the latest and greatest (myself included). If you have the time and you want to take a crack at some of it, it'd be a big help! I'll try to bang this out in the near future.

@hectr
Copy link
Contributor Author

hectr commented May 15, 2014

I would gladly contribute to fix it, but I'm afraid I don't fully understand yet the implications of mutating a shared date formatter.

I would probably suggest to use a different shared formatter for each kNSDateHelperFormat*, but perhaps that could ruin the benefits of having a single formatter in most scenarios. Implementing some sort of synchronization mechanism seems an even worse solution for me.

I also thought about having a pool of formatters, but this solution seemed a bit over engineered.

I am sure there must be a smarter way for dealing with the thread safety issue, but it's simply beyond my knowledge.

@billymeltdown
Copy link
Owner

Well I think the first thing to do is get rid of the shared formatter since that's a no-go before 10.9 and iOS 7, and replace each with individual NSDateFormatter objects to start with. From there we can consider what needs to be optimized, what can be shared because it's never intended to be mutated (e.g. if the user is asking for a format, we need a new formatter, otherwise maybe not).

I remember when this was first suggested, sharing the formatter, it was because it was getting too expensive to use the category due to all the formatter objects created. I suspect that maybe we just shouldn't be doing this and the application code should be handling, but maybe we can thread the needle.

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.

2 participants