Fix white space handling while parsing LRC with word time tags #85
Conversation
Maxr1998
force-pushed
the
fix-lrc-parser
branch
2 times, most recently
from
c223e2c to
3ebeb81
Compare
andy840119
left a comment
andy840119
left a comment
There was a problem hiding this comment.
Only review the first and 3rd commit.
Resolve the review feedback than I'll continuous review the rest of one. Need to make sure that those breaking change is correct.
andy840119
left a comment
andy840119
left a comment
There was a problem hiding this comment.
Seems there are several changes within a PR:
- Adjust test case.
- Doing refactor and add more tests.
- (breaking change) Deal with spacing within the lyric.
- (breaking change) Adjust the index.
- (breaking change but cancelled) not decode the lyric if not start with timing info.
As lrc has not strict rule definition, it's hard for me to decide lots of change within a PR.
Should be better to separate those changes with different PRs.
Maxr1998
changed the title
Maxr1998
force-pushed
the
fix-lrc-parser
branch
2 times, most recently
from
7a976d8 to
c32a970
Compare
andy840119
added
the
Breaking change
Maxr1998
force-pushed
the
fix-lrc-parser
branch
3 times, most recently
from
94a1ac3 to
9e88207
Compare
andy840119
left a comment
andy840119
left a comment
There was a problem hiding this comment.
Overall LGTM.
I haven't deeply review LrcParser/Parser/Lrc/Utils/LrcTimedTextUtils.cs but can give it a quick pass because the test case is much enough.
| [TestCase("<00:51.00><01:29.99><01:48.29><02:31.00><02:41.99>You gotta fight !", "You gotta fight !", new[] { "[0,start]:51000" })] // decode with invalid format. | ||
| public void TestDecodeWithInvalidFormat(string text, string expectedText, string[] expectedTimeTags) | ||
| { | ||
| var (actualText, actualTimeTags) = LrcTimedTextUtils.TimedTextToObject(text); | ||
| var (actualText, actualTimeTags) = LrcTimedTextUtils.TimedTextToObject(text, lineStartTime); |
There was a problem hiding this comment.
Have no idea why you remove this test case.
There was a problem hiding this comment.
That's the one I moved above, since the parsing itself passes fine. Thus, I don't consider it invalid.
There was a problem hiding this comment.
I see.
The old question:
does the lrc define those kinds of case?
There was a problem hiding this comment.
Not really 🥲
However, the approach I chose seems logical to me:
<00:51.00><01:29.99><01:48.29><02:31.00><02:41.99>You gotta fight !
There are four segments that have a time defined, but no content. Thus, they don't need to be extracted and will be skipped. The last segment isn't empty, and will have a start time of 02:41.99.
There was a problem hiding this comment.
hmmm...
I think should be OK now until someone have complain after.
- Correctly handle white space between word time tags - Support parsing time tags that surround each non-blank segment
2 participants
Fixes #83.