ControlProxy tweaks #4229

daschuer · 2021-08-19T07:59:22Z

These are the tweaks form #1717 regarding the existing ControlProxy.
The new PollingControlProxy in #1717is now rebased on top of this.
Pleas see commit messages.

coveralls · 2021-08-19T16:27:06Z

Pull Request Test Coverage Report for Build 1151266515

38 of 46 (82.61%) changed or added relevant lines in 3 files are covered.
2 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.009%) to 25.958%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/control/control.cpp	21	25	84.0%
src/control/controlobject.cpp	9	13	69.23%

Files with Coverage Reduction	New Missed Lines	%
src/control/controlobject.cpp	1	89.66%
src/engine/enginevumeter.cpp	1	90.24%

Totals
Change from base Build 1148481046:	0.009%
Covered Lines:	20004
Relevant Lines:	77064

💛 - Coveralls

Holzhaus · 2021-08-19T16:51:42Z

src/control/control.h

@@ -37,7 +37,7 @@ class ControlDoublePrivate : public QObject {
    // Used to implement control persistence. All controls that are marked
    // "persist in user config" get and set their value on creation/deletion
    // using this UserSettings.
-    static void setUserConfig(UserSettingsPointer pConfig) {
+    static void setUserConfig(const UserSettingsPointer& pConfig) {


What is the purpose of passing a pointer by reference?

Passing smart pointer that uses atomics internally for bookkeeping is a valid choice.

But in this case when taking ownership I would suggest to use pass-by-value + std::move which would resemble Rust's move semantics.

Since QSharedPointer the underlying type of UserSettingsPointer is not movable this is the most efficient way to pass the pointer.

uklotzde

I am unsure about the default control itself. Ideally it should ignore all invocations of set and always return a default value, e.g. 0. Otherwise it might return varying values depending on by whom and when it is set and and by whom it is shared and read.

Due to race condition multiple default CO instances could be created. This makes debugging even more difficult.

uklotzde · 2021-08-19T21:01:35Z

src/control/control.h

@@ -133,7 +134,7 @@ class ControlDoublePrivate : public QObject {
    }
    void deleteCreatorCO();

-    ConfigKey getKey() {
+    inline const ConfigKey& getKey() {


Please don't add redundant inline direction within a class definition

src/control/control.cpp

src/control/control.h

daschuer · 2021-08-19T23:02:02Z

The default CO is read only. So there should not be an issue with random values.
You can however construct a case where this is bypassed.
Lets have a look what we can do against it.

uklotzde · 2021-08-20T10:50:25Z

The default CO is read only. So there should not be an issue with random values.
You can however construct a case where this is bypassed.
Lets have a look what we can do against it.

How do I know? Please add a comment for clarification. I didn't see why we can safely assume that this particular CO is never written.

uklotzde

If s_defaultCO is immutable then why should we use a QWeakPointer at all?? Allocating a single QSharedPointer that never gets out of scope would work perfectly fine then.

src/control/control.cpp

daschuer · 2021-08-20T15:12:06Z

If s_defaultCO is immutable then why should we use a QWeakPointer at all?? Allocating a single QSharedPointer that never gets out of scope would work perfectly fine then.

Than we have a leaking memory, valgind will complain about. Since it does not make a difference form the performance and uses the normal pattern to have not have a static shared pointer, I prefer using the weak pointer.

…ssarily

daschuer · 2021-08-20T16:06:49Z

Done

uklotzde

Thank you! LGTM

Holzhaus · 2021-09-01T15:30:20Z

https://bugs.launchpad.net/mixxx/+bug/1942350

This fixes lp1942350 a regression from mixxxdj#4229

daschuer mentioned this pull request Aug 19, 2021

PollingControlProxy #1717

Merged

daschuer requested a review from Holzhaus August 19, 2021 08:02

daschuer added 4 commits August 19, 2021 12:28

Added common kNullKey to lower the controlproxy memory footprint

1517dbd

Remove inefficient const char* constructor

08fa6df

use const ref

1af2412

Save a default CO to ommit null checks everywhere.

b8e8abb

daschuer force-pushed the controlproxytweaks branch from f4093e9 to f76c92e Compare August 19, 2021 10:31

Remove more redundant null checks

fb29af1

daschuer force-pushed the controlproxytweaks branch from f76c92e to fb29af1 Compare August 19, 2021 15:38

Holzhaus reviewed Aug 19, 2021

View reviewed changes

uklotzde suggested changes Aug 19, 2021

View reviewed changes

daschuer added 5 commits August 20, 2021 09:02

Improve assertions with ControlFlags

bebbd6a

Remove redundant inline keyword in a class definition

b65fefd

Make the default constructed ControlObject really const

294351a

move private static variables to anonymous namespace

b21d1b4

Protect against creating two default COs from two threads.

98d1c9f

daschuer force-pushed the controlproxytweaks branch from bb389e9 to 98d1c9f Compare August 20, 2021 08:48

uklotzde suggested changes Aug 20, 2021

View reviewed changes

daschuer added 6 commits August 20, 2021 17:14

Rename s_pDefaultCO

4e8b180

remove redundant static keyword from the anonymous namespace

f1a7b67

Remove redundant nullptr initalisation of QAtomicPointer

881eb71

Assert that ControlDoublePrivate::setUserConfig() is not called unece…

0e94198

…ssarily

Assert that persistant COs have access to the user settings

48de8ed

Make use of QWeakPointer::lock()

eb26629

uklotzde approved these changes Aug 20, 2021

View reviewed changes

uklotzde merged commit 6ad1449 into mixxxdj:main Aug 20, 2021

daschuer added a commit to daschuer/mixxx that referenced this pull request Sep 1, 2021

Silence warnings for expected invalid keys.

70fac19

This fixes lp1942350 a regression from mixxxdj#4229

daschuer mentioned this pull request Sep 1, 2021

Silence warnings for expected invalid keys. #4257

Merged

daschuer added a commit to daschuer/mixxx that referenced this pull request Sep 1, 2021

Silence warnings for expected invalid keys.

70ebab0

This fixes lp1942350 a regression from mixxxdj#4229

daschuer deleted the controlproxytweaks branch September 26, 2021 17:44

mixxxbot mentioned this pull request Aug 24, 2022

warning [Main] ControlDoublePrivate::getControl returning nullptr for invalid ConfigKey "" , "" #10507

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ControlProxy tweaks #4229

ControlProxy tweaks #4229

daschuer commented Aug 19, 2021

coveralls commented Aug 19, 2021 •

edited

Loading

Holzhaus Aug 19, 2021

uklotzde Aug 19, 2021

daschuer Aug 19, 2021

uklotzde left a comment •

edited

Loading

uklotzde Aug 19, 2021

daschuer commented Aug 19, 2021

uklotzde commented Aug 20, 2021 •

edited

Loading

uklotzde left a comment

daschuer commented Aug 20, 2021

daschuer commented Aug 20, 2021

uklotzde left a comment

Holzhaus commented Sep 1, 2021

ControlProxy tweaks #4229

ControlProxy tweaks #4229

Conversation

daschuer commented Aug 19, 2021

coveralls commented Aug 19, 2021 • edited Loading

Pull Request Test Coverage Report for Build 1151266515

💛 - Coveralls

Holzhaus Aug 19, 2021

Choose a reason for hiding this comment

uklotzde Aug 19, 2021

Choose a reason for hiding this comment

daschuer Aug 19, 2021

Choose a reason for hiding this comment

uklotzde left a comment • edited Loading

Choose a reason for hiding this comment

uklotzde Aug 19, 2021

Choose a reason for hiding this comment

daschuer commented Aug 19, 2021

uklotzde commented Aug 20, 2021 • edited Loading

uklotzde left a comment

Choose a reason for hiding this comment

daschuer commented Aug 20, 2021

daschuer commented Aug 20, 2021

uklotzde left a comment

Choose a reason for hiding this comment

Holzhaus commented Sep 1, 2021

coveralls commented Aug 19, 2021 •

edited

Loading

uklotzde left a comment •

edited

Loading

uklotzde commented Aug 20, 2021 •

edited

Loading