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

Refactor songtag LRC handling #384

Merged
merged 30 commits into from
Nov 4, 2024
Merged

Refactor songtag LRC handling #384

merged 30 commits into from
Nov 4, 2024

Conversation

hasezoey
Copy link
Contributor

@hasezoey hasezoey commented Nov 1, 2024

This PR refactors many parts of the lib::songtag::lrc module and touches upon some related lyric things, in more details:

  • add tests for LRC parsing and writing
  • fix writing milliseconds to LRC format (previously the completely wrong value was used)
  • refactor a bunch of stuff to require less heap (string) allocations
  • refactor a bunch of stuff to do less (repeated) searching
  • general addition of doc-comments
  • cleanup unused code / variables / properties

as its only used once, all other places have it inlined already
…the full millis

also no modulation for millis
nested formatting without intermediate allocations!
…tions and checking

also reverse "sort_by" variable names
instead of removing, shifting, shifting again and adding.
Change so that "get_index" does not add 2seconds to the time, if it is still wanted, the caller should do it.
Also change input to be milliseconds
…ble" message on same value

instead of "on no lyric frames" and later use "parsed lyrics"
instead of getting seconds, then converting to milliseconds
@hasezoey
Copy link
Contributor Author

hasezoey commented Nov 2, 2024

I did some more changes:

  • add tests for merge_adjacent, adjust_offset and get_text
  • change Lyric::get_index to not use a artificial 2 second offset anymore
  • fix Lyric::adjust_offset inverting the given offset when below 10 seconds (ie Lyric::adjust_offset(_, 1000) would give self.offset = -1000 instead of self.offset = 1000), this didnt seem intentional, but if it is we should add a comment
  • change from casting some values to using try_from
  • use milliseconds directly instead of as_secs
  • some style changes
  • less cloning again
  • change tui no lyrics available message to be the else case instead of basing it on another value
  • add a note why Lyric::get_text uses a artificial 2 second offset

@tramhao tramhao merged commit beb2517 into tramhao:master Nov 4, 2024
5 checks passed
@hasezoey hasezoey deleted the lyrics branch November 4, 2024 10:15
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