Remove some skin internals from DlgPrefInterface #3891

Holzhaus · 2021-05-22T19:01:48Z

First step to untangle our Skin system a bit, so that we can have both legacy XML skins and QML skins at the same time.

This adds a new Skin class that we can later make multiple subclasses from. For now, this class is only used to simplify DlgPrefInterface.

Prerequisite for my QML skin PRs.

Holzhaus · 2021-05-22T19:04:41Z

Please ignore the code style issues, I used --no-verify to avoid mass reformattings when moving the legacy files.

uklotzde · 2021-05-25T16:50:10Z

Is this the first PR that should be merged before introducing the QML skin components?

Holzhaus · 2021-05-25T16:50:54Z

Should I open a separate PR for the first commit to make this easier to review?

Holzhaus · 2021-05-25T16:51:08Z

Is this the first PR that should be merged before introducing the QML skin components?

Yes.

uklotzde

No objections.

@ronso0?

uklotzde · 2021-05-25T16:56:38Z

src/skin/legacy/legacyskin.h

+class LegacySkin : public mixxx::skin::Skin {
+  public:
+    LegacySkin() = default;
+    LegacySkin(const QFileInfo& path);


ctor: explicit

dtor: Add an override = default destructor

uklotzde · 2021-05-25T17:06:10Z

We should keep the skin related changes in 2.3.x to a minimum or shortly release 2.4.0 afterwards to avoid merge conflicts.

Preventing merge conflicts is a valid reason for releasing a new version. Even if those features are not yet visible for users.

Holzhaus · 2021-05-25T17:15:05Z

We should keep the skin related changes in 2.3.x to a minimum or shortly release 2.4.0 afterwards to avoid merge conflicts.

Preventing merge conflicts is a valid reason for releasing a new version. Even if those features are not yet visible for users.

I mean I could even rebase the renaming commit on 2.3 and open a separate PR for that. That also makes this PR easier to review.

uklotzde · 2021-05-25T17:15:10Z

Should I open a separate PR for the first commit to make this easier to review?

No, this is mainly reorganization. If it works everything is fine.

Holzhaus · 2021-05-25T17:28:33Z

Should I open a separate PR for the first commit to make this easier to review?

No, this is mainly reorganization. If it works everything is fine.

Oops, I didn't see that comment and opend #3912. Should I close it or do we want to merge the mass renaming into 2.3 to avoid conflicts?

daschuer · 2021-05-25T19:50:29Z

Should I close it or do we want to merge the mass renaming into 2.3 to avoid conflicts?

Merging to 2.3 sounds reasonable.

Holzhaus · 2021-05-25T23:06:49Z

I rebased on latest main. Ready to review/merge. I'll rebase the follow-up PRs tomorrow.

Holzhaus · 2021-05-25T23:20:41Z

By the way, this also fixes the situation when you configured a custom skin which is broken. Now it automatically falls back to the default skin. Before, it only used the fallback if the configured skin is missing, not when it's broken.

Holzhaus · 2021-05-26T14:13:08Z

This has already been approved. Merge?

uklotzde · 2021-05-26T21:37:01Z

LGTM

daschuer · 2021-05-26T22:35:24Z

Unfortunately it segfaults when the skin folder is missing, after the error dialog:

[New Thread 0x7fff3f2fd700 (LWP 11181)]
[New Thread 0x7fff3e24d700 (LWP 11182)]
[New Thread 0x7fff3d6bb700 (LWP 11183)]
critical [Main] "Skin directory does not exist: /home/daniel/workspace/mixxx/res/skins"
critical [Main] "Skin directory does not exist: /home/daniel/workspace/mixxx/res/skins"
warning [Main] Configured skin "LateNight" not found, falling back to default skin "LateNight"
critical [Main] "Skin directory does not exist: /home/daniel/workspace/mixxx/res/skins"
--Type for more, q to quit, c to continue without paging--

Thread 1 "mixxx" received signal SIGSEGV, Segmentation fault.
0x0000555555783eb1 in DlgPrefInterface::slotUpdateSchemes (this=0x55555a83fba0)
at /usr/include/c++/9/bits/shared_ptr_base.h:1020
1020 _M_get() const noexcept
(gdb) bt
#0 0x0000555555783eb1 in DlgPrefInterface::slotUpdateSchemes()
(this=0x55555a83fba0) at /usr/include/c++/9/bits/shared_ptr_base.h:1020
#1 0x0000555555785df0 in DlgPrefInterface::DlgPrefInterface(QWidget*, MixxxMainWindow*, std::shared_ptrmixxx::skin::SkinLoader, QSharedPointer<ConfigObject >) (this=0x55555a83fba0, parent=, mixxx=
0x7fffffffdea0, pSkinLoader=..., pConfig=...)
at /home/daniel/workspace/mixxx/src/preferences/dialog/dlgprefinterface.cpp:123
#2 0x000055555577f0cd in DlgPreferences::DlgPreferences(MixxxMainWindow*, std::shared_ptrmixxx::skin::SkinLoader, std::shared_ptr, std::shared_ptr, std::shared_ptr, std::shared_ptr, LV2Backend*, std::shared_ptr, std::shared_ptr, std::shared_ptr)
(this=0x555558e8d5e0, mixxx=0x7fffffffdea0, pSkinLoader=std::shared_ptrmixxx::skin::SkinLoader (use count 4, weak count 0) = {...}, pSoundManager=..., pPlayerManager=..., pControllerManager=..., pVCManager=
std::shared_ptr (use count 3, weak count 0) = {...}, pLV2Backend=0x555556ad31d0, pEffectsManager=std::shared_ptr (use count 2, weak count 0) = {...}, pSettingsManager=std::shared_ptr (use count 2, weak count 0) = {...}, pLibrary=std::shared_ptr (use count 3, weak count 0) = {...}) at /usr/include/c++/9/bits/atomic_base.h:318
#3 0x000055555575be4d in MixxxMainWindow::MixxxMainWindow(QApplication*, std::shared_ptrmixxx::CoreServices)
--Type for more, q to quit, c to continue without paging--
(this=0x7fffffffdea0, pApp=0x7fffffffde60, pCoreServices=...)
at /usr/include/c++/9/ext/atomicity.h:96
#4 0x000055555571f93a in (anonymous namespace)::runMixxx
(args=..., app=0x7fffffffde60) at /usr/include/c++/9/ext/atomicity.h:96
#5 main(int, char**) (argc=, argv=)
at /home/daniel/workspace/mixxx/src/main.cpp:112

daschuer · 2021-05-26T22:38:46Z

It segfaults at the same place when the configured skin is missing:

warning [Main] Configured skin "LateNight" not found, falling back to default skin "LateNight"
--Type <RET> for more, q to quit, c to continue without paging--

Thread 1 "mixxx" received signal SIGSEGV, Segmentation fault.
0x0000555555783eb1 in DlgPrefInterface::slotUpdateSchemes (this=0x55555a8289d0)
    at /usr/include/c++/9/bits/shared_ptr_base.h:1020
1020	      _M_get() const noexcept
(gdb) bt -a
Argument to negate operation not a number.
(gdb) bt 
#0  0x0000555555783eb1 in DlgPrefInterface::slotUpdateSchemes()

daschuer · 2021-05-26T22:41:12Z

During shutdown I get this warning:

warning [LibraryScanner 1] QSqlDatabasePrivate::removeDatabase: connection 'MIXXX-2' is still in use, all queries will cease to work.

daschuer · 2021-05-26T22:43:53Z

During start of LateNight I see various debug assertions:

warning [Main] ControlDoublePrivate::getControl returning NULL for ( "[Channel3]" , "end_of_track" )
critical [Main] DEBUG ASSERT: "flags.testFlag(ControlFlag::NoAssertIfMissing)" in function static QSharedPointer<ControlDoublePrivate> ControlDoublePrivate::getControl(const ConfigKey&, ControlFlags, ControlObject*, bool, bool, bool, double) at /home/daniel/workspace/mixxx/src/control/control.cpp:152
critical [Main] DEBUG ASSERT: "m_pControl || flags.testFlag(ControlFlag::NoAssertIfMissing)" in function void ControlProxy::initialize(ControlFlags) at /home/daniel/workspace/mixxx/src/control/controlproxy.cpp:37
critical [Main] DEBUG ASSERT: "valid() || flags.testFlag(ControlFlag::NoAssertIfMissing)" in function void ControlProxy::initialize(ControlFlags) at /home/daniel/workspace/mixxx/src/control/controlproxy.cpp:38
warning [Main] ControlDoublePrivate::getControl returning NULL for ( "[Channel3]" , "time_remaining" )
critical [Main] DEBUG ASSERT: "flags.testFlag(ControlFlag::NoAssertIfMissing)" in function static QSharedPointer<ControlDoublePrivate> ControlDoublePrivate::getControl(const ConfigKey&, ControlFlags, ControlObject*, bool, bool, bool, double) at /home/daniel/workspace/mixxx/src/control/control.cpp:152
critical [Main] DEBUG ASSERT: "m_pControl || flags.testFlag(ControlFlag::NoAssertIfMissing)" in function void ControlProxy::initialize(ControlFlags) at /home/daniel/workspace/mixxx/src/control/controlproxy.cpp:37
critical [Main] DEBUG ASSERT: "valid() || flags.testFlag(ControlFlag::NoAssertIfMissing)" in function void ControlProxy::initialize(ControlFlags) at /home/daniel/workspace/mixxx/src/control/controlproxy.cpp:38
debug [Main] Created QGLWidget. Context Valid: true Sharing: true
debug [Main] Created QGLWidget. Context Valid: true Sharing: true
debug [Main] Created QGLWidget. Context Valid: true Sharing: true
warning [Main] ControlDoublePrivate::getControl returning NULL for ( "[Channel4]" , "end_of_track" )
critical [Main] DEBUG ASSERT: "flags.testFlag(ControlFlag::NoAssertIfMissing)" in function static QSharedPointer<ControlDoublePrivate> ControlDoublePrivate::getControl(const ConfigKey&, ControlFlags, ControlObject*, bool, bool, bool, double) at /home/daniel/workspace/mixxx/src/control/control.cpp:152
critical [Main] DEBUG ASSERT: "m_pControl || flags.testFlag(ControlFlag::NoAssertIfMissing)" in function void ControlProxy::initialize(ControlFlags) at /home/daniel/workspace/mixxx/src/control/controlproxy.cpp:37
critical [Main] DEBUG ASSERT: "valid() || flags.testFlag(ControlFlag::NoAssertIfMissing)" in function void ControlProxy::initialize(ControlFlags) at /home/daniel/workspace/mixxx/src/control/controlproxy.cpp:38
warning [Main] ControlDoublePrivate::getControl returning NULL for ( "[Channel4]" , "time_remaining" )
critical [Main] DEBUG ASSERT: "flags.testFlag(ControlFlag::NoAssertIfMissing)" in function static QSharedPointer<ControlDoublePrivate> ControlDoublePrivate::getControl(const ConfigKey&, ControlFlags, ControlObject*, bool, bool, bool, double) at /home/daniel/workspace/mixxx/src/control/control.cpp:152
critical [Main] DEBUG ASSERT: "m_pControl || flags.testFlag(ControlFlag::NoAssertIfMissing)" in function void ControlProxy::initialize(ControlFlags) at /home/daniel/workspace/mixxx/src/control/controlproxy.cpp:37
critical [Main] DEBUG ASSERT: "valid() || flags.testFlag(ControlFlag::NoAssertIfMissing)" in function void ControlProxy::initialize(ControlFlags) at /home/daniel/workspace/mixxx/src/control/controlproxy.cpp:38

daschuer · 2021-05-26T22:47:21Z

The fall back to the default skin works.

Holzhaus · 2021-05-26T23:01:04Z

It segfaults at the same place when the configured skin is missing:

warning [Main] Configured skin "LateNight" not found, falling back to default skin "LateNight"
--Type <RET> for more, q to quit, c to continue without paging--

Thread 1 "mixxx" received signal SIGSEGV, Segmentation fault.
0x0000555555783eb1 in DlgPrefInterface::slotUpdateSchemes (this=0x55555a8289d0)
    at /usr/include/c++/9/bits/shared_ptr_base.h:1020
1020	      _M_get() const noexcept
(gdb) bt -a
Argument to negate operation not a number.
(gdb) bt 
#0  0x0000555555783eb1 in DlgPrefInterface::slotUpdateSchemes()

I'll have a look tomorrow. In this case the configured skin is the default skin, so there is no way that Mixxx is able to start anyway.

Holzhaus · 2021-05-26T23:02:13Z

During shutdown I get this warning:

warning [LibraryScanner 1] QSqlDatabasePrivate::removeDatabase: connection 'MIXXX-2' is still in use, all queries will cease to work.

I see this all the time, even in 2.3 beta. It's always been like that IIRC.

uklotzde · 2021-05-26T23:22:02Z

During start of LateNight I see various debug assertions:

warning [Main] ControlDoublePrivate::getControl returning NULL for ( "[Channel3]" , "end_of_track" )
critical [Main] DEBUG ASSERT: "flags.testFlag(ControlFlag::NoAssertIfMissing)" in function static QSharedPointer<ControlDoublePrivate> ControlDoublePrivate::getControl(const ConfigKey&, ControlFlags, ControlObject*, bool, bool, bool, double) at /home/daniel/workspace/mixxx/src/control/control.cpp:152
critical [Main] DEBUG ASSERT: "m_pControl || flags.testFlag(ControlFlag::NoAssertIfMissing)" in function void ControlProxy::initialize(ControlFlags) at /home/daniel/workspace/mixxx/src/control/controlproxy.cpp:37
critical [Main] DEBUG ASSERT: "valid() || flags.testFlag(ControlFlag::NoAssertIfMissing)" in function void ControlProxy::initialize(ControlFlags) at /home/daniel/workspace/mixxx/src/control/controlproxy.cpp:38
warning [Main] ControlDoublePrivate::getControl returning NULL for ( "[Channel3]" , "time_remaining" )
critical [Main] DEBUG ASSERT: "flags.testFlag(ControlFlag::NoAssertIfMissing)" in function static QSharedPointer<ControlDoublePrivate> ControlDoublePrivate::getControl(const ConfigKey&, ControlFlags, ControlObject*, bool, bool, bool, double) at /home/daniel/workspace/mixxx/src/control/control.cpp:152
critical [Main] DEBUG ASSERT: "m_pControl || flags.testFlag(ControlFlag::NoAssertIfMissing)" in function void ControlProxy::initialize(ControlFlags) at /home/daniel/workspace/mixxx/src/control/controlproxy.cpp:37
critical [Main] DEBUG ASSERT: "valid() || flags.testFlag(ControlFlag::NoAssertIfMissing)" in function void ControlProxy::initialize(ControlFlags) at /home/daniel/workspace/mixxx/src/control/controlproxy.cpp:38
debug [Main] Created QGLWidget. Context Valid: true Sharing: true
debug [Main] Created QGLWidget. Context Valid: true Sharing: true
debug [Main] Created QGLWidget. Context Valid: true Sharing: true
warning [Main] ControlDoublePrivate::getControl returning NULL for ( "[Channel4]" , "end_of_track" )
critical [Main] DEBUG ASSERT: "flags.testFlag(ControlFlag::NoAssertIfMissing)" in function static QSharedPointer<ControlDoublePrivate> ControlDoublePrivate::getControl(const ConfigKey&, ControlFlags, ControlObject*, bool, bool, bool, double) at /home/daniel/workspace/mixxx/src/control/control.cpp:152
critical [Main] DEBUG ASSERT: "m_pControl || flags.testFlag(ControlFlag::NoAssertIfMissing)" in function void ControlProxy::initialize(ControlFlags) at /home/daniel/workspace/mixxx/src/control/controlproxy.cpp:37
critical [Main] DEBUG ASSERT: "valid() || flags.testFlag(ControlFlag::NoAssertIfMissing)" in function void ControlProxy::initialize(ControlFlags) at /home/daniel/workspace/mixxx/src/control/controlproxy.cpp:38
warning [Main] ControlDoublePrivate::getControl returning NULL for ( "[Channel4]" , "time_remaining" )
critical [Main] DEBUG ASSERT: "flags.testFlag(ControlFlag::NoAssertIfMissing)" in function static QSharedPointer<ControlDoublePrivate> ControlDoublePrivate::getControl(const ConfigKey&, ControlFlags, ControlObject*, bool, bool, bool, double) at /home/daniel/workspace/mixxx/src/control/control.cpp:152
critical [Main] DEBUG ASSERT: "m_pControl || flags.testFlag(ControlFlag::NoAssertIfMissing)" in function void ControlProxy::initialize(ControlFlags) at /home/daniel/workspace/mixxx/src/control/controlproxy.cpp:37
critical [Main] DEBUG ASSERT: "valid() || flags.testFlag(ControlFlag::NoAssertIfMissing)" in function void ControlProxy::initialize(ControlFlags) at /home/daniel/workspace/mixxx/src/control/controlproxy.cpp:38

Unrelated. This is a known issue on main with a fresh profile that has already been reported.

Holzhaus added the skins label May 22, 2021

Holzhaus added this to the 2.4.0 milestone May 22, 2021

github-actions bot added build code quality ui labels May 22, 2021

Holzhaus force-pushed the skin-refactor-pt1 branch from 3ba5bb4 to dbb5f21 Compare May 22, 2021 21:33

Holzhaus mentioned this pull request May 23, 2021

QML Pt. 1: Add initial support for loading QML skins #3894

Merged

Holzhaus force-pushed the skin-refactor-pt1 branch from c310c71 to f367b37 Compare May 24, 2021 10:39

Holzhaus requested a review from uklotzde May 24, 2021 16:20

Holzhaus mentioned this pull request May 25, 2021

QML Pt. 2: Add demo skin #3907

Merged

Holzhaus force-pushed the skin-refactor-pt1 branch from 61e0ece to a1f378d Compare May 25, 2021 15:13

uklotzde reviewed May 25, 2021

View reviewed changes

uklotzde approved these changes May 25, 2021

View reviewed changes

uklotzde requested a review from ronso0 May 25, 2021 17:07

Holzhaus added 6 commits May 26, 2021 00:53

skin/legacy: Add Skin abstraction

f287998

DlgPrefInterface: Use QMap of skins

e596e51

DlgPrefInterface: Remove unused parameter from slotSetSkinDescription

4675b64

DlgPrefInterface: Make some slots private

c4357ca

SkinLoader: Avoid detach of Qt container in range loop

b03a419

Skin: Add default constructor and add validity checks

6f56f2e

Holzhaus added 7 commits May 26, 2021 00:53

Skin: Add preview() method to retrieve preview image

ab8347d

Skin: Add abstract base class for LegacySkin

3a8effa

SkinLoader: Pass CoreServices to loadConfiguredSkin

d50070a

SkinLoader: Move actual loader implementation into LegacySkin class

728fcbd

SkinLoader: Move more legacy skin internals out of SkinLoader class

649feaf

SkinLoader: Move class into mixxx::skin:: namespace

512ca67

SkinLoader: Improve error handling (add fallback if skin is broken)

b94d0ee

Holzhaus force-pushed the skin-refactor-pt1 branch from a1f378d to b94d0ee Compare May 25, 2021 22:57

uklotzde merged commit 1169734 into mixxxdj:main May 26, 2021

ronso0 mentioned this pull request May 26, 2021

show preferences when configured skin failed to load #3845

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove some skin internals from DlgPrefInterface #3891

Remove some skin internals from DlgPrefInterface #3891

Holzhaus commented May 22, 2021 •

edited

Loading

Holzhaus commented May 22, 2021

uklotzde commented May 25, 2021

Holzhaus commented May 25, 2021

Holzhaus commented May 25, 2021

uklotzde left a comment

uklotzde May 25, 2021

uklotzde commented May 25, 2021

Holzhaus commented May 25, 2021

uklotzde commented May 25, 2021

Holzhaus commented May 25, 2021

daschuer commented May 25, 2021 •

edited

Loading

Holzhaus commented May 25, 2021 •

edited

Loading

Holzhaus commented May 25, 2021

Holzhaus commented May 26, 2021

uklotzde commented May 26, 2021

daschuer commented May 26, 2021

daschuer commented May 26, 2021

daschuer commented May 26, 2021

daschuer commented May 26, 2021 •

edited

Loading

daschuer commented May 26, 2021

Holzhaus commented May 26, 2021

Holzhaus commented May 26, 2021

uklotzde commented May 26, 2021

Remove some skin internals from DlgPrefInterface #3891

Remove some skin internals from DlgPrefInterface #3891

Conversation

Holzhaus commented May 22, 2021 • edited Loading

Holzhaus commented May 22, 2021

uklotzde commented May 25, 2021

Holzhaus commented May 25, 2021

Holzhaus commented May 25, 2021

uklotzde left a comment

Choose a reason for hiding this comment

uklotzde May 25, 2021

Choose a reason for hiding this comment

uklotzde commented May 25, 2021

Holzhaus commented May 25, 2021

uklotzde commented May 25, 2021

Holzhaus commented May 25, 2021

daschuer commented May 25, 2021 • edited Loading

Holzhaus commented May 25, 2021 • edited Loading

Holzhaus commented May 25, 2021

Holzhaus commented May 26, 2021

uklotzde commented May 26, 2021

daschuer commented May 26, 2021

daschuer commented May 26, 2021

daschuer commented May 26, 2021

daschuer commented May 26, 2021 • edited Loading

daschuer commented May 26, 2021

Holzhaus commented May 26, 2021

Holzhaus commented May 26, 2021

uklotzde commented May 26, 2021

Holzhaus commented May 22, 2021 •

edited

Loading

daschuer commented May 25, 2021 •

edited

Loading

Holzhaus commented May 25, 2021 •

edited

Loading

daschuer commented May 26, 2021 •

edited

Loading