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