add `ConfigObject::get-/setValue<EnumType>` #11883

Swiftb0y · 2023-08-28T15:11:45Z

This allows the callsites to avoid verbose static_casts (see 491b436). It also makes it easier to change the machanism used when storing enums in the config easier in the future. The remaining commits are cleanups I did along the way while making use of the new overload.

daschuer · 2023-08-28T22:51:13Z

src/preferences/configobject.h

    template <class ResultType>
    ResultType getValue(const ConfigKey& key, const ResultType& default_value) const;
    QString getValue(const ConfigKey& key, const char* default_value) const;
+    template<typename EnumType>
+    requires std::is_enum_v<EnumType>


Suggested change

template <class ResultType>

ResultType getValue(const ConfigKey& key, const ResultType& default_value) const;

QString getValue(const ConfigKey& key, const char* default_value) const;

template<typename EnumType>

requires std::is_enum_v<EnumType>

template <class ResultType, std::enable_if_t<!std::is_enum_v<ResultType>, bool> = true>

ResultType getValue(const ConfigKey& key, const ResultType& default_value) const;

QString getValue(const ConfigKey& key, const char* default_value) const;

template<typename EnumType, std::enable_if_t<std::is_enum_v<EnumType>, bool> = true>

explicitly opting out of enums for the default template is unnecessary when using requires as the template with requires has a higher specificity and is thus chosen over the unconstrained template. I'd prefer to not have the negative requirement, as the maintenance burden will explode in case we add even more templates.

daschuer · 2023-08-28T22:54:34Z

src/preferences/configobject.h

    template <class ResultType>
    void setValue(const ConfigKey& key, const ResultType& value);
+    template<class EnumType>
+    requires std::is_enum_v<EnumType>


We need to use std::enable_if if you want to merge this into 2.4 because requires does not work with Ubuntu Focal
The alternative would be to merge that to main, because this is not a last minute bug fix.
But I consider that a low risk PR that we can let slip in.

Suggested change

template <class ResultType>

void setValue(const ConfigKey& key, const ResultType& value);

template<class EnumType>

requires std::is_enum_v<EnumType>

template <class ResultType, std::enable_if_t<!std::is_enum_v<ResultType>, bool> = true>

void setValue(const ConfigKey& key, const ResultType& value);

template<class EnumType, std::enable_if_t<std::is_enum_v<EnumType>, bool> = true>

The alternative would be to merge that to main, because this is not a last minute bug fix.

I'd prefer merging to main then. The error messages generated when using enable_if are much uglier than the ones generated by requires clauses.

src/mixxxmainwindow.cpp

src/preferences/dialog/dlgprefautodj.cpp

Swiftb0y · 2023-08-29T07:15:56Z

I'll push a couple fixups. If you're okay with them I'll squash them and rebase onto main in a single step after approval. Reverting to draft as long as the history contains fixups.

daschuer

LGTM, Ready to rebase.

src/preferences/configobject.h

Swiftb0y · 2023-08-29T18:59:59Z

rebased to main, fixups are gone and it builds. merge?

Holzhaus · 2023-08-29T19:55:33Z

.pre-commit-config.yaml

@@ -91,7 +91,7 @@ repos:
          - manual
        language: python
        additional_dependencies:
-          - clang-format==14.0.6
+          - clang-format==16.0.6


Isn't this from the pre-commit PR? Did you pull 2.4 into main?

No it was required to resolve the messed formatting I posted about above. I didn't pull 2.4 into main because thats currently broken IIUC. I just figured I'd just update it here and the merge conflict will be easy to resolve later. The Alternative would be to cherry-pick #11889 into main.

Hmm, my local test worked fine, but if we changed deps this does look weird. Since #11839 has already been merged I'd rather take care of updating the vcpkg env now rather than risking that changes in 2.4 pile up and become a pain to resolve later on.

I agree, I just don't like messing with the buildenv as that's not my area of expertise. Thank you for taking care.

daschuer

OK, we see now everything is green. So I suggest to remove the .pre-commit-config.yaml and we can merge this with the failing pre-commit (if #11892 does not make it before). This will avoid later conflicts and doe snot block this PR.

Swiftb0y · 2023-08-30T13:53:15Z

removed the pre-commit change. CI is obviously still failing due it being broken on main.

This allows the callsites to avoid verbose `static_cast`s. It also makes it easier to change the machanism used when storing enums in the config easier in the future.

… the config

Swiftb0y · 2023-08-31T12:04:32Z

merge?

daschuer · 2023-08-31T16:25:35Z

Yes, thank you.

Swiftb0y · 2023-08-31T17:30:51Z

Thank you

github-actions bot added library ui labels Aug 28, 2023

daschuer reviewed Aug 28, 2023

View reviewed changes

daschuer requested changes Aug 28, 2023

View reviewed changes

Swiftb0y marked this pull request as draft August 29, 2023 07:21

daschuer approved these changes Aug 29, 2023

View reviewed changes

Swiftb0y changed the base branch from 2.4 to main August 29, 2023 15:21

Swiftb0y force-pushed the refactor/ConfigObject branch from 3c716dd to 213e08c Compare August 29, 2023 15:22

Swiftb0y marked this pull request as ready for review August 29, 2023 15:22

Swiftb0y commented Aug 29, 2023

View reviewed changes

src/preferences/configobject.h Outdated Show resolved Hide resolved

Swiftb0y force-pushed the refactor/ConfigObject branch from 213e08c to a587631 Compare August 29, 2023 16:05

github-actions bot added the code quality label Aug 29, 2023

Holzhaus approved these changes Aug 29, 2023

View reviewed changes

daschuer requested changes Aug 30, 2023

View reviewed changes

Swiftb0y force-pushed the refactor/ConfigObject branch from a587631 to 5d63b5c Compare August 30, 2023 13:48

github-actions bot removed the code quality label Aug 30, 2023

Swiftb0y added 8 commits August 30, 2023 22:45

feat: add ConfigObject::get-/setValue<EnumType>

e71ae8f

This allows the callsites to avoid verbose `static_cast`s. It also makes it easier to change the machanism used when storing enums in the config easier in the future.

refactor: remove unnecessary static_casts when storing enum values in…

9323870

… the config

refactor: DlgAutoDJ::slotRepeatPlaylistChanged(s/int/bool/)

79c6a88

improve config value saving type-safety

2f50b2a

improve typesafety of signals and when interacting with config

617e5a6

simplify ScreensaverManager

2d79173

simplify WaveformWidgetFactory::setWidgetType

fe826c3

prevent unnecessary QString construction

25462c7

Swiftb0y force-pushed the refactor/ConfigObject branch from 5d63b5c to 25462c7 Compare August 30, 2023 20:45

daschuer merged commit 18d695d into mixxxdj:main Aug 31, 2023
11 checks passed

Swiftb0y deleted the refactor/ConfigObject branch August 31, 2023 17:31

Swiftb0y mentioned this pull request Sep 10, 2023

Expose [BPM],sync_lock_algorithm option in deck preferences. #11828

Merged

ronso0 mentioned this pull request Feb 12, 2024

Preferences: touch multi-sampling option only with QML enabled #12794

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add `ConfigObject::get-/setValue<EnumType>` #11883

add `ConfigObject::get-/setValue<EnumType>` #11883

Swiftb0y commented Aug 28, 2023

daschuer Aug 28, 2023

Swiftb0y Aug 29, 2023

daschuer Aug 28, 2023

Swiftb0y Aug 29, 2023

daschuer Aug 29, 2023

Swiftb0y commented Aug 29, 2023

daschuer left a comment

Swiftb0y commented Aug 29, 2023

Holzhaus Aug 29, 2023

Swiftb0y Aug 29, 2023

Holzhaus Aug 29, 2023

Holzhaus Aug 29, 2023

Swiftb0y Aug 29, 2023

daschuer left a comment

Swiftb0y commented Aug 30, 2023

Swiftb0y commented Aug 31, 2023

daschuer commented Aug 31, 2023

Swiftb0y commented Aug 31, 2023

add ConfigObject::get-/setValue<EnumType> #11883

add ConfigObject::get-/setValue<EnumType> #11883

Conversation

Swiftb0y commented Aug 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Swiftb0y commented Aug 29, 2023

daschuer left a comment

Choose a reason for hiding this comment

Swiftb0y commented Aug 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer left a comment

Choose a reason for hiding this comment

Swiftb0y commented Aug 30, 2023

Swiftb0y commented Aug 31, 2023

daschuer commented Aug 31, 2023

Swiftb0y commented Aug 31, 2023

add `ConfigObject::get-/setValue<EnumType>` #11883

add `ConfigObject::get-/setValue<EnumType>` #11883