broulik added a comment.
Looking fine, cannot really comment, though. Without context makes it hard to
review.
INLINE COMMENTS
> ddcutilbrightness.cpp:26
> {
> + QVector<int> m_usedVcp = {0x10};
> m_setBrightnessEventFilter.setInterval(100);
Use initializer:
DDCutilBrightness::DDCUtilBrightness()
: m_usedVcp({0x10})
> ddcutilbrightness.cpp:63
>
> - m_descrToVcp_perDisp[iDisp].insert(
> - QString(ddca_get_feature_name(vcpCode)), vcpCode);
> -
> -
> - ddca_get_feature_info_by_display(m_displayHandleList.at(iDisp),
> vcpCode, &featureInfo);
> - if (featureInfo == nullptr) {
> - continue;
> + for (int iVcp=0; iVcp<m_usedVcp.count();iVcp++) {
> + DDCA_Single_Vcp_Value *returnValue;
Coding style:
for (int iVcp = 0; iVcp < m_usedVcp.count(); ++iVcp) {
> ddcutilbrightness.cpp:65
> + DDCA_Single_Vcp_Value *returnValue;
> + rc = ddca_get_vcp_value(dh, m_usedVcp[iVcp],
> DDCA_NON_TABLE_VCP_VALUE, &returnValue);
> + if (rc == 0) {
Use `.value()` instead of `operator[]` which is const
> ddcutilbrightness.cpp:68-69
> + qCDebug(POWERDEVIL) << "[DDCutilBrightness]: This monitor
> does not seam to support " << m_usedVcp[iVcp];
> }
> - qCDebug(POWERDEVIL) <<
> featureInfo->feature_code<<":"<<featureInfo->desc;
> - if ((featureInfo->feature_flags & DDCA_SIMPLE_NC) !=
> DDCA_SIMPLE_NC) {
> - continue;
> - }
> - for (int
> iVcpVal=0;featureInfo->sl_values[iVcpVal].value_code!=0;++iVcpVal) {
> -
> - qCDebug(POWERDEVIL) <<
> "\t"<<featureInfo->sl_values[iVcpVal].value_code
> - <<":"<< featureInfo->sl_values[iVcpVal].value_name;
> -
> - bool thisVcpValIsSupported=false;
> -
> - for (int iSupportedVcpVal=0;
> iSupportedVcpVal<parsedCapabilities->vcp_codes[iVcp].value_ct;
> iSupportedVcpVal++) {
> -
> if(parsedCapabilities->vcp_codes[iVcp].values[iSupportedVcpVal]
> - ==featureInfo->sl_values[iVcpVal].value_code) {
> - thisVcpValIsSupported=true;
> - }
> - }
> -
> - if (thisVcpValIsSupported) {
> -
> (m_vcpTovcpValueWithDescr_perDisp[iDisp])[vcpCode].insert(
> - featureInfo->sl_values[iVcpVal].value_code,
> - featureInfo->sl_values[iVcpVal].value_name);
> - }
> + else {
> + m_supportedVcp_perDisp[iDisp].append(m_usedVcp[iVcp]);
Coding style: Put on same line
} else {
> ddcutilbrightness.cpp:84
> {
> -#ifndef WITH_DDCUTIL
> - return false;
> -#else
> + #if WITH_DDCUTIL
> return !m_displayHandleList.isEmpty();
Leave preprocessor directives unindented
> ddcutilbrightness.cpp:160
> + #if WITH_DDCUTIL
> + if (m_supportedVcp_perDisp.at(0).contains(0x10)) {
> + qCDebug(POWERDEVIL) << "[DDCutilBrightness::brightness]: trying to
> set brightness for monitor that does not support it";
Can `m_supportedVcp_perDisp` be empty? `.at(0)` will assert if so.
REPOSITORY
R122 Powerdevil
REVISION DETAIL
https://phabricator.kde.org/D8626
To: dvogel, broulik, davidedmundson
Cc: asturmlechner, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed,
jensreuterberg, abetts, sebas, apol, mart
