Control widget property connection improvements#3806
Conversation
…id hash lookups at runtime.
|
@daschuer I can confirm this PR fixes the lag as well. Thanks |
| vParameter.convert(m_property.type()); | ||
| if (m_propertyValue == vParameter) { | ||
| // don't repeat writing the same value that may stall the GUI | ||
| // Comparing the property value directly does not work, because | ||
| // in some cases (e.g. visible) it has to be propagated to the child widgets. | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
I'm not sure if this is a good idea. If I understand correctly, this code basically deduplicates subsequent signals which carry the same value. Some GUI controls might rely on the fact that those "duplicate" signals are not swallowed.
There was a problem hiding this comment.
What are the alternatives?
One alternative would be to make the no ops updates optional. I have skimmed through the skins and did not find a point where it would be enabled. If we find a point, we can add it.
What do you think?
There was a problem hiding this comment.
If the value is not accompanied by something else like a timestamp that actually changes receivers should better not make any assumptions about how frequently an unchanged value is sent. A value should be written at least once after it has been modified. Period.
But we need to test thoroughly! The current code might contain bugs and hidden dependencies that require repeated updates for not getting stuck in an inconsistent state.
There was a problem hiding this comment.
Since this is just influencing the behavior of the CO in the skin and not the CO in general, it should be fine, but this discrepancy/optimization needs to be documented so others don't trip over this when they notice that some CO works differently in Skins and Controllermappings.
There was a problem hiding this comment.
Good idea. I have add a sentence to https://github.com/mixxxdj/mixxx/wiki/Creating-Skins.
| vParameter.convert(m_property.type()); | ||
| if (m_propertyValue == vParameter) { | ||
| // don't repeat writing the same value that may stall the GUI | ||
| // Comparing the property value directly does not work, because | ||
| // in some cases (e.g. visible) it has to be propagated to the child widgets. | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
If the value is not accompanied by something else like a timestamp that actually changes receivers should better not make any assumptions about how frequently an unchanged value is sent. A value should be written at least once after it has been modified. Period.
But we need to test thoroughly! The current code might contain bugs and hidden dependencies that require repeated updates for not getting stuck in an inconsistent state.
|
Done. |
daschuer
force-pushed
the
ControlWidgetPropertyConnection
branch
from
9a4dce4 to
2c20cc7
Compare
| QVariant property = pWidget->property(m_propertyName.constData()); | ||
| if (property.type() == QVariant::Bool) { | ||
| parameter = getControlParameterForValue(v) > 0; | ||
| double parameter = getControlParameterForValue(v); |
There was a problem hiding this comment.
| double parameter = getControlParameterForValue(v); | |
| const double parameter = getControlParameterForValue(v); |
| VERIFY_OR_DEBUG_ASSERT(meta) { | ||
| return QMetaProperty(); | ||
| } | ||
| int id = meta->indexOfProperty(name.toLatin1().constData()); |
There was a problem hiding this comment.
| int id = meta->indexOfProperty(name.toLatin1().constData()); | |
| const int id = meta->indexOfProperty(name.toLatin1().constData()); |
| DEBUG_ASSERT(success); | ||
| Q_UNUSED(success); |
There was a problem hiding this comment.
What if success is false? Should we return early?
|
done. |
6 participants
This should hopefully fix the root cause for the GUI lag using high res rate slider reported here:
https://mixxx.discourse.group/t/mixtrack-pro-3-tempo-slider-lag-on-w10/21835/18
The original fix in is still relevant for an improved API.
#3804
@NotYourAverageAl
Please test this fix as well.