Skip to content

Conversation

@deepin-ci-robot
Copy link
Contributor

@deepin-ci-robot deepin-ci-robot commented Jul 17, 2025

Synchronize source files from linuxdeepin/dde-session-shell.

Source-pull-request: linuxdeepin/dde-session-shell#29

Summary by Sourcery

Sync session-shell changes to filter stale sessions, prevent login state crashes for custom accounts, and correct greeter installation in CMake.

Bug Fixes:

  • Prevent crash when updating login state for custom domain accounts by validating user names before insertion

Enhancements:

  • Use DBusDisplayManager to filter out stale systemd sessions when retrieving logged-in users

Build:

  • Switch deepin-greeter installation in CMakeLists.txt from FILES to PROGRAMS

Synchronize source files from linuxdeepin/dde-session-shell.

Source-pull-request: linuxdeepin/dde-session-shell#29
@sourcery-ai
Copy link

sourcery-ai bot commented Jul 17, 2025

Reviewer's Guide

This PR syncs updates from linuxdeepin/dde-session-shell, integrating a display manager–based session filtering workaround, refining user login state logic, and correcting CMake install directives for the greeter executable.

Sequence diagram for updated user login state filtering via DisplayManager

sequenceDiagram
    participant SessionBaseModel
    participant DBusDisplayManager
    participant DBusDisplayManagerSession
    actor Systemd

    SessionBaseModel->>DBusDisplayManager: Query sessions()
    loop For each session
        DBusDisplayManager->>DBusDisplayManagerSession: Get session path
        DBusDisplayManagerSession->>DBusDisplayManagerSession: Get UserName property
        alt UserName is not empty
            SessionBaseModel->>SessionBaseModel: Add to loggedUserNameList
        end
    end
    SessionBaseModel->>SessionBaseModel: Use loggedUserNameList to filter logined users
    Note right of SessionBaseModel: Only users present in DisplayManager sessions are considered logged in
    Systemd-->>SessionBaseModel: (Sessions may be stale if kwin crashes)
Loading

Class diagram for updated SessionBaseModel user login logic

classDiagram
    class SessionBaseModel {
        - m_loginedUsers: QMap
        - m_users: QMap
        + updateLoginedUserList(list: QString)
        + addUser(path: QString)
    }
    class DBusDisplayManager {
        + sessions(): QList<Session>
    }
    class DBusDisplayManagerSession {
        + path(): QString
        + property(name: QString): QVariant
    }
    SessionBaseModel --> DBusDisplayManager : uses
    DBusDisplayManager --> DBusDisplayManagerSession : manages
    SessionBaseModel --> DBusDisplayManagerSession : queries
Loading

File-Level Changes

Change Details Files
Add display manager–based session filtering in updateLoginedUserList
  • Include dconfig_helper and dbus/dbusdisplaymanager headers
  • Instantiate DBusDisplayManager to fetch active sessions
  • Filter sessions via QDBusInterface and collect valid user names
  • Log the filtered list of logged-in users
src/session-widgets/sessionbasemodel.cpp
Refine login state update logic to use filtered user names
  • Retrieve user object before insertion
  • Check if user name is empty or in filtered list before marking logged in
  • Update login state only when conditions are met
src/session-widgets/sessionbasemodel.cpp
Fix CMake install directives for deepin-greeter
  • Change install(FILES) to install(PROGRAMS) for greeter in non-snipe block
  • Change install(FILES) to install(PROGRAMS) for greeter in snipe block
CMakeLists.txt

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@deepin-ci-robot
Copy link
Contributor Author

deepin pr auto review

代码审查意见:

  1. CMakeLists.txt 文件中的 install 命令:

    • 在安装文件时,使用 install(PROGRAMS ...) 而不是 install(FILES ...) 是正确的,因为 deepin-greeter 是一个可执行文件。确保所有类似的情况都正确地使用了 PROGRAMS
  2. configs/org.deepin.dde.lock.json 文件中的 JSON 配置:

    • 添加了 "WLAN""enableShortcutForLock"value 列表中,需要确认这是否符合业务逻辑,并且是否在所有相关的地方都进行了相应的更新。
  3. files/wayland/lightdm-deepin-greeter-wayland 脚本:

    • 新增的 device_handle 函数和相关的 dbus 调用,需要确保这些调用是安全的,并且不会因为错误的输入导致程序崩溃。
    • dbus_values_get_stringsdbus_values_get_bool 函数中使用了 grep -oPawk,这些命令在处理特殊字符时可能会出现问题,建议使用更健壮的方法来解析 dbus 信息。
  4. src/session-widgets/sessionbasemodel.cpp 文件中的代码:

    • updateLoginedUserList 函数中,新增了对 DBusDisplayManager 的调用,用于获取登录的用户列表。需要确保 DBusDisplayManager 的实现是线程安全的,并且在调用 dbus 方法时不会阻塞主线程。
    • 在处理 m_users->value(path) 时,添加了对 user->name().isEmpty() 的检查,这是一个好的做法,可以避免潜在的空指针异常。
    • 在添加用户到 m_loginedUsers 时,添加了对 loggedUserNameList.contains(user->name()) 的检查,这是一个合理的逻辑,可以确保只添加实际登录的用户。

总体来说,代码的修改看起来是合理的,但是需要注意确保所有新增的 dbus 调用和文件操作都是安全的,并且不会对系统的其他部分产生负面影响。

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @deepin-ci-robot - I've reviewed your changes and they look great!

Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments

### Comment 1
<location> `src/session-widgets/sessionbasemodel.cpp:480` </location>
<code_context>
+                loggedUserNameList.append(userName);
+        }
+    }
+    qInfo(DDE_SHELL) << "Logined users from display manager: " << loggedUserNameList;

     QList<QString> loginedUsersTmp = m_loginedUsers->keys();
</code_context>

<issue_to_address>
Consider using 'Logged-in' instead of 'Logined' in log messages.

Using standard terminology like 'Logged-in' enhances clarity and maintainability.

Suggested implementation:

```cpp
    qInfo(DDE_SHELL) << "Logged-in users from display manager: " << loggedUserNameList;

    QList<QString> loggedInUsersTmp = m_loginedUsers->keys();

```

If `loginedUsersTmp` is used elsewhere in this function or file, you should also rename those occurrences to `loggedInUsersTmp` for consistency.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

loggedUserNameList.append(userName);
}
}
qInfo(DDE_SHELL) << "Logined users from display manager: " << loggedUserNameList;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick (typo): Consider using 'Logged-in' instead of 'Logined' in log messages.

Using standard terminology like 'Logged-in' enhances clarity and maintainability.

Suggested implementation:

    qInfo(DDE_SHELL) << "Logged-in users from display manager: " << loggedUserNameList;

    QList<QString> loggedInUsersTmp = m_loginedUsers->keys();

If loginedUsersTmp is used elsewhere in this function or file, you should also rename those occurrences to loggedInUsersTmp for consistency.

@deepin-ci-robot
Copy link
Contributor Author

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: deepin-ci-robot, yixinshark

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@yixinshark yixinshark merged commit 5753f0a into master Aug 14, 2025
26 of 27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants