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

Add singular form for verbose time description #10

Closed
wants to merge 2 commits into from

Conversation

mkchoi212
Copy link
Contributor

@mkchoi212 mkchoi212 commented May 14, 2018

When using VLCTime.verboseDescription,

1 minutes 20 seconds is now 1 minute 20 seconds

@fkuehne fkuehne self-requested a review May 14, 2018 09:30
Copy link
Contributor

@fkuehne fkuehne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Patch itself looks good to me!

What I’m wondering about - didn’t newer versions of macOS / iOS introduce convenience code to print such user visible strings in all locales? Can we use that?

@mkchoi212
Copy link
Contributor Author

@fkuehne Are you referring to this?

It's cool but I don't see any Localizable.strings files in the project. Am I missing something?

@carolanitz
Copy link
Member

awesome!!! This is exactly the reason why we need the Testsuite. To find all those tiny bugs and fix them along the way and make sure that this doesn't break in the future 🙌

@carolanitz carolanitz self-requested a review May 15, 2018 13:50
@carolanitz
Copy link
Member

@fkuehne I think you might be talking about using the Date components Api. We should definitely switch to that here and not try to reimplement the logic.
I'm not sure if there are differences in time formats by country but if, we don't handle that.
Also we would be wrong for 1 hour and 1 second.
The code should get more consize by using date components but make sure to only instantiate the object once for the entire class since it's very expensive to create.

https://developer.apple.com/documentation/foundation/datecomponents

@fkuehne
Copy link
Contributor

fkuehne commented May 15, 2018 via email

@mkchoi212
Copy link
Contributor Author

One question! When you have 1:00:30, do we want this to be "1 hour 30 seconds" or just "1 hour"?

@fkuehne
Copy link
Contributor

fkuehne commented May 15, 2018 via email

@mkchoi212
Copy link
Contributor Author

I also noticed "1 hour 20 minutes 30 seconds" is represented as "1 hour 20 minutes". Is this desired behavior?

@carolanitz
Copy link
Member

carolanitz commented May 16, 2018

Why don't you take a look at what the nsdateformatter will give you by default? The rest just comes down to personal preference ¯_(ツ)_/¯ and like felix says if someone wants something else they can easily adapt it

@mkchoi212
Copy link
Contributor Author

Ok cool. I'll go with the default format. I was just wondering cause I thought there was some particular reason behind things :D

- (void)initDateComponents
{
if (!components)
components = [[NSDateComponents alloc] init];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

turns out I've been mixing up the expensiveness of NSDateFormatter with NSDateComponents.
You can use this one actually directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Member

@carolanitz carolanitz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the rest looks good. I would add a couple of examples in the description and update the News to note that we adjusted this method . As reference for the description I would point to - (NSString *)stringFromDate:(NSDate *)fromDate toDate:(NSDate *)toDate; and I think this can be merged then

@mkchoi212 mkchoi212 force-pushed the time-description branch 3 times, most recently from b939944 to 2a7651e Compare May 23, 2018 21:04
@carolanitz
Copy link
Member

I just tested it and it works great but the localization for remaining doesn't work. I will merge it anyway because it's improvement.
For german you get 1 Stunde 33 Minuten und 41 Sekunden remaining that should be fixed at some point

@carolanitz
Copy link
Member

this was merged

@carolanitz carolanitz closed this May 24, 2018
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.

3 participants