diff --git a/src/lyric_auto_edit.cpp b/src/lyric_auto_edit.cpp index c588576..b909680 100644 --- a/src/lyric_auto_edit.cpp +++ b/src/lyric_auto_edit.cpp @@ -27,7 +27,7 @@ std::optional auto_edit::RunAutoEdit(AutoEditType type, const LyricDa std::optional auto_edit::ReplaceHtmlEscapedChars(const LyricData& lyrics) { - LyricDataUnstructured unstructured = parsers::lrc::serialise(lyrics); + std::string text = from_tstring(parsers::lrc::expand_text(lyrics, false)); std::pair replacements[] = { {"&", '&'}, @@ -41,12 +41,12 @@ std::optional auto_edit::ReplaceHtmlEscapedChars(const LyricData& lyr for(auto [escaped, replacement] : replacements) { size_t current_index = 0; - while(current_index < unstructured.text.length()) + while(current_index < text.length()) { - size_t next_index = unstructured.text.find(escaped, current_index); + size_t next_index = text.find(escaped, current_index); if(next_index == std::string::npos) break; - unstructured.text.replace(next_index, escaped.length(), 1, replacement); + text.replace(next_index, escaped.length(), 1, replacement); current_index = next_index + 1; replace_count++; } @@ -55,7 +55,7 @@ std::optional auto_edit::ReplaceHtmlEscapedChars(const LyricData& lyr if(replace_count > 0) { - return {parsers::lrc::parse(unstructured)}; + return {parsers::lrc::parse(lyrics, text)}; } else { @@ -253,32 +253,31 @@ std::optional auto_edit::FixMalformedTimestamps(const LyricData& lyri } }; - LyricDataUnstructured new_lyrics = parsers::lrc::serialise(lyrics); - + std::string text = from_tstring(parsers::lrc::expand_text(lyrics, false)); int change_count = 0; - size_t current_index = new_lyrics.text.find('['); + size_t current_index = text.find('['); while(current_index != std::string::npos) { bool changed = false; - size_t end_index = new_lyrics.text.find(']', current_index); + size_t end_index = text.find(']', current_index); if(end_index == std::string::npos) { break; } - std::string_view tag {new_lyrics.text.c_str() + current_index, end_index - current_index + 1}; + std::string_view tag {text.c_str() + current_index, end_index - current_index + 1}; changed |= fix_decimal_separator(tag); if(changed) { change_count++; } - current_index = new_lyrics.text.find('[', current_index+1); + current_index = text.find('[', current_index+1); } if(change_count > 0) { - return {parsers::lrc::parse(new_lyrics)}; + return {parsers::lrc::parse(lyrics, text)}; } else { diff --git a/src/lyric_io.cpp b/src/lyric_io.cpp index 86db7c4..27ffad4 100644 --- a/src/lyric_io.cpp +++ b/src/lyric_io.cpp @@ -75,42 +75,7 @@ bool io::save_lyrics(metadb_handle_ptr track, const metadb_v2_rec_t& track_info, return false; } - std::string text; - if(lyrics.IsTimestamped() && preferences::saving::merge_equivalent_lrc_lines()) - { - LyricData merged_lyrics = lyrics; - const auto lexicographic_sort = [](const auto& lhs, const auto& rhs){ return lhs.text < rhs.text; }; - std::stable_sort(merged_lyrics.lines.begin(), merged_lyrics.lines.end(), lexicographic_sort); - std::vector::iterator equal_begin = merged_lyrics.lines.begin(); - - while(equal_begin != merged_lyrics.lines.end()) - { - std::vector::iterator equal_end = equal_begin + 1; - while((equal_end != merged_lyrics.lines.end()) && (equal_begin->text == equal_end->text) && (equal_end->timestamp != DBL_MAX)) - { - equal_end++; - } - - // NOTE: We don't need to move equal_begin back one because we don't add - // the first timestamp to the string. That'll happen as part of the - // normal printing below. - for(auto iter=equal_end-1; iter!=equal_begin; iter--) - { - equal_begin->text = to_tstring(parsers::lrc::print_timestamp(iter->timestamp)) + equal_begin->text; - } - equal_begin = merged_lyrics.lines.erase(equal_begin+1, equal_end); - } - - const auto timestamp_sort = [](const auto& lhs, const auto& rhs){ return lhs.timestamp < rhs.timestamp; }; - std::stable_sort(merged_lyrics.lines.begin(), merged_lyrics.lines.end(), timestamp_sort); - - text = from_tstring(parsers::lrc::expand_text(merged_lyrics)); - } - else - { - text = from_tstring(parsers::lrc::expand_text(lyrics)); - } - + const std::string text = from_tstring(parsers::lrc::expand_text(lyrics, preferences::saving::merge_equivalent_lrc_lines())); try { std::string output_path = source->save(track, track_info, lyrics.IsTimestamped(), text, allow_overwrite, abort); diff --git a/src/main.cpp b/src/main.cpp index 7dde5b1..302b122 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -53,6 +53,7 @@ void OpenLyricsVersion::get_about_message(pfc::string_base & out) "- Fix album art backgrounds disappearing when playing the same track twice\n" "- Fix the 'encoding' tag in LRC files being considered a lyric line\n" "- Fix rounding errors causing timestamps to randomly change when saving\n" + "- Fix LRC same-line collapsing sometimes deleting parts of a line\n" "- Many, many minor non-functional internal code improvements\n" "\n"; out += "Version 1.9 (2024-06-12):\n" diff --git a/src/parsers.h b/src/parsers.h index bcfd88b..cb3ae15 100644 --- a/src/parsers.h +++ b/src/parsers.h @@ -18,9 +18,9 @@ namespace lrc bool try_parse_timestamp(std::string_view tag, double& out_timestamp); LyricData parse(const LyricDataUnstructured& input); - LyricDataUnstructured serialise(const LyricData& input); + LyricData parse(const LyricDataCommon& metadata, std::string text_utf8); - std::tstring expand_text(const LyricData& data); + std::tstring expand_text(const LyricData& data, bool merge_equivalent_lrc_lines); } // namespace lrc } // namespace parsers diff --git a/src/parsers/lrc.cpp b/src/parsers/lrc.cpp index d0fd7f1..4302004 100644 --- a/src/parsers/lrc.cpp +++ b/src/parsers/lrc.cpp @@ -196,6 +196,7 @@ struct LineTimeParseResult std::string print_timestamp(double timestamp) { + assert(timestamp != DBL_MAX); double total_seconds_flt = std::floor(timestamp); int total_seconds = static_cast(total_seconds_flt); int time_hours = total_seconds/3600; @@ -372,6 +373,11 @@ std::vector collapse_concurrent_lines(const std::vector out_lines; + out_lines.reserve(data.lines.size()); + for(const LyricDataLine& in_line : data.lines) + { + // NOTE: Ordinarily a single line is just a single line and contains no newlines. + // However if two lines in an lrc file have identical timestamps, then we merge them + // during parsing. In that case we need to split them out again here. + size_t start_index = 0; + while(start_index <= in_line.text.length()) // This is specifically less-or-equal so that empty lines do not get ignored and show up in the editor + { + size_t end_index = min(in_line.text.length(), in_line.text.find('\n', start_index)); + size_t length = end_index - start_index; + std::tstring out_text(&in_line.text.c_str()[start_index], length); + out_lines.push_back({ out_text, in_line.timestamp }); + + start_index = end_index+1; + } + } + + if(merge_equivalent_lrc_lines) + { + const auto enumerate = [](std::vector input) -> std::vector> + { + std::vector> output; + output.reserve(input.size()); + size_t index = 0; + for(LyricDataLine& value : input) + { + output.push_back({index, std::move(value)}); + index++; + } + return output; + }; + const auto denumerate = [](std::vector> input) -> std::vector + { + std::vector output; + output.reserve(input.size()); + for(auto& value : input) + { + output.push_back(std::move(value.second)); + } + return output; + }; + std::vector> indexed_lines = enumerate(std::move(out_lines)); + + const auto lexicographic_sort = [](const auto& lhs, const auto& rhs){ return lhs.second.text < rhs.second.text; }; + std::stable_sort(indexed_lines.begin(), indexed_lines.end(), lexicographic_sort); + decltype(indexed_lines)::iterator equal_begin = indexed_lines.begin(); + + while(equal_begin != indexed_lines.end()) + { + decltype(indexed_lines)::iterator equal_end = equal_begin + 1; + while((equal_end != indexed_lines.end()) && (equal_begin->second.text == equal_end->second.text) && (equal_end->second.timestamp != DBL_MAX)) + { + equal_end++; + } + + // NOTE: We don't need to move equal_begin back one because we don't add + // the first timestamp to the string. That'll happen as part of the + // normal printing below. + for(auto iter=equal_end-1; iter!=equal_begin; iter--) + { + equal_begin->second.text = to_tstring(parsers::lrc::print_timestamp(iter->second.timestamp)) + equal_begin->second.text; + } + equal_begin = indexed_lines.erase(equal_begin+1, equal_end); + } + + const auto index_sort = [](const auto& lhs, const auto& rhs){ return lhs.first < rhs.first; }; + std::sort(indexed_lines.begin(), indexed_lines.end(), index_sort); + out_lines = denumerate(indexed_lines); + } + + for(const LyricDataLine& line : out_lines) + { + // Even timestamped lyrics can still contain untimestamped lines + if(line.timestamp != DBL_MAX) + { + expanded_text += to_tstring(print_timestamp(line.timestamp)); + } + expanded_text += line.text; + expanded_text += _T("\r\n"); + } + } + else { - if(line.timestamp == DBL_MAX) + // Not timestamped + for(const LyricDataLine& line : data.lines) { + assert(line.timestamp == DBL_MAX); if(line.text.empty()) { // NOTE: In the lyric editor, we automatically select the next line after synchronising the current one. @@ -513,25 +600,6 @@ std::tstring expand_text(const LyricData& data) } expanded_text += _T("\r\n"); } - else - { - // NOTE: Ordinarily a single line is just a single line and contains no newlines. - // However if two lines in an lrc file have identical timestamps, then we merge them - // during parsing. In that case we need to split them out again here. - size_t start_index = 0; - while(start_index <= line.text.length()) // This is specifically less-or-equal so that empty lines do not get ignored and show up in the editor - { - size_t end_index = min(line.text.length(), line.text.find('\n', start_index)); - size_t length = end_index - start_index; - std::tstring_view view(&line.text.data()[start_index], length); - - expanded_text += to_tstring(print_timestamp(line.timestamp)); - expanded_text += view; - expanded_text += _T("\r\n"); - - start_index = end_index+1; - } - } } return expanded_text; @@ -585,4 +653,175 @@ MVTF_TEST(lrcparse_timestamp_parsing_and_printing_roundtrips) ASSERT(output == input); } +MVTF_TEST(lrcparse_parsing_merges_lines_with_matching_timestamps) +{ + const std::string input = "[02:29.75]linePart1\n[02:29.75]linePart2"; + const LyricData parsed = parsers::lrc::parse({}, input); + + ASSERT(parsed.lines.size() == 1); + ASSERT(parsed.lines[0].text == _T("linePart1\nlinePart2")); +} + +MVTF_TEST(lrcparse_parsing_duplicates_lines_with_multiple_timestamps) +{ + const std::string input = "[00:36.28][01:25.09]dupe-line"; + const LyricData parsed = parsers::lrc::parse({}, input); + + ASSERT(parsed.lines.size() == 2); + ASSERT(parsed.lines[0].text == _T("dupe-line")); + ASSERT(parsed.lines[0].timestamp == 36.28); + ASSERT(parsed.lines[1].text == _T("dupe-line")); + ASSERT(parsed.lines[1].timestamp == 85.09); +} + +MVTF_TEST(lrcparse_expanding_splits_lines_with_matching_timestamps) +{ + LyricData input = {}; + input.lines.push_back({_T("line1Part1\nline1Part2"), 149.75}); + input.lines.push_back({_T("line2Part1\nline2Part2\nline2Part3"), 153.09}); + + const std::tstring output = parsers::lrc::expand_text(input, false); + ASSERT(output == _T("[02:29.75]line1Part1\r\n[02:29.75]line1Part2\r\n[02:33.09]line2Part1\r\n[02:33.09]line2Part2\r\n[02:33.09]line2Part3\r\n")); +} + +MVTF_TEST(lrcparse_expanding_splits_lines_with_matching_timestamps_and_then_merges_matching_lines) +{ + // Checks for the timestamp-modifying part of https://github.com/jacquesh/foo_openlyrics/issues/354 + LyricData input = {}; + input.lines.push_back({_T("linePart1\nlinePart2"), 149.75}); + input.lines.push_back({_T("linePart1\nlinePart2\nlinePart3"), 153.09}); + + const std::tstring output = parsers::lrc::expand_text(input, true); + ASSERT(output == _T("[02:29.75][02:33.09]linePart1\r\n[02:29.75][02:33.09]linePart2\r\n[02:33.09]linePart3\r\n")); +} + +MVTF_TEST(lrcparse_expanding_splits_lines_with_matching_timestamps_in_their_original_order) +{ + LyricData input = {}; + input.lines.push_back({_T("lineBBBB\nlineAAAA"), 149.75}); + // These lines should remain in their given order, even though this is not lexicographic order, + // which the code might conceivably change if it involved a sort to check for equivalent lines + + const std::tstring output = parsers::lrc::expand_text(input, true); + ASSERT(output == _T("[02:29.75]lineBBBB\r\n[02:29.75]lineAAAA\r\n")); +} + +MVTF_TEST(lrcparse_expanding_does_not_merge_matching_lines_when_not_requested) +{ + LyricData input = {}; + input.lines.push_back({_T("thebestline"), 5.0}); + input.lines.push_back({_T("thebestline"), 10.0}); + input.lines.push_back({_T("anotherline"), 12.0}); + input.lines.push_back({_T("anotherline"), 14.0}); + + const std::tstring output = parsers::lrc::expand_text(input, false); + ASSERT(output == _T("[00:05.00]thebestline\r\n[00:10.00]thebestline\r\n[00:12.00]anotherline\r\n[00:14.00]anotherline\r\n")); +} + +MVTF_TEST(lrcparse_expanding_merges_matching_lines) +{ + LyricData input = {}; + input.lines.push_back({_T("thebestline"), 5.0}); + input.lines.push_back({_T("thebestline"), 10.0}); + input.lines.push_back({_T("anotherline"), 12.0}); + input.lines.push_back({_T("anotherline"), 14.0}); + + const std::tstring output = parsers::lrc::expand_text(input, true); + ASSERT(output == _T("[00:05.00][00:10.00]thebestline\r\n[00:12.00][00:14.00]anotherline\r\n")); +} + +MVTF_TEST(lrcparse_expanding_merges_matching_lines_with_matching_timestamps) +{ + LyricData input = {}; + input.lines.push_back({_T("thebestline-part1"), 5.0}); + input.lines.push_back({_T("thebestline-part2"), 5.0}); + input.lines.push_back({_T("anotherline-part1"), 10.0}); + input.lines.push_back({_T("anotherline-part2"), 10.0}); + input.lines.push_back({_T("thebestline-part1"), 15.0}); + input.lines.push_back({_T("thebestline-part2"), 15.0}); + + const std::tstring output = parsers::lrc::expand_text(input, true); + ASSERT(output == _T("[00:05.00][00:15.00]thebestline-part1\r\n[00:05.00][00:15.00]thebestline-part2\r\n[00:10.00]anotherline-part1\r\n[00:10.00]anotherline-part2\r\n")); +} + +MVTF_TEST(lrcparse_expanding_merges_matching_lines_in_timestamp_order) +{ + // Unfortunately I couldn't find a smaller example. + // This is the result of std::sort not being std::stable_sort, so it depends on std::sort doing "unstable" things + // which is not really something that is easily controlled from outside std::sort + LyricData input = {}; + input.lines.push_back({_T(""), 0.0}); + input.lines.push_back({_T("13"), 0.83}); + input.lines.push_back({_T("14"), 10.79}); + input.lines.push_back({_T("15"), 18.31}); + input.lines.push_back({_T(""), 20.96}); + input.lines.push_back({_T("16"), 35.27}); + input.lines.push_back({_T("17"), 44.97}); + input.lines.push_back({_T("18"), 50.21}); + input.lines.push_back({_T(""), 54.53}); + input.lines.push_back({_T("19"), 54.66}); + input.lines.push_back({_T("20"), 60.05}); + input.lines.push_back({_T("21"), 64.40}); + input.lines.push_back({_T("22"), 69.90}); + input.lines.push_back({_T(""), 75.51}); + input.lines.push_back({_T("23"), 79.39}); + input.lines.push_back({_T("24"), 89.12}); + input.lines.push_back({_T("1"), 94.28}); + input.lines.push_back({_T(""), 98.51}); + input.lines.push_back({_T("2"), 98.72}); + input.lines.push_back({_T("3"), 104.10}); + input.lines.push_back({_T("4"), 108.52}); + input.lines.push_back({_T("22"), 113.96}); + input.lines.push_back({_T(""), 119.64}); + input.lines.push_back({_T("5"), 137.93}); + input.lines.push_back({_T("6"), 148.06}); + input.lines.push_back({_T("7"), 154.95}); + input.lines.push_back({_T(""), 161.98}); + input.lines.push_back({_T("20"), 167.83}); + input.lines.push_back({_T(""), 172.02}); + input.lines.push_back({_T("8"), 172.14}); + input.lines.push_back({_T("9"), 177.64}); + input.lines.push_back({_T("10"), 182.76}); + input.lines.push_back({_T("11"), 186.76}); + input.lines.push_back({_T(""), 189.80}); + + const std::tstring output = parsers::lrc::expand_text(input, true); + const TCHAR* expected = _T( + "[00:00.00][00:20.96][00:54.53][01:15.51][01:38.51][01:59.64][02:41.98][02:52.02][03:09.80]\r\n" + "[00:00.83]13\r\n" + "[00:10.79]14\r\n" + "[00:18.31]15\r\n" + "[00:35.27]16\r\n" + "[00:44.97]17\r\n" + "[00:50.21]18\r\n" + "[00:54.66]19\r\n" + "[01:00.05][02:47.83]20\r\n" + "[01:04.40]21\r\n" + "[01:09.90][01:53.96]22\r\n" + "[01:19.39]23\r\n" + "[01:29.12]24\r\n" + "[01:34.28]1\r\n" + "[01:38.72]2\r\n" + "[01:44.10]3\r\n" + "[01:48.52]4\r\n" + "[02:17.93]5\r\n" + "[02:28.06]6\r\n" + "[02:34.95]7\r\n" + "[02:52.14]8\r\n" + "[02:57.64]9\r\n" + "[03:02.76]10\r\n" + "[03:06.76]11\r\n"); + ASSERT(output == expected); +} + +MVTF_TEST(lrcparse_expanding_places_untimestamped_lines_at_the_end_with_no_timestamp) +{ + LyricData input = {}; + input.lines.push_back({_T("timeline1"), 1.0}); + input.lines.push_back({_T("timeline2"), 2.0}); + input.lines.push_back({_T("untimed"), DBL_MAX}); + + const std::tstring output = parsers::lrc::expand_text(input, true); + ASSERT(output == _T("[00:01.00]timeline1\r\n[00:02.00]timeline2\r\nuntimed\r\n")); +} #endif diff --git a/src/sources/lrclib.cpp b/src/sources/lrclib.cpp index 2fde29e..8d14aa9 100644 --- a/src/sources/lrclib.cpp +++ b/src/sources/lrclib.cpp @@ -390,14 +390,14 @@ static void upload_lyrics(LyricData lyrics, const UploadChallenge& challenge, ui std::string synced_lyrics; if(lyrics.IsTimestamped()) { - synced_lyrics = from_tstring(parsers::lrc::expand_text(lyrics)); + synced_lyrics = from_tstring(parsers::lrc::expand_text(lyrics, false)); lyrics.RemoveTimestamps(); - plain_lyrics = from_tstring(parsers::lrc::expand_text(lyrics)); + plain_lyrics = from_tstring(parsers::lrc::expand_text(lyrics, false)); } else { - plain_lyrics = from_tstring(parsers::lrc::expand_text(lyrics)); + plain_lyrics = from_tstring(parsers::lrc::expand_text(lyrics, false)); } cJSON* content_root = cJSON_CreateObject(); diff --git a/src/ui_contextmenu.cpp b/src/ui_contextmenu.cpp index 2ad625c..16a4e42 100644 --- a/src/ui_contextmenu.cpp +++ b/src/ui_contextmenu.cpp @@ -90,7 +90,7 @@ class OpenLyricsContextSubItem : public contextmenu_item_simple if(success) { LyricData lyrics = update.get_result(); - std::tstring text = parsers::lrc::expand_text(lyrics); + std::tstring text = parsers::lrc::expand_text(lyrics, false); if(text.empty()) { popup_message::g_show("No lyrics saved", dialog_title.c_str()); diff --git a/src/ui_lyric_editor.cpp b/src/ui_lyric_editor.cpp index 44e352c..15dfa20 100644 --- a/src/ui_lyric_editor.cpp +++ b/src/ui_lyric_editor.cpp @@ -20,7 +20,7 @@ class LyricEditor : public CDialogImpl, private play_callback_impl_ // Dialog resource ID enum { IDD = IDD_LYRIC_EDIT }; - LyricEditor(LyricDataUnstructured lyrics, LyricUpdateHandle& update); + LyricEditor(LyricDataCommon common_data, std::tstring text, LyricUpdateHandle& update); ~LyricEditor() override; BEGIN_MSG_MAP_EX(LyricEditor) @@ -82,10 +82,10 @@ class LyricEditor : public CDialogImpl, private play_callback_impl_ fb2k::CCoreDarkModeHooks m_dark; }; -LyricEditor::LyricEditor(LyricDataUnstructured lyrics, LyricUpdateHandle& update) : +LyricEditor::LyricEditor(LyricDataCommon common_data, std::tstring text, LyricUpdateHandle& update) : m_update(update), - m_common_data(lyrics), - m_input_text(to_tstring(lyrics.text)) + m_common_data(common_data), + m_input_text(text) { } @@ -341,7 +341,7 @@ void LyricEditor::SelectLineWithTimestampGreaterOrEqual(double threshold_timesta void LyricEditor::SetEditorContents(const LyricData& lyrics) { - std::tstring new_contents = parsers::lrc::expand_text(lyrics); + std::tstring new_contents = parsers::lrc::expand_text(lyrics, false); SetDlgItemText(IDC_LYRIC_TEXT, new_contents.c_str()); SendDlgItemMessage(IDC_LYRIC_TEXT, EM_SCROLLCARET , 0, 0); } @@ -603,8 +603,8 @@ HWND SpawnLyricEditor(const LyricData& lyrics, LyricUpdateHandle& update) HWND result = nullptr; try { - LyricDataUnstructured unstructured = parsers::lrc::serialise(lyrics); - auto new_window = fb2k::newDialog(unstructured, update); + const std::tstring text = parsers::lrc::expand_text(lyrics, false); + auto new_window = fb2k::newDialog(lyrics, text, update); result = new_window->m_hWnd; } catch(const std::exception& e) diff --git a/src/ui_lyric_manual_search.cpp b/src/ui_lyric_manual_search.cpp index 6179785..d3cdbf4 100644 --- a/src/ui_lyric_manual_search.cpp +++ b/src/ui_lyric_manual_search.cpp @@ -293,7 +293,7 @@ LRESULT ManualLyricSearch::OnNotify(int /*idCtrl*/, LPNMHDR notify) assert(item_lyrics != nullptr); if(item_lyrics != nullptr) { - std::tstring lyrics_tstr = parsers::lrc::expand_text(*item_lyrics); + std::tstring lyrics_tstr = parsers::lrc::expand_text(*item_lyrics, false); SetDlgItemText(IDC_MANUALSEARCH_PREVIEW, lyrics_tstr.c_str()); } }