Refactor/preferences enums #12798

Swiftb0y · 2024-02-12T17:18:59Z

Alternative to #12796 along with a little extra refactoring

Swiftb0y · 2024-02-12T17:40:13Z

okay, enough force pushing now. I'd appreciate a review.

ronso0 · 2024-02-13T11:39:34Z

Looks good as far as I can tell, though I'd prefer to keep the constants compact therefore I removed the nested constants namespace (and tweaked some vars etc.)

Commit is b771309, or Swiftb0y#18, as you prefer.

Swiftb0y · 2024-02-13T14:31:33Z

I'd prefer to keep the constants compact therefore I removed the nested constants namespace

I'd prefer to remove that too, but its required to work around QTBUG-68611 (as pointed out in the comment above). If we remove the constants namespace, all other members of the preferences namespace would have to be declared in that file as well.

ronso0 · 2024-02-13T15:12:11Z

Hmkay, which other members are you referring to? Future extension?
My PR is build without issues, or when would we notice issues?

Swiftb0y · 2024-02-13T15:25:06Z

Hmkay, which other members are you referring to? Future extension?

Yes, future additions. In can imagine that this will happen as the QML transition progresses and we'll want to expose enums to it.

My PR is build without issues, or when would we notice issues?

You'll notice issues when you will try to add the following to some file (other than preferences/constants.h):

namespace mixxx {
namespace preferences {
Q_NAMESPACE
// add any members here that need the Q_NAMESPACE (eg Q_ENUM_NS)
}}

The same goes for the constants namespace, but I think its unlikely that you'll want to add something to that namespace from a file other than preferences/constants.h.

ronso0 · 2024-02-13T16:36:45Z

okay.

src/preferences/dialog/dlgprefinterface.cpp

ronso0 · 2024-02-13T16:45:32Z

src/preferences/dialog/dlgprefinterface.cpp

 #include <QtGlobal>

 #include "control/controlobject.h"
 #include "control/controlproxy.h"
 #include "defs_urls.h"
 #include "moc_dlgprefinterface.cpp"
+#include "preferences/constants.h"


can be moved to preferences/dialog/dlgpreferencepage.h to include it only once

... and be removed from src/mixxxmainwindow.cpp

are you sure about dlgpreferencepage.h? its not used in there anywhere.

That's the baseclass of all pref pages, would it increase the include 'load' including it there?
If so, just leave as it is.

not really, but its bad practice as well. It also wouldn't make any difference in terms of compilation time. In general its best to include what you use at the point of usage and nowhere else...

okay. it's already included in the header

yeah, so the cppcoreguidelines don't say much about that. If you insist, I'll remove the header from the .cpp.

Yes, please remove it. It'd be removed anyway with the next include cleanup.

Umm, this got a little mixed up, I was referring to the include in src/mixxxmainwindow.cpp.
Anyway, clean this up as much you like, I don't mind too much, just noticed it when looking for places where constants.h is included.

yup sorry. I don't think anymore cleanup is needed now.

Swiftb0y · 2024-02-27T10:48:23Z

friendly ping @ronso0. If you're happy with the current state, I'll squash the fixup.

ronso0 · 2024-02-27T10:58:36Z

No one else reviewed this, and my c++/enum knowledge can't compete with yours, so... it LGTM, it builds, it works AFAICT.
Ready for merge IMO.
If there are no objections within the next 24h I'll press the button @mixxxdj/developers

ronso0 · 2024-02-27T10:59:02Z

Can you squash the fixups and push?

Swiftb0y · 2024-02-27T11:04:30Z

src/preferences/constants.h

+// required for Qt-Macros
+#include <QObject>
+
 namespace mixxx {

+namespace preferences {
+
+namespace constants {
+Q_NAMESPACE
+
+// In order for this Q_NAMESPACE to work, all members of the namespace must
+// be declared here. see QTBUG-68611
+


I think for the other devs, this would be most controversial aspect. Putting a QObject header into this header just for the enums is not great in terms of compile-time overhead, but I think its a necessary evil anyways in order to make these enums usable from within QML.

Q_NAMESPACE is defined in qobjectdefs.h

Tried that, but I got MOC errors.

I cannot confirm this. Which Qt version did you try? I tried 6.2.3 and 5.12.8.

Tried 5.15.12. Though I may be misremembering what header I tried.

ronso0 · 2024-03-12T17:26:20Z

@mixxxdj/developers Does someone have time to take a look at this?

daschuer

Thank you. I have just two comments.

daschuer · 2024-03-12T19:30:04Z

src/mixxxmainwindow.cpp

@@ -1159,14 +1160,14 @@ bool MixxxMainWindow::eventFilter(QObject* obj, QEvent* event) {
                        "DlgPreferences") {
            // return true for no tool tips
            switch (m_toolTipsCfg) {
-            case mixxx::TooltipsPreference::TOOLTIPS_ONLY_IN_LIBRARY:
+            case mixxx::preferences::constants::Tooltips::OnlyInLibrary:


All the double :: look cluttered. Can we remove the "constants" namespace?

we discussed that here #12798 (comment)

Ah, I just read it. I can however not follow the reasoning. This PR introduces both "preferences::constants" there is no need to make it that long just to safe the "preferences" namespace for future use. We can introduce another independent namespace all the time.

Btw: We have no policy of folder matching namespaces like in C#

We can introduce another independent namespace all the time.

Right, but what if you want to use the preferences namespace? If its really about characters being typed/read, I can rename it to prefs::c I guess... but I really doubt that avoiding long namespaces here has any value. Avoiding nesting here also wouldn't have much value IMO.

We have no policy of folder matching namespaces like in C#

I'm aware, but the restriction on Q_NAMESPACE does force us to us to only declare the namespace in a single file, which is why it make sense to name it the same as the filename.

I have just checked how others do this:
They define a namespace each type and than class less enums:
https://github.com/zhuzichu520/FluentUI/blob/f830d5a9bfc0f6836e6e2aa7c80d7201b730013f/src/Def.h#L95
https://github.com/KDE/kdenlive/blob/7283b739900a01ef3caf86d458bedcf418ce1f60/src/definitions.h#L119

Lets follow them and not introduce preferences and constants namespace. Just mixxx is sufficient IMHO.

I don't like the class-less enums though. There is no reason to open up this enum to implicit conversions to int etc. wbout

namespace mixxx { namespace Tooltips { Q_NAMESPACE enum class TooltipsOptions {...} Q_ENUM_NS(TooltipsOptions) using enum TooltipsOptions; } }

would that work for you?

The reason is to get rid of the enum class namespace when using the enum in QML and use the extra namespace instead.

But it also works for me if we get rid of at least one of the extra namespaces as you like.

The reason is to get rid of the enum class namespace when using the enum in QML and use the extra namespace instead.

Well, QML is a different discussion here. IIUC we can rename the type as we'd like. I'm primarily focused on the C++ ergonomics right now.

daschuer · 2024-03-12T19:43:17Z

src/preferences/constants.h

+// required for Qt-Macros
+#include <QObject>
+
 namespace mixxx {

+namespace preferences {
+
+namespace constants {
+Q_NAMESPACE
+
+// In order for this Q_NAMESPACE to work, all members of the namespace must
+// be declared here. see QTBUG-68611
+


Q_NAMESPACE is defined in qobjectdefs.h

Swiftb0y · 2024-05-10T13:47:59Z

friendly ping @daschuer

…ferences::constants

Swiftb0y · 2024-05-16T13:18:13Z

I removed the namespace naming requirement by use of an inline namespace. wdyt @daschuer?

Swiftb0y · 2024-05-16T13:21:26Z

src/mixxxmainwindow.h

-    mixxx::TooltipsPreference m_toolTipsCfg;
+    mixxx::Tooltips m_toolTipsCfg;

-    mixxx::ScreenSaverPreference m_inhibitScreensaver;
+    mixxx::ScreenSaver m_inhibitScreensaver;


this looks a little short now, wdyt about reintroducing the preferences namespace again? Adding it to the enum name looks a little clunky.

I agree, mixxx:screensaver would imply a subsystem about screensaver handling. something like mixxx::preferences::screensaver make sense

daschuer · 2024-05-16T22:12:51Z

I removed the namespace naming requirement by use of an inline namespace. wdyt @daschuer?

Interesting. I didn't even know this feature exists.

Works for me:

            case mixxx::TooltipsPreference::TOOLTIPS_ONLY_IN_LIBRARY: // original 
            case mixxx::preferences::Tooltips::OnlyInLibrary: // proposal

…ants

…space

Swiftb0y · 2024-05-20T17:01:14Z

@daschuer I think I addressed everything

daschuer

LGTM, Thank you for polishing this.

github-actions bot added ui build preferences labels Feb 12, 2024

Swiftb0y force-pushed the refactor/preferences-enums branch 4 times, most recently from 3daa394 to 89e4f89 Compare February 12, 2024 17:39

ronso0 reviewed Feb 13, 2024

View reviewed changes

src/preferences/dialog/dlgprefinterface.cpp Show resolved Hide resolved

ronso0 reviewed Feb 13, 2024

View reviewed changes

Swiftb0y force-pushed the refactor/preferences-enums branch from 82451d4 to 3898dca Compare February 14, 2024 12:46

Swiftb0y commented Feb 27, 2024

View reviewed changes

Swiftb0y force-pushed the refactor/preferences-enums branch from 3898dca to 1a6ebd6 Compare February 27, 2024 11:06

ronso0 added the needs review label Mar 12, 2024

daschuer requested changes Mar 12, 2024

View reviewed changes

github-actions bot added the qml label Mar 20, 2024

Swiftb0y added 2 commits May 16, 2024 15:15

refactor(preferences): move enum constants into namespace mixxx::pre…

12f4caa

…ferences::constants

refactor(preferences): dlgprefinterface use namespace prefs::constants

f219a2d

Swiftb0y force-pushed the refactor/preferences-enums branch from 5cfa85e to 25f796f Compare May 16, 2024 13:17

Swiftb0y commented May 16, 2024

View reviewed changes

ronso0 and others added 2 commits May 17, 2024 10:18

refactor(preferences): use enum for QML multi-sampling mode

c598f26

refactor(preferences): avoid static_casts<int> for preferences::const…

c9c8e20

…ants

Swiftb0y force-pushed the refactor/preferences-enums branch from e43dc15 to ae28ae4 Compare May 17, 2024 08:31

refactor: hide preferences::constants namespace by use of inline name…

da7e474

…space

Swiftb0y force-pushed the refactor/preferences-enums branch from ae28ae4 to da7e474 Compare May 17, 2024 09:01

Swiftb0y mentioned this pull request May 20, 2024

feat: simplify waveform combobox #13220

Merged

Swiftb0y requested a review from daschuer May 20, 2024 17:00

acolombier mentioned this pull request May 20, 2024

Refactor WaveformFactory and Pref #13245

Open

daschuer approved these changes May 21, 2024

View reviewed changes

daschuer merged commit 8e4d26e into mixxxdj:main May 21, 2024
13 checks passed

Swiftb0y deleted the refactor/preferences-enums branch May 21, 2024 08:59

Refactor/preferences enums #12798

Refactor/preferences enums #12798

Conversation

Swiftb0y commented Feb 12, 2024

Swiftb0y commented Feb 12, 2024

ronso0 commented Feb 13, 2024

Swiftb0y commented Feb 13, 2024

ronso0 commented Feb 13, 2024

Swiftb0y commented Feb 13, 2024 • edited Loading

ronso0 commented Feb 13, 2024

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

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 Feb 27, 2024

ronso0 commented Feb 27, 2024

ronso0 commented Feb 27, 2024

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

ronso0 commented Mar 12, 2024

daschuer left a 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

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

Choose a reason for hiding this comment

Swiftb0y commented May 10, 2024

Swiftb0y commented May 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer commented May 16, 2024

Swiftb0y commented May 20, 2024

daschuer left a comment

Choose a reason for hiding this comment

Swiftb0y commented Feb 13, 2024 •

edited

Loading