D8626: DDCUtil: Improved DDCUtil support for brightness control over DDC/CI channel for supported monitors
dvogel updated this revision to Diff 55690. dvogel marked an inline comment as done. dvogel added a comment. Reverted to the "old way" of trying ddcutil compatibility of the detected displays: by trying to read the feature setting. The `ddca_get_feature_list_by_dref` function was giving inconsistent results when used right after boot... (logging out an in again seemed to help, but not always..). REPOSITORY R122 Powerdevil CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8626?vs=55605&id=55690 REVISION DETAIL https://phabricator.kde.org/D8626 AFFECTED FILES CMakeLists.txt daemon/backends/upower/ddcutilbrightness.cpp daemon/backends/upower/ddcutilbrightness.h To: dvogel, broulik, davidedmundson Cc: ngraham, asturmlechner, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D8626: DDCUtil: Improved DDCUtil support for brightness control over DDC/CI channel for supported monitors
broulik accepted this revision.
broulik added inline comments.
INLINE COMMENTS
> CMakeLists.txt:77
>
> +
> +
Stray whitespace
> ddcutilbrightness.cpp:38
> // Inquire about detected monitors.
> -DDCA_Display_Info_List * dlist = ddca_get_display_info_list();
> -qCDebug(POWERDEVIL) << "ddca_get_display_info_list() returned "<< dlist;
> +DDCA_Display_Info_List * dlist = NULL;
> +ddca_get_display_info_list2(true, &dlist);
`nullptr`
> ddcutilbrightness.cpp:71
> +
> +// if (ddca_feature_list_contains(&vcpList,
> m_usedVcp.value(iVcp))) {
> +// tmpVcpList.append(m_usedVcp.value(iVcp));
Remove commented code
> ddcutilbrightness.cpp:82
> +//we only store displays that actually support the features we want.
> +if( tmpVcpList.contains(0x10) ) {
> +qCDebug(POWERDEVIL) << "Display supports Brightness, adding
> handle to list";
Coding style: `if (...) {`
REPOSITORY
R122 Powerdevil
REVISION DETAIL
https://phabricator.kde.org/D8626
To: dvogel, broulik, davidedmundson
Cc: ngraham, asturmlechner, plasma-devel, jraleigh, GB_2, ragreen, Pitel,
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D8626: DDCUtil: Improved DDCUtil support for brightness control over DDC/CI channel for supported monitors
ngraham added a comment. @dvogel I'll land this for you after you address those review comments. REPOSITORY R122 Powerdevil REVISION DETAIL https://phabricator.kde.org/D8626 To: dvogel, broulik, davidedmundson Cc: ngraham, asturmlechner, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D8626: DDCUtil: Improved DDCUtil support for brightness control over DDC/CI channel for supported monitors
dvogel updated this revision to Diff 55854. dvogel added a comment. Applied changes suggested by @broulik. Thank you for your help. REPOSITORY R122 Powerdevil CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8626?vs=55690&id=55854 REVISION DETAIL https://phabricator.kde.org/D8626 AFFECTED FILES CMakeLists.txt backends/upower/ddcutilbrightness.cpp backends/upower/ddcutilbrightness.h To: dvogel, broulik, davidedmundson Cc: ngraham, asturmlechner, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D8626: DDCUtil: Improved DDCUtil support for brightness control over DDC/CI channel for supported monitors
This revision was automatically updated to reflect the committed changes. Closed by commit R122:cbd6d0eaa6fc: DDCUtil: Improved DDCUtil support for brightness control over DDC/CI channel… (authored by dvogel, committed by ngraham). CHANGED PRIOR TO COMMIT https://phabricator.kde.org/D8626?vs=55854&id=55871#toc REPOSITORY R122 Powerdevil CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8626?vs=55854&id=55871 REVISION DETAIL https://phabricator.kde.org/D8626 AFFECTED FILES CMakeLists.txt backends/upower/ddcutilbrightness.cpp backends/upower/ddcutilbrightness.h daemon/backends/upower/ddcutilbrightness.cpp daemon/backends/upower/ddcutilbrightness.h To: dvogel, broulik, davidedmundson Cc: ngraham, asturmlechner, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D8626: DDCUtil: Improved DDCUtil support for brightness control over DDC/CI channel for supported monitors
ngraham added a comment. Thanks @dvogel! Nice patch. May it be the second of many. :) For the next one, you might consider setting up `arc`, which makes submitting and managing patches easier for you, and reviewing and landing them easier for us. See https://community.kde.org/Infrastructure/Phabricator#Using_Arcanist_to_post_patches REPOSITORY R122 Powerdevil REVISION DETAIL https://phabricator.kde.org/D8626 To: dvogel, broulik, davidedmundson Cc: ngraham, asturmlechner, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D8626: DDCUtil: Improved DDCUtil support for brightness control over DDC/CI channel for supported monitors
broulik added a comment. There's a build failure on CI now: https://build.kde.org/view/OS%20-%20Linux/job/Administration/job/Dependency%20Build%20Plasma%20kf5-qt5%20SUSEQt5.12/lastFailedBuild/console CMake Error at daemon/backends/CMakeLists.txt:49 (add_library): No SOURCES given to target: powerdevilupowerbackend REPOSITORY R122 Powerdevil REVISION DETAIL https://phabricator.kde.org/D8626 To: dvogel, broulik, davidedmundson Cc: ngraham, asturmlechner, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D8626: DDCUtil: Improved DDCUtil support for brightness control over DDC/CI channel for supported monitors
broulik added a comment. You had the files in the wrong place, I moved them, not it builds :) REPOSITORY R122 Powerdevil REVISION DETAIL https://phabricator.kde.org/D8626 To: dvogel, broulik, davidedmundson Cc: ngraham, asturmlechner, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D8626: DDCUtil: Improved DDCUtil support for brightness control over DDC/CI channel for supported monitors
rikmills added a comment. add_compile_definitions requires cmake 3.13, so breaks build in Neon where cmake from Bionic is 3.10 REPOSITORY R122 Powerdevil REVISION DETAIL https://phabricator.kde.org/D8626 To: dvogel, broulik, davidedmundson Cc: rikmills, ngraham, asturmlechner, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
