Skip to content

Conversation

@BLumia
Copy link
Member

@BLumia BLumia commented Dec 12, 2025

原本 menu 指针判空的位置肯定不会成立,因为 menu() 调用一定会返回一个
有效的 QMenu. 此处因而转为检查 menu 是否为空.

这个提交是在为使用 Xembed to SNI proxy 做准备,尽管不一定实际会使用
这个方案.

Summary by Sourcery

Bug Fixes:

  • Ensure right-click on system tray items falls back to the SNI context menu when the DBus-imported menu is empty instead of assuming a valid populated QMenu.

原本 menu 指针判空的位置肯定不会成立,因为 menu() 调用一定会返回一个
有效的 QMenu. 此处因而转为检查 menu 是否为空.

这个提交是在为使用 Xembed to SNI proxy 做准备,尽管不一定实际会使用
这个方案.

Log:
@BLumia BLumia requested a review from tsic404 December 12, 2025 03:05
@sourcery-ai
Copy link

sourcery-ai bot commented Dec 12, 2025

Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

Updates SNI tray right-click handling to always expect a valid QMenu from the D-Bus menu importer, and to fall back to invoking the SNI ContextMenu() method when the menu is empty instead of treating the menu pointer as nullable.

Sequence diagram for updated SNI tray right-click context menu handling

sequenceDiagram
    actor User
    participant SniTrayProtocolHandler
    participant DbusMenuImporter
    participant QMenu
    participant SniInterface

    User->>SniTrayProtocolHandler: right_click_tray_icon()
    SniTrayProtocolHandler->>DbusMenuImporter: menu()
    DbusMenuImporter-->>SniTrayProtocolHandler: QMenu_instance
    SniTrayProtocolHandler->>QMenu: isEmpty()
    alt menu_is_empty
        SniTrayProtocolHandler->>SniInterface: ContextMenu(0, 0)
        SniTrayProtocolHandler-->>User: no_local_menu_shown
    else menu_has_items
        SniTrayProtocolHandler->>QMenu: setFixedSize(sizeHint())
        SniTrayProtocolHandler->>QMenu: winId()
        SniTrayProtocolHandler-->>User: show_context_menu()
    end
Loading

File-Level Changes

Change Details Files
Adjust right-click tray menu handling to check for an empty menu and call SNI ContextMenu() as a fallback while asserting the menu pointer is always valid.
  • Replace the null-pointer check on the QMenu returned by the D-Bus menu importer with Q_CHECK_PTR to assert it is never null
  • Add a check for menu->isEmpty() and, when true, invoke m_sniInter->ContextMenu(0, 0) and exit early from the event filter
  • Retain the existing behavior of sizing and realizing the menu window when the menu is non-empty
plugins/application-tray/sniprotocolhandler.cpp

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

deepin pr auto review

我来对这段代码的修改进行审查:

  1. 语法逻辑:
  • 修改后的代码逻辑合理,增加了对菜单为空的检查
  • 使用 Q_CHECK_PTR 是一个很好的防御性编程实践,可以在调试模式下快速发现空指针问题
  • 当菜单为空时调用 ContextMenu 是一个合理的备选方案
  1. 代码质量:
  • 代码可读性良好,逻辑清晰
  • 错误处理更加完善
  • 使用 Q_CHECK_PTR 提高了代码的健壮性
  1. 代码性能:
  • 新增的 isEmpty() 检查是一个轻量级操作,不会显著影响性能
  • menu->setFixedSize(menu->sizeHint()) 的调用位置合适,在确保菜单有效后才执行
  1. 代码安全:
  • 增加了空指针检查,提高了代码安全性
  • 使用 Q_CHECK_PTR 可以在调试阶段及早发现潜在问题
  • 对空菜单的处理更加完善,避免了可能的崩溃

改进建议:

  1. 考虑在调用 ContextMenu 后添加日志记录,以便调试和追踪
  2. 可以考虑将菜单有效性检查封装成一个单独的函数,提高代码复用性
  3. 建议在 Q_CHECK_PTR 后添加错误处理逻辑,而不仅仅依赖断言

总体来说,这是一个很好的改进,提高了代码的健壮性和可靠性。修改后的代码能够更好地处理边界情况,减少了潜在的崩溃风险。

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 there - I've reviewed your changes and found some issues that need to be addressed.

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

## Individual Comments

### Comment 1
<location> `plugins/application-tray/sniprotocolhandler.cpp:233-234` </location>
<code_context>
             } else if (mouseEvent->button() == Qt::RightButton) {
                 auto menu = m_dbusMenuImporter->menu();
-                if (!menu) return false;
+                Q_CHECK_PTR(menu);
+                if (menu->isEmpty()) {
+                    m_sniInter->ContextMenu(0, 0);
+                    return false;
</code_context>

<issue_to_address>
**issue (bug_risk):** Using Q_CHECK_PTR instead of the previous null check can cause crashes in release builds.

`Q_CHECK_PTR` is a no-op in release builds, so `menu` can still be null and `menu->isEmpty()` would dereference a null pointer and crash. Keep an explicit null check (e.g. `if (!menu) return false;`) in addition to or instead of `Q_CHECK_PTR`.
</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.

Comment on lines +233 to +234
Q_CHECK_PTR(menu);
if (menu->isEmpty()) {
Copy link

Choose a reason for hiding this comment

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

issue (bug_risk): Using Q_CHECK_PTR instead of the previous null check can cause crashes in release builds.

Q_CHECK_PTR is a no-op in release builds, so menu can still be null and menu->isEmpty() would dereference a null pointer and crash. Keep an explicit null check (e.g. if (!menu) return false;) in addition to or instead of Q_CHECK_PTR.

@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: BLumia, tsic404

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

@BLumia BLumia merged commit 858edc9 into linuxdeepin:master Dec 12, 2025
9 of 10 checks passed
@BLumia BLumia deleted the sni-ctxmenu branch December 12, 2025 03:17
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