D8626: DDCUtil: Improved DDCUtil support for brightness control

[Kai Uwe Broulik]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20180104/b71b398a/attachment-0001.html>