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

resolve DLNAService parsing of date with fraction of seconds #26

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

mikyl
Copy link

@mikyl mikyl commented May 28, 2015

No description provided.

@eunikolsky
Copy link
Contributor

@mikyl, thanks for the PR.

  1. Where did you see the problem this PR solves? It would be great to have unit tests for the change to make sure we don't break anything. Please add them if you can; or we can do that.
  2. Why did you change parsing time from using the specialized NSDateFormatter class to manual parsing? I'd say it's better to use two formatters – with and w/o the milliseconds, and one of them should fail.
  3. The PR contains three unrelated commits: Release 1.4.2 38a3f16; Release 1.4.3 5be6d35; Release 1.4.4 610dc29. Could you please create a branch off the current dev branch, rebase your commit there, and submit that as a PR?

@mikyl
Copy link
Author

mikyl commented May 29, 2015

hello @eunikolsky

-> 1. I saw this on a proprietary DMR implementation, and verified on the DMR UPnP specification. It tells that time format for RelativeTimePosition, AbsoluteTimePosition is the same as for CurrentTrackDuration and may return a date with millisecond if DMR implementation wants to. That's why, this PR is more an adaptation to be compliant with the UPnP specification.

H+:MM:SS[.F+] or H+:MM:SS[.F0/F1]
where :

  • H+ means one or more digits to indicate elapsed hours
  • MM means exactly 2 digits to indicate minutes (00 to 59)
  • SS means exactly 2 digits to indicate seconds (00 to 59)
  • [.F+] means optionally a dot followed by one or more digits to indicate fractions of seconds
  • [.F0/F1] means optionally a dot followed by a fraction, with F0 and F1 at least one digit long, and F0 < F1

The string may be preceded by an optional + or – sign, and the decimal point itself may be omitted if there are no fractional second digits. This variable does not apply to Tuners. If the service implementation doesn’t support track duration information then this state variable must be set to value “NOT_IMPLEMENTED”.

-> 2. Indeed, it would be better to go with NSDateFormatter but i assume that, according to the possible time formats, it would require 3 different NSDateFormatter instances.
Moreover, NSDateFormatter shouldn't be instantiate in the method but in class. As this method may be called frequently, it would definitely avoid lots of alloc/dealloc. I mean i thought this is always true with ios 8..
-> 3. Yep, i saw that, after pushing the PR, sorry for that. I will perform what you said when i find some free time.

@eunikolsky
Copy link
Contributor

@mikyl, thanks for the explanation.

  1. Why would anyone use the fraction format?!!
  2. I dare to say we could parse these formats using one regex, but using a specific parser is better. Yes, we'd need a few time parsers, and they should be cached. Ideally, timeForString: and stringForTime: functions should be extracted into a separate class, and injected into DLNAService (for easier testing and following SRP).
    And again, we need tests for parsing all these formats.
  3. Great, thanks.

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