Implement diacritics stripping and string normalization command by mierak · Pull Request #2329 · MusicPlayerDaemon/MPD

mierak · 2025-06-28T09:58:35Z

Attempt to implement #2327

Introduces new stringnormalization protocol command
Implements first string normalization to strip diacritics when using search and the related commands

The implementation works, but I am unsure whether I took the right approach (and its my first go at CPP). The threading through of booleans is also getting pretty gnarly in there. So I am mostly looking for feedback at this point. We can also workshop the stringnormalization name.

MaxKellermann · 2025-06-29T19:36:18Z

src/song/StringFilter.hxx


 	bool GetFoldCase() const noexcept {
-		return fold_case;
+		return icu_compare;


Is this correct? If case folding is disabled but diacritics stripping is enabled, this will return true, won't it?

Yup, I missed this one, thanks.

MaxKellermann · 2025-07-31T17:03:16Z

Lots and lots of build failures:

../../test/TestStringFilter.cxx: In member function ‘virtual void StringFilterTest_ASCII_Test::TestBody()’:
../../test/TestStringFilter.cxx:23:82: error: no matching function for call to ‘StringFilter::StringFilter(<brace-enclosed initializer list>)’
   23 |         const StringFilter f{"needle", false, StringFilter::Position::FULL, false};
      |                                                                                  ^

And some bad coding style, uninitialized variables, .... sigh.

MaxKellermann · 2025-07-31T17:09:38Z

Plus Windows build failures:

../../src/lib/icu/Compare.cxx:34:1: error: no declaration matches 'IcuCompare::IcuCompare(std::string_view)'
   34 | IcuCompare::IcuCompare(std::string_view _needle) noexcept
      | ^~~~~~~~~~

Same build failure when building without ICU.

When you fix a bad commit, don't add a fixup commit - instead, amend the existing known-bad commit.

mierak · 2025-07-31T19:06:11Z

Lots and lots of build failures:
Same build failure when building without ICU.

Compiles for me now both with and without ICU. I have fixed the tests and added a simple test case for diacritic stripping both with and without fold case as well.

When you fix a bad commit, don't add a fixup commit - instead, amend the existing known-bad commit.

I have squashed them into one since there was already more than one commit.

Plus Windows build failures

Hopefully should be fine now, but I do not have means to test a windows build atm.

bad coding style

Not much to go of off here, dev docs state to use spaces, I have tried to follow that as well as to copy the style elsewhere. Im going to need a bit more than "bad"

uninitialized variables, .... sigh.

Which ones?

MaxKellermann · 2025-08-01T04:56:27Z

bad coding style

Not much to go of off here, dev docs state to use spaces, I have tried to follow that as well as to copy the style elsewhere. Im going to need a bit more than "bad"

Where does it say that?

uninitialized variables, .... sigh.

Which ones?

You added new variables to class IcuCompare, but forgot to initialize them in the copy/move constructors.

MaxKellermann

Just ... look at the diff and you see your indentation is all wrong.

MaxKellermann · 2025-08-01T04:58:26Z

src/command/ClientCommands.cxx

+	if (request.empty()) {
+		string_normalizations_print(client, r);
+		return CommandResult::OK;
+    }


MaxKellermann · 2025-08-01T04:58:48Z

src/lib/icu/Canonicalize.cxx

+#include <fmt/format.h>
+#include <string_view>


MaxKellermann · 2025-08-01T04:58:58Z

src/lib/icu/Canonicalize.cxx

+
+	strip_diacritics_transliterator =
+		new IcuTransliterator(ToStringView(std::span{UCharFromUTF8(strip_diacritics_id)}),
+			  {});


MaxKellermann · 2025-08-01T04:59:04Z

src/lib/icu/Canonicalize.cxx


+	if (strip_diacritics) {
+		if (auto s = strip_diacritics_transliterator->Transliterate(ToStringView(std::span{u}));
+			s != nullptr)


MaxKellermann · 2025-08-01T04:59:14Z

src/lib/icu/Compare.cxx

+	fold_case(_fold_case),
+	strip_diacritics(_strip_diacritics) {}


MaxKellermann · 2025-08-01T04:59:34Z

src/lib/icu/Compare.hxx

+    bool fold_case;
+    bool strip_diacritics;


MaxKellermann · 2025-08-01T04:59:38Z

src/lib/icu/Compare.hxx

+    bool GetFoldCase() const noexcept {
+        return needle != nullptr && fold_case;
+    }


MaxKellermann · 2025-08-01T04:59:57Z

test/TestStringFilter.cxx

 	EXPECT_TRUE(f.Match("’"));
 	EXPECT_FALSE(f.Match("\""));
+	EXPECT_TRUE(StringFilter("áéíóúýčďěňřšťžůåäöüàãâçêõîşûğăôơư", true, true, StringFilter::Position::FULL, false)
+            .Match("aeiouycdenrstzuaaouaaaceoisugaoou"));


MaxKellermann · 2025-08-01T04:59:59Z

test/TestStringFilter.cxx

+	EXPECT_FALSE(StringFilter("ÁÉÍÓÚÝČĎĚŇŘŠŤŽŮÅÄÖÜÀÃÂÇÊÕÎŞÛĞĂÔƠƯ", false, true, StringFilter::Position::FULL, false)
+            .Match("áéíóúýčďěňřšťžůåäöüàãâçêõîşûğăôơư"));
 }



MaxKellermann · 2025-08-01T05:00:01Z

test/TestStringFilter.cxx

+	EXPECT_TRUE(StringFilter("áéíóúýčďěňřšťžůåäöüàãâçêõîşûğăôơư", true, true, StringFilter::Position::FULL, false)
+            .Match("aeiouycdenrstzuaaouaaaceoisugaoou"));
+	EXPECT_TRUE(StringFilter("ÁÉÍÓÚÝČĎĚŇŘŠŤŽŮÅÄÖÜÀÃÂÇÊÕÎŞÛĞĂÔƠƯ", true, true, StringFilter::Position::FULL, false)
+            .Match("áéíóúýčďěňřšťžůåäöüàãâçêõîşûğăôơư"));


MaxKellermann · 2025-08-01T05:01:48Z

Plus Windows build failures

Hopefully should be fine now, but I do not have means to test a windows build atm.

GH Actions disagrees.

../src/lib/icu/Compare.cxx: In constructor 'IcuCompare::IcuCompare(std::string_view, bool, bool)':
../src/lib/icu/Compare.cxx:23:41: warning: unused parameter '_needle' [-Wunused-parameter]
   23 | IcuCompare::IcuCompare(std::string_view _needle, bool _fold_case, bool _strip_diacritics) noexcept
      |                        ~~~~~~~~~~~~~~~~~^~~~~~~
../src/lib/icu/Compare.cxx: At global scope:
../src/lib/icu/Compare.cxx:27:1: error: expected unqualified-id before '{' token
   27 | {
      | ^

mierak · 2025-08-05T19:20:40Z

Where does it say that?

I mistyped in a haste, meant to say tabs, sorry.

Just ... look at the diff and you see your indentation is all wrong.

My apologies, I have misconfigured my editor (only set tabwidth for one file) and my github was on 4 spaces so the diffs looked completely fine to me. Not an excuse, just an explanation. Hopefully they are fine now but tell me if you find any more.

You added new variables to class IcuCompare, but forgot to initialize them in the copy/move constructors.

I am a bit out of my league here since I am not very familiar with cpp in particular as I have said in the PR description. I have done my reading but am not completely sure I have done the implementation correctly so I would appreciate a feedback on them, thanks!
The bools are initialized in the copy constructors and the move constructor as well and I have attempted to default the source in the latter.

GH Actions disagrees.

The GH build action now succeeded in my fork I had an extra pair of braces in there.

src/lib/icu/Compare.hxx

MaxKellermann · 2025-08-07T07:44:12Z

src/lib/icu/Compare.hxx

-	IcuCompare(IcuCompare &&) = default;
-	IcuCompare &operator=(IcuCompare &&) = default;
+	IcuCompare(IcuCompare &&src) noexcept
+		:needle(src
+			? AllocatedString(std::move(src.needle))
+			: nullptr),
+		 fold_case(src.fold_case),
+		 strip_diacritics(src.strip_diacritics)
+	{
+		src.needle = nullptr;
+		src.fold_case = false;
+		src.strip_diacritics = false;
+	}
+
+	IcuCompare &operator=(IcuCompare &&src) noexcept {
+		needle = src
+			? AllocatedString(std::move(src.needle))
+			: nullptr;
+		fold_case = src.fold_case;
+		strip_diacritics = src.strip_diacritics;
+
+		src.needle = nullptr;
+		src.fold_case = false;
+		src.strip_diacritics = false;
+		return *this;
+	}


What's the point of this change?

That is what I was asking about in the last comment. To me it seemed like default should already do exactly what I did here but you specifically pointed out that I did not initialize variables in the move constructor.
If this is indeed not needed then I can roll this one back.

That comment was wrong, sorry - I meant the copy ctor+operator, not copy+move ctor.

All good, I have rolled the change back.

mierak mentioned this pull request Jun 28, 2025

Feat/add diacritics mierak/rmpc#498

Closed

5 tasks

MaxKellermann reviewed Jun 29, 2025

View reviewed changes

mierak force-pushed the master branch from 4e85e52 to db41e32 Compare July 31, 2025 18:59

MaxKellermann requested changes Aug 1, 2025

View reviewed changes

mierak force-pushed the master branch 2 times, most recently from ff0f465 to 52cb762 Compare August 5, 2025 18:46

mierak requested a review from MaxKellermann August 5, 2025 19:58

MaxKellermann reviewed Aug 6, 2025

View reviewed changes

src/lib/icu/Compare.hxx Outdated Show resolved Hide resolved

mierak force-pushed the master branch from 52cb762 to d10e539 Compare August 6, 2025 10:19

mierak requested a review from MaxKellermann August 7, 2025 07:27

MaxKellermann reviewed Aug 7, 2025

View reviewed changes

mierak force-pushed the master branch from d10e539 to 1204761 Compare August 7, 2025 13:55

Implement diacritics stripping and string normalization command

c56214e

mierak force-pushed the master branch from 1204761 to c56214e Compare August 7, 2025 13:57

MaxKellermann merged commit 20f9a43 into MusicPlayerDaemon:master Aug 8, 2025

mierak mentioned this pull request Aug 8, 2025

Ability to ignore diacritics when searching/filtering mierak/rmpc#359

Closed

jcorporation mentioned this pull request Aug 12, 2025

Support stringnormalization command (MPD 0.25) jcorporation/myMPD#1456

Closed

mierak mentioned this pull request Sep 13, 2025

Strip diacritics when using search #2327

Closed

		fold_case(_fold_case),
		strip_diacritics(_strip_diacritics) {}

		#include <fmt/format.h>
		#include <string_view>

Comments

Conversation

mierak commented Jun 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaxKellermann commented Jul 31, 2025

Uh oh!

MaxKellermann commented Jul 31, 2025

Uh oh!

mierak commented Jul 31, 2025

Uh oh!

MaxKellermann commented Aug 1, 2025

Uh oh!

MaxKellermann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaxKellermann commented Aug 1, 2025

Uh oh!

mierak commented Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mierak Aug 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mierak commented Jun 28, 2025 •

edited

Loading

mierak commented Aug 5, 2025 •

edited

Loading

mierak Aug 7, 2025 •

edited

Loading