Key metadata track properties edit fix

[daschuer]

This fixed reloading and deleting of key value in the Track properties dialog.

These are the leftover commits form the key metadata handling PRs for #10890

[daschuer]

Rebased to resolve conflicts.

[Swiftb0y]

This tells me that the enum was deliberately made for error handling. Specifically Rejected should be checked because it almost always would require handling from the caller. This removes that feature, making the error checking optional (which I hope we can agree is very bad practice).

I need more explanation why its appropriate to weaken the guarantees this enum was supposed to provide.

[Swiftb0y]

Concretely: What is your rationale for removing the checks you removed in this PR?

[daschuer]

We have discussed this earlier and conclude that [[nodiscard]] is usefull if discarding is an error. In this case it is only an enum. Discarding can't be an error. It might be an error depending on the returning function. In our case it is updateGlobalKeyNormalizeText() wich works well without error checking. So the change allows us to remove static_cast<void>() on these calls. See at dlgtrackinfomulti.cpp

The counter example are places like this where [[nodiscard]] heps to avois programming errors:

mixxx/src/util/compatibility/qmutex.h

Line 32 in a4e61f9

[[nodiscard]] inline QT_MUTEX_LOCKER lockMutex(QMutex* pMutex) {

[Swiftb0y]

In this case it is only an enum. Discarding can't be an error.

Is it though? It seems that Rejected and Unchanged did need some special handling in some cases (which you have removed). I'm asking specifically why its fine to remove the handling for those cases here.

[daschuer]

I think so. I have left an inline comment:
#14022 (comment)

[daschuer]

Here we need to call displayKeyText() independent of the result of updateGlobalKeyNormalizeText() because we either want to see the normalized value or the old.

[ronso0]

Is there an agreement wrt the nodiscard issue?
If yes, I'd also take a look / test.

[Swiftb0y]

No agreement afaict. The type is made for error handling and error values should not be ignored. In the few cases they should, they can be documented explicitly using program logic (either explicitly ignore or check for them). If the check is unnecessary, the compiler can optimize them away.

[daschuer]

It looks like I have not made my point clear enough regarding [[nodiscard]].

We have this rule:
https://github.com/mixxxdj/mixxx/wiki/Coding-Guidelines#attribute-nodiscard

A single reasonable void cast on a no discard function IMHO that does not introduce a programming error rectifies that our rule is violated. There was already such a void cast in the 2.5 branch. This PR would have introduce another.

In addition it feels flawed to put a nodiscard on a type declaration, because it limits it use, it would belong to a function.

I am happy to reintroduce error checks if you can draft them along with a note which error it will fix.

[Swiftb0y]

We have this rule:

That rule has been written without considering this usecase. referring back to it unconditionally right now makes no sense.

UpdateResult is essentially an error code. If the return value can be easily ignored, the error handling is broken. Thats why [[nodiscard]] on this type is the right thing to do (and keep). See the bottom of this cppcoregl that supports this argument: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#e25-if-you-cant-throw-exceptions-simulate-raii-for-resource-management

In the few instances where it makes sense to ignore the return value, use std::ignore or just simply check the value anyway.

[daschuer]

For my understanding the use case has been considered. We have set this rule to not clutter our code with [[nodiscard]] and static_cast<void>() pairs. It is in addtion a bit cumbersome to add [[nodiscard]] to a return type, because this way it is out of sight when using a function returning the value.

I can confirm the view form the link. The discussion is around throwing an error, which will force error handling or breaking the program and the alterntive of forcing error handling by [[nodiscard]].

See also:
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es48-avoid-casts

Never cast to (void) to ignore a [[nodiscard]] return value. If you deliberately want to discard such a result, first think hard about whether that is really a good idea (there is usually a good reason the author of the function or of the return type used [[nodiscard]] in the first place).

In our case we have no reason to force error handling because there is nothing reasonable to handle IMHO. We do not not introduce a bug or anything so we don't need hard thinkining and hint later readers that we have ignored here something deliberal in an exeptional way.

The fucntion in question is here:

mixxx/src/track/trackrecord.cpp

Line 30 in ae2dc10

UpdateResult TrackRecord::updateGlobalKeyNormalizeText(

If you have and idea how the enforced error handling should look like, I am happy to add it to this PR, else let's merge it as it is.

[cr7pt0gr4ph7]

UpdateResult is (was) used as an optimization to skip updating the text shown in the UI when the underlying key hasn't changed.

Due to normalization, there are actually 4 different cases (given QString oldKey, newKey;):

1. oldKey == newKey && oldKey == normalize(newKey) && isValid(newKey) -> UpdateResult::Unchanged, no display text update required
2. oldKey != newKey && oldKey == normalize(newKey) && isValid(newKey) -> still UpdateResult::Unchanged, but display text update required
3. oldKey != normalize(newKey) && isValid(newKey) -> UpdateResult::Updated, display text update required
4. !isValid(newKey) -> UpdateResult::Rejected, display text update required

So it's not really about error handling, but about optimizing the synchronization UI <-> Data Model. The current code fails to handle handle case 2 properly, treating it as case 1 instead. Instead of distinguishing cases 1 and 2, this PR chooses to simply always update the display text, which is correct in all cases.

I think that removing [[nodiscard]] is the correct way forward here, as it's not really about error handling or resource leakage. UpdateResult is also used in only one additional place besides the track dialog.

[daschuer]

@Swiftb0y Is it OK to merge now?

[Swiftb0y]

Sorry, I have missed the ping previously. Thank you @cr7pt0gr4ph7 for the detailed explanation. It has motivated to me to indeed take another deep look at the surrounding code and can see how [nodiscard] was likely used without very good reason in this particular instance. Nevertheless, weakening the guarantees a type provides always needs careful consideration (to warrant my previous firm stance).

[Swiftb0y]

LGTM. Thank you.