fix: fix user cannot unlock after lockscreen #674
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The use of org.freedesktop.login1.Session User property is incorrect, what we exactly need is the Name property. Correct this. Btw, trying to mark already logged in logind sessions as logged-in in treeland is incorrect, remove this part of code.
Reviewer's guide (collapsed on small PRs)Reviewer's GuideCorrects how the greeter identifies users from logind sessions and simplifies session state handling, ensuring user login state updates use the session Name property and are dispatched back to the GUI thread without pre-marking existing sessions as logged-in. Sequence diagram for updated user login state handlingsequenceDiagram
participant Logind as LogindSession
participant GreeterProxy as GreeterProxy
participant ThreadPool as QThreadPool
participant SessionIF as Login1SessionInterface
participant GuiThread as GuiThread
participant UserModel as UserModel
Logind ->> GreeterProxy: SessionNew/SessionRemoved
GreeterProxy ->> ThreadPool: start(updateUserLoginState path, loggedIn)
ThreadPool ->> SessionIF: create(path)
ThreadPool ->> SessionIF: name()
SessionIF -->> ThreadPool: username
ThreadPool ->> GuiThread: QMetaObject::invokeMethod(username, loggedIn)
GuiThread ->> GreeterProxy: lambda(username, loggedIn)
GreeterProxy ->> UserModel: updateUserLoginState(username, loggedIn)
GreeterProxy ->> GreeterProxy: updateLocketState()
Class diagram for updated GreeterProxy session handlingclassDiagram
class GreeterProxy {
+GreeterProxy()
+void init()
+void login(QString user, QString password, int sessionIndex)
+void error()
+void updateUserLoginState(QDBusObjectPath path, bool loggedIn)
+UserModel* userModel()
+void updateLocketState()
}
class UserModel {
+UserModel()
+void updateUserLoginState(QString username, bool loggedIn)
}
class OrgFreedesktopLogin1SessionInterface {
+OrgFreedesktopLogin1SessionInterface(QString serviceName, QString path, QDBusConnection connection)
+QString name()
}
class QThreadPool {
+static QThreadPool* globalInstance()
+void start(std::function<void()> task)
}
class QMetaObject {
+static bool invokeMethod(QObject* object, std::function<void()> method)
}
GreeterProxy --> UserModel : uses
GreeterProxy --> OrgFreedesktopLogin1SessionInterface : queries_session_name
GreeterProxy --> QThreadPool : schedules_background_task
GreeterProxy --> QMetaObject : dispatches_to_gui_thread
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The lambda passed to QThreadPool::globalInstance()->start still captures
thiswithout any lifetime guard; consider using aQPointer<GreeterProxy>or moving the DBus call to the main thread to avoid accessing a destroyed object. - When switching to
session.name()as the username source, it may be worth handling the case wherename()is empty or the DBus call fails, so thatupdateUserLoginStatedoesn’t silently act on an invalid username.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The lambda passed to QThreadPool::globalInstance()->start still captures `this` without any lifetime guard; consider using a `QPointer<GreeterProxy>` or moving the DBus call to the main thread to avoid accessing a destroyed object.
- When switching to `session.name()` as the username source, it may be worth handling the case where `name()` is empty or the DBus call fails, so that `updateUserLoginState` doesn’t silently act on an invalid username.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical bug preventing users from unlocking the screen after lockscreen. The fix corrects the logind session property usage and simplifies session tracking logic.
- Replaces incorrect
Userproperty with correctNameproperty when retrieving username from logind sessions - Removes redundant eager initialization code that incorrectly marked pre-existing sessions as logged-in
- Simplifies asynchronous UI updates by replacing DThreadUtils with QMetaObject::invokeMethod
| void GreeterProxy::updateUserLoginState(const QDBusObjectPath &path, bool loggedIn) | ||
| { | ||
| QThreadPool ::globalInstance()->start([this, path, loggedIn] { | ||
| QThreadPool::globalInstance()->start([this, path, loggedIn] { |
There was a problem hiding this comment.
Missing include for QThreadPool. The code uses QThreadPool::globalInstance() but does not include the necessary header. Add #include <QThreadPool> to the includes section to ensure proper compilation.
缺少 QThreadPool 的头文件包含。代码使用了 QThreadPool::globalInstance() 但没有包含必要的头文件。请在包含部分添加 #include <QThreadPool> 以确保正确编译。
| QDBusConnection ::systemBus()); | ||
| QString username = QString::fromLocal8Bit(getpwuid(session.user().userId)->pw_name); | ||
| DThreadUtils::gui().run(this, [this, username, loggedIn]() { | ||
| QString username = session.name(); |
There was a problem hiding this comment.
Missing error handling for DBus property access. The call to session.name() may fail or return an empty/invalid value if the DBus session object is invalid or the Name property cannot be retrieved. Consider checking if the returned username is valid before invoking the GUI update, or handle potential DBus errors to prevent unexpected behavior.
缺少对 DBus 属性访问的错误处理。如果 DBus 会话对象无效或无法检索 Name 属性，对 session.name() 的调用可能会失败或返回空/无效值。建议在调用 GUI 更新之前检查返回的用户名是否有效，或处理潜在的 DBus 错误以防止意外行为。
Suggested change
| QString username = session.name(); | |
| if (!session.isValid()) { | |
| qCWarning(treelandGreeter) << "Failed to create logind session interface for path" | |
| << path.path(); | |
| return; | |
| } | |
| const QString username = session.name(); | |
| if (username.isEmpty()) { | |
| qCWarning(treelandGreeter) << "Received empty username from logind session for path" | |
| << path.path(); | |
| return; | |
| } |
Comment on lines
294
to
302
| QThreadPool::globalInstance()->start([this, path, loggedIn] { | ||
| OrgFreedesktopLogin1SessionInterface session(Logind::serviceName(), | ||
| path.path(), | ||
| QDBusConnection ::systemBus()); | ||
| QString username = QString::fromLocal8Bit(getpwuid(session.user().userId)->pw_name); | ||
| DThreadUtils::gui().run(this, [this, username, loggedIn]() { | ||
| QString username = session.name(); | ||
| QMetaObject::invokeMethod(this, [this, username, loggedIn]() { | ||
| userModel()->updateUserLoginState(username, loggedIn); | ||
| updateLocketState(); | ||
| }); |
There was a problem hiding this comment.
Potential lifetime issue with lambda capture. The lambda captures this in both the thread pool task and the inner QMetaObject::invokeMethod call. If the GreeterProxy object is destroyed before these lambdas execute, accessing this will lead to undefined behavior. Consider using QPointer or adding lifetime guards to ensure the object is still valid when the callbacks execute.
Lambda 捕获可能存在生命周期问题。Lambda 在线程池任务和内部的 QMetaObject::invokeMethod 调用中都捕获了 this。如果 GreeterProxy 对象在这些 lambda 执行之前被销毁，访问 this 将导致未定义行为。建议使用 QPointer 或添加生命周期保护以确保回调执行时对象仍然有效。
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The use of org.freedesktop.login1.Session User property is incorrect, what we exactly need is the Name property. Correct this.
Btw, trying to mark already logged in logind sessions as logged-in in treeland is incorrect, remove this part of code.
Summary by Sourcery
Correct user session handling with logind to fix unlock issues after lockscreen.
Bug Fixes:
Enhancements: