D5381: Add brightness control using ddcutil lib

[Kai Uwe Broulik]

broulik added a comment.


  Cool!

INLINE COMMENTS

> ddcutilbrightness.cpp:7
> +{
> +    QList<DDCA_Display_Handle> m_displayHandleList = QList<DDCA_Display_Handle>();
> +    QList<DDCA_Display_Info> m_displayInfoList = QList<DDCA_Display_Info>();

No need to explicitly initialize these, the constructor could just remain empty

> ddcutilbrightness.cpp:17
> +
> +void DDCutilBrightness::detect(){
> +    

Coding style, opening brace for functions on next line:

  void DDCUtilBrightness:detect()
  {
  ...
  }

> ddcutilbrightness.cpp:22
> +    DDCA_Display_Ref dref;
> +    DDCA_Display_Handle dh = NULL;  // initialize to avoid clang analyzer warning
> +    

Prefer `nullptr`

> ddcutilbrightness.cpp:26
> +    
> +    qCWarning(POWERDEVIL)<<"\nCheck for monitors using ddca_get_displays()...\n";
> +    // Inquire about detected monitors.

Coding style:

  qCWarning(POWERDEVIL) << "...";

Also, no need for the line breaks? Also, should rather be `qCDebug` or `qCInfo`, it's not a warning after all

> ddcutilbrightness.cpp:31
> +    qCWarning(POWERDEVIL)<<dlist->ct << "display(s) were detected";
> +    for(int iDisp=0;iDisp<dlist->ct;iDisp++)
> +    {

Coding style:

  for (int i = 0; i < ...; ++i) {

(brace on the same line for everything else, ie. `for`, `while`, `if`)

> ddcutilbrightness.cpp:37
> +        
> +        m_displayInfoList.append(dlist->info[iDisp]);
> +        

Perhaps `clear()` the list at the beginning of this method? Also, use `reserve()` if you already know how many items you're going to append to the list

> ddcutilbrightness.cpp:52-53
> +            "): "<< ddca_status_code_desc(rc);
> +        }
> +        else {
> +            char * dref_repr = ddca_repr_display_ref(dref);

Coding style, braces on the same line:

  } else {

Also, I would prefer if you returned (and or use continue in a loop) instead of nesting if after if, ie.

  if (!foo) {
      return;
  }
  if (!bar) {
      return;
  }

instead of

  if (!foo) {
      if (!bar) {
          ...

> ddcutilbrightness.cpp:73
> +                
> +                m_descrToVcp_perDisp.append(new QMap<QString, int>);
> +                m_vcpTovcpValueWithDescr_perDisp.append(new QMap<int, QMap<int, QString>*>);

You don't seem to be cleaning up those containers in the destructor (there is none). Also, I don't think you should allocate those on the heap

> ddcutilbrightness.cpp:139
> +{
> +    return (m_displayHandleList.count()!=0);
> +    

`return !m_displayHandleList.isEmpty();`

> ddcutilbrightness.h:1
> +
> +#ifndef DDCUTILBRIGHTNESS_H

Missing copyright header in this and other files

> ddcutilbrightness.h:2
> +
> +#ifndef DDCUTILBRIGHTNESS_H
> +#define DDCUTILBRIGHTNESS_H

Feel free to use `#pragma once` in PowerDevil code :)

> ddcutilbrightness.h:22
> +    
> +    QList<DDCA_Display_Handle> m_displayHandleList;
> +    QList<DDCA_Display_Info> m_displayInfoList;

Prefer `QVector` over `QList`

> powerdevilupowerbackend.cpp:217-218
> +            brightnessJob->start();
> +        }
> +        else{
> +            qCDebug(POWERDEVIL) << "Using DDCutillib";

Coding style:

  } else {

> powerdevilupowerbackend.cpp:382
>      QList<QString> controls = allControls.keys(controlType);
> -
> +    
>      if (controls.isEmpty()) {

Whitespace

> powerdevilupowerbackend.cpp:413
>      int result = 0;
> -
> +    
>      if (type == Screen) {

Whitespace

> powerdevilupowerbackend.cpp:716
>  {
> -    m_brightnessControl->setBrightness(value.toInt());
> +    if(m_brightnessControl->isSupported()){
> +        m_brightnessControl->setBrightness(value.toInt());

Coding style

  if (m_brightnessControl->isSupported()) {

REPOSITORY
  R122 Powerdevil

REVISION DETAIL
  https://phabricator.kde.org/D5381

To: dvogel, broulik
Cc: plasma-devel, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20170410/88cc58aa/attachment-0001.html>