Merge branch 'jacquesh:main' into main

Author: illegitimate-egg

.github/workflows/check-pull-request-contents.yml

@@ -0,0 +1,22 @@
+name: Check pull request contents
+on: pull_request
+
+jobs:
+  check-changelist-modified:
+    name: Check that the changelog has been updated
+    runs-on: windows-2022
+    steps:
+      - name: Checkout
+        uses: actions/checkout@v4
+
+      - name: Check what files have been modified
+        uses: dorny/paths-filter@v3.0.2
+        id: filter
+        with:
+          filters: |
+            main:
+              - 'src/main.cpp'
+
+      - name: Fail if the changelog wasn't updated
+        if: ${{ steps.filter.outputs.main == 'false' }}
+        run: exit 1

.github/workflows/release.yml

@@ -16,6 +16,8 @@ jobs:
     name: Build Release
     runs-on: windows-latest
     needs: run-tests
+    permissions:
+      contents: write  # Needed to allow creating a release
 
     steps:
       - name: Add MSBuild to the PATH

.github/workflows/run_tests.yml

@@ -2,10 +2,12 @@ name: Run tests
 on:
   push:
     branches: main
+  pull_request:
   workflow_call:
 
 jobs:
   print-windows-sdks:
+    name: Log Windows SDKs
     runs-on: windows-2022
     steps:
       - name: Print installed Windows SDK versions
@@ -14,6 +16,7 @@ jobs:
         run: Get-ChildItem -Name "HKLM:\SOFTWARE\Microsoft\Windows Kits\Installed Roots"
 
   print-compiler-version:
+    name: Log MSVC compiler version
     runs-on: windows-2022
     steps:
       - name: Add MSVC compiler to the PATH
@@ -25,8 +28,8 @@ jobs:
         run: cl /?
 
   build-and-test-32bit:
+    name: Build & test - x86
     runs-on: windows-2022
-    name: Build and test - x86
     steps:
       - name: Add MSBuild to the PATH
         uses: microsoft/setup-msbuild@v2
@@ -41,8 +44,8 @@ jobs:
         run: build\Debug\test_foo_openlyrics.exe build\Debug\foo_openlyrics.dll
 
   build-and-test-64bit:
+    name: Build & test - x64
     runs-on: windows-2022
-    name: Build and test - x64
     steps:
       - name: Add MSBuild to the PATH
         uses: microsoft/setup-msbuild@v2

build/foo_openlyrics.vcxproj

@@ -301,6 +301,7 @@
     <ClInclude Include="..\src\resource.h" />
     <ClInclude Include="..\src\sources\lyric_source.h" />
     <ClInclude Include="..\src\stdafx.h" />
+    <ClInclude Include="..\src\string_split.h" />
     <ClInclude Include="..\src\tag_util.h" />
     <ClInclude Include="..\src\uie_shim_panel.h" />
     <ClInclude Include="..\src\ui_hooks.h" />

build/foo_openlyrics.vcxproj.filters

@@ -269,6 +269,9 @@
     <ClInclude Include="..\src\lyric_search.h">
       <Filter>Header Files</Filter>
     </ClInclude>
+    <ClInclude Include="..\src\string_split.h">
+      <Filter>Header Files</Filter>
+    </ClInclude>
   </ItemGroup>
   <ItemGroup>
     <ResourceCompile Include="..\src\foo_openlyrics.rc">

src/PCH.cpp

@@ -2,3 +2,11 @@
 
 #define MVTF_IMPLEMENTATION
 #include "mvtf/mvtf.h"
+
+#define STRINGSPLIT_IMPLEMENTATION
+#ifdef MVTF_TESTS_ENABLED
+#define STRINGSPLIT_TESTS_ENABLED
+#define STRINGSPLIT_TEST MVTF_TEST
+#define STRINGSPLIT_ASSERT ASSERT
+#endif
+#include "string_split.h"

src/main.cpp

@@ -1,5 +1,7 @@
 #include "stdafx.h"
 
+#include "mvtf/mvtf.h"
+
 // NOTE: We only pull in the generated header in release builds because that file gets recreated
 //       on every build, which forces every file that includes it to also be recompiled on every
 //       build. This is a significant waste of time during development.
@@ -17,17 +19,8 @@
 // Copied from the DECLARE_COMPONENT_VERSION(NAME,VERSION,ABOUT) macro, defined in foo_SDK/foobar2000/SDK/component_version.h
 namespace
 {
-    class OpenLyricsVersion : public componentversion
-    {
-        public:
-            void get_file_name(pfc::string_base& out) final {out = core_api::get_my_file_name();}
-            void get_component_name(pfc::string_base & out) final {out = "OpenLyrics";}
-            void get_component_version(pfc::string_base & out) final {out = OPENLYRICS_VERSION;}
-            void get_about_message(pfc::string_base & out) final;
-    };
-    static service_factory_single_t<OpenLyricsVersion> g_openlyricsversion_factory;
-}
-void OpenLyricsVersion::get_about_message(pfc::string_base & out)
+
+static void compute_about_message_string(pfc::string_base & out)
 {
     out = "foo_openlyrics " OPENLYRICS_VERSION "\n";
     out += "Open-source lyrics retrieval and display\n";
@@ -36,6 +29,10 @@ void OpenLyricsVersion::get_about_message(pfc::string_base & out)
     out += "\nChangelog:\n";
     // out += "Version " OPENLYRICS_VERSION " (" __DATE__ "):\n"
     // "\n";
+    out += "Version " OPENLYRICS_VERSION " (" __DATE__ "):\n"
+    "- Fix a crash when trying to show lyrics for a track with none saved\n"
+    "- Fix letras.com returning random lyrics when a good match isn't found\n"
+    "\n";
     out += "Version 1.11 (2024-09-05):\n"
     "- Add a source for Bandcamp.com\n"
     "- Fix SongLyrics.com sometimes returning lyrics saying there are no lyrics\n"
@@ -340,5 +337,47 @@ void OpenLyricsVersion::get_about_message(pfc::string_base & out)
     "- Initial release\n";
 }
 
+class OpenLyricsVersion : public componentversion
+{
+    public:
+        void get_file_name(pfc::string_base& out) final {out = core_api::get_my_file_name();}
+        void get_component_name(pfc::string_base & out) final {out = "OpenLyrics";}
+        void get_component_version(pfc::string_base & out) final {out = OPENLYRICS_VERSION;}
+        void get_about_message(pfc::string_base & out) final { compute_about_message_string(out); }
+};
+static service_factory_single_t<OpenLyricsVersion> g_openlyricsversion_factory;
+}
+
 // This will prevent users from renaming your component around (important for proper troubleshooter behaviors) or loading multiple instances of it.
 VALIDATE_COMPONENT_FILENAME("foo_openlyrics.dll");
+
+// ============
+// Tests
+// ============
+#ifdef MVTF_TESTS_ENABLED
+#include "string_split.h"
+
+MVTF_TEST(changelog_isnt_empty)
+{
+    pfc::string8 changelog;
+    compute_about_message_string(changelog);
+    ASSERT(!changelog.is_empty());
+}
+
+MVTF_TEST(changelog_has_no_lines_long_enough_to_wrap_in_the_fb2k_about_dialog)
+{
+    constexpr size_t max_non_wrapping_line_length = 76;
+    pfc::string8 changelog_pfc;
+    compute_about_message_string(changelog_pfc);
+
+    std::string_view changelog(changelog_pfc.ptr(), changelog_pfc.length());
+    string_split split(changelog, "\n");
+    while(!split.reached_the_end())
+    {
+        const std::string_view line = split.next();
+        ASSERT(line.length() <= max_non_wrapping_line_length);
+    }
+    ASSERT(!split.failed());
+}
+
+#endif

src/sources/letras.cpp

@@ -17,8 +17,6 @@ class LetrasSource : public LyricSourceRemote
     const GUID& id() const final { return src_guid; }
     std::tstring_view friendly_name() const final { return _T("letras.com"); }
 
-    std::string extract_lyrics_from_page(pfc::string8 page_content) const;
-
     std::vector<LyricDataRaw> search(const LyricSearchParams& params, abort_callback& abort) final;
     bool lookup(LyricDataRaw& data, abort_callback& abort) final;
 };
@@ -56,22 +54,6 @@ static std::string transform_artist_for_url(const std::string_view artist)
     return transform_tag_for_url(artist);
 }
 
-std::string LetrasSource::extract_lyrics_from_page(pfc::string8 page_content) const
-{
-    std::string lyric_text;
-    pugi::xml_document doc;
-    load_html_document(page_content.c_str(), doc);
-
-    pugi::xpath_query query_lyricdivs("//div[@class='lyric-original']");
-    pugi::xpath_node_set lyricdivs = query_lyricdivs.evaluate_node_set(doc);
-    if(!lyricdivs.empty())
-    {
-        add_all_text_to_string(lyric_text, lyricdivs.first().node());
-    }
-
-    return lyric_text;
-}
-
 std::vector<LyricDataRaw> LetrasSource::search(const LyricSearchParams& params, abort_callback& abort)
 {
     http_request::ptr request = http_client::get()->create_request("GET");
@@ -94,25 +76,45 @@ std::vector<LyricDataRaw> LetrasSource::search(const LyricSearchParams& params,
         return {};
     }
 
-    const std::string lyric_text = extract_lyrics_from_page(content);
-    if(lyric_text.empty())
+    LyricDataRaw result = {};
+    result.source_id = id();
+    result.source_path = url;
+
+    pugi::xml_document doc;
+    load_html_document(content.c_str(), doc);
+
+    std::string lyric_text;
+    pugi::xpath_query query_lyricdivs("//div[@class='lyric-original']");
+    pugi::xpath_node_set lyricdivs = query_lyricdivs.evaluate_node_set(doc);
+    if(!lyricdivs.empty())
+    {
+        add_all_text_to_string(lyric_text, lyricdivs.first().node());
+        result.text_bytes = string_to_raw_bytes(trim_surrounding_whitespace(lyric_text));
+    }
+
+    pugi::xpath_query query_title_elem("//div[@id='js-lyricHeader']//div[@class='title-content']//h1");
+    pugi::xpath_node_set title_element = query_title_elem.evaluate_node_set(doc);
+    if(!title_element.empty())
+    {
+        add_all_text_to_string(result.title, title_element.first().node());
+    }
+
+    pugi::xpath_query query_artist_elem("//div[@id='js-lyricHeader']//div[@class='title-content']//h2");
+    pugi::xpath_node_set artist_element = query_artist_elem.evaluate_node_set(doc);
+    if(!artist_element.empty())
+    {
+        add_all_text_to_string(result.artist, artist_element.first().node());
+    }
+
+    if(result.text_bytes.empty() || result.artist.empty() || result.title.empty())
     {
         throw new std::runtime_error("Failed to parse lyrics, the page format may have changed");
     }
     else
     {
         LOG_INFO("Successfully retrieved lyrics from %s", url.c_str());
-        const std::string_view trimmed_text = trim_surrounding_whitespace(lyric_text);
-
-        LyricDataRaw result = {};
-        result.source_id = id();
-        result.source_path = url;
-        result.artist = params.artist;
-        result.album = params.album;
-        result.title = params.title;
-        result.text_bytes = string_to_raw_bytes(trimmed_text);
 
-        const LyricData parsed = parsers::lrc::parse(result, trimmed_text);
+        const LyricData parsed = parsers::lrc::parse(result, lyric_text);
         result.type = parsed.IsTimestamped() ? LyricType::Synced : LyricType::Unsynced;
         return {std::move(result)};
     }