graesslin requested changes to this revision. graesslin added inline comments. This revision now requires changes to proceed.
INLINE COMMENTS > CMakeLists.txt:78 > TYPE RUNTIME > - PURPOSE "Needed for emergency unlock in case that the greeter is broken. > In case your distribution does not provide loginctl please contact > plasma-devel@kde.org to discuss alternatives." > + PURPOSE "Needed for emergency unlock in case that the greeter is broken." > ) I would keep this information about the contact. We want to be friendly here and it's such a touchy topic that I prefer to be very explicit that we do want to support other solutions. > CMakeLists.txt:81-87 > +find_package(ConsoleKit) > +set_package_properties(ConsoleKit PROPERTIES > + URL "http://www.freedesktop.org/wiki/Software/ConsoleKit"; > + DESCRIPTION "Framework for defining and tracking users" > + TYPE RUNTIME > + PURPOSE "Needed for emergency unlock in case that the greeter is broken." > + ) I would prefer if we only search for one of the two. What I want to not have is a situation where we get false positive warnings about missing runtime components. E.g. if your distro is using logind there is just no point in looking for ConsoleKit and it would now produce a false positive runtime warning. The same applies the other way around. Maybe we could work with add_feature_info? > CMakeLists.txt:109-111 > +if (HAVE_CONSOLEKIT) > + configure_file(ck-unlock-session.cmake > ${CMAKE_CURRENT_BINARY_DIR}/ck-unlock-session) > +endif () given that it's used for a configure check: this is not a runtime but a build time dependency. > abstractlocker.cpp:52-57 > +#if HAVE_LOGINCTL > auto text = ki18n("The screen locker is broken and unlocking is not > possible anymore.\n" > "In order to unlock switch to a virtual terminal > (e.g. Ctrl+Alt+F2),\n" > "log in and execute the command:\n\n" > "loginctl unlock-sessions\n\n" > "Afterwards switch back to the running session > (Ctrl+Alt+F%1)."); please update this part as the code got changed ;-) > abstractlocker.cpp:62 > + "log in as root and execute the command:\n\n" > + "# ck-unlock-session <session-name>\n\n" > + "The <session-name> can be obtained by running the > command:\n\n:" cant we make the bash script just do what it is supposed to do? > abstractlocker.cpp:67-70 > + "Unfortunately your installation was compiled > without support for > + " 1) loginctl\n" > + " 2) consolekit\n" > + "which could be used to unlock the session again." I would scrap that part. It's too technical and doesn't help the user. > ck-unlock-session.cmake:17 > +{ > + ${dbussend_EXECUTABLE} --system --print-reply > --dest="org.freedesktop.ConsoleKit" "/org/freedesktop/ConsoleKit/$1" > org.freedesktop.ConsoleKit.Session.Unlock > +} Is this asking for the password in some way? Or do we now install a script which allows to bypass authentication? With logind one can configure the system to always require a password. > FindConsoleKit.cmake:28 > +find_program(cklistsessions_EXECUTABLE NAMES ck-list-sessions) > +find_program(dbussend_EXECUTABLE NAMES dbus-send) > +find_package_handle_standard_args(ConsoleKit just wondering: why dbus-send and not qdbus? qdbus would be a binary our session depends on anyway. > config-kscreenlocker.h.cmake:17 > + > +#cmakedefine01 HAVE_LOGINCTL > +#cmakedefine01 HAVE_CONSOLEKIT This also turns loginctl into a build time dependency. REPOSITORY R133 KScreenLocker REVISION DETAIL https://phabricator.kde.org/D6004 To: tcberner, #freebsd, graesslin, #plasma Cc: erichameleers, plasma-devel, ZrenBot, spstarr, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, lukas
