Skip to content

fix: reset m_fIsDeleting on cancel by listening to DFDeleteDialog::re…#196

Merged
deepin-bot[bot] merged 1 commit into
linuxdeepin:masterfrom
JWWTSL:master
May 14, 2026
Merged

fix: reset m_fIsDeleting on cancel by listening to DFDeleteDialog::re…#196
deepin-bot[bot] merged 1 commit into
linuxdeepin:masterfrom
JWWTSL:master

Conversation

@JWWTSL

@JWWTSL JWWTSL commented May 14, 2026

Copy link
Copy Markdown
Contributor

log: Pressing Cancel on the delete-confirmation dialog triggers reject() + close() on an already-hidden dialog, on which DDialog::closed is not reliably emitted. As a result cancelDelete() was never called and m_fIsDeleting stayed at Deleting, so the next call to delCurrentFont() was blocked at the entry gate if (m_fIsDeleting > UnDeleting) return;, making the confirmation dialog fail to appear on every subsequent right-click delete.

Connect DFDeleteDialog::rejected directly to cancelDelete() so the delete state flag is always reset on the cancel path.

pms: bug-347229

Summary by Sourcery

Bug Fixes:

  • Hook up the delete-confirmation dialog's rejection signal to the cancellation handler so subsequent delete attempts are no longer blocked after pressing Cancel.

@sourcery-ai

sourcery-ai Bot commented May 14, 2026

Copy link
Copy Markdown
Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

Connects the delete confirmation dialog's rejected signal directly to the cancel handler so the deletion state flag is always reset when the user cancels, preventing the dialog from failing to appear on subsequent delete attempts.

Sequence diagram for delete dialog cancel path fix

sequenceDiagram
    actor User
    participant DFontMgrMainWindow
    participant DFDeleteDialog

    User->>DFontMgrMainWindow: delCurrentFont(activatedByRightmenu)
    DFontMgrMainWindow->>DFDeleteDialog: new DFDeleteDialog(...)
    DFontMgrMainWindow->>DFDeleteDialog: connect(accepted, onconfirmDelDlgAccept)
    DFontMgrMainWindow->>DFDeleteDialog: connect(rejected, cancelDelete)

    User->>DFDeleteDialog: reject()
    DFDeleteDialog-->>DFontMgrMainWindow: rejected
    DFontMgrMainWindow->>DFontMgrMainWindow: cancelDelete()
Loading

File-Level Changes

Change Details Files
Ensure deletion cancel path is always executed when the delete dialog is rejected.
  • Instantiate DFDeleteDialog as before for delete confirmation
  • Connect DFDeleteDialog::accepted to the existing onconfirmDelDlgAccept slot
  • Add a new connection from DFDeleteDialog::rejected to cancelDelete so the delete state flag is reset on cancel
deepin-font-manager/views/dfontmgrmainwindow.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

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hey - I've reviewed your changes and they look great!


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.

…jected

log: Pressing Cancel on the delete-confirmation dialog triggers reject() + close()
on an already-hidden dialog, on which DDialog::closed is not reliably emitted.
As a result cancelDelete() was never called and m_fIsDeleting stayed at
Deleting, so the next call to delCurrentFont() was blocked at the entry gate
'if (m_fIsDeleting > UnDeleting) return;', making the confirmation dialog
fail to appear on every subsequent right-click delete.

Connect DFDeleteDialog::rejected directly to cancelDelete() so the delete
state flag is always reset on the cancel path.

pms: bug-347229
@deepin-ci-robot

Copy link
Copy Markdown

deepin pr auto review

你好!我是CodeGeeX,很高兴为你进行代码审查。

针对你提供的 Git Diff,本次修改非常简单:在删除字体时弹出的确认对话框中,新增了对 rejected 信号(即用户点击取消或关闭对话框)的监听,并连接到 cancelDelete 槽函数。

虽然修改意图明确,但在语法逻辑、代码质量、代码性能和代码安全方面,我有以下审查意见和改进建议:

1. 语法与逻辑

  • 信号与槽的匹配性DFDeleteDialog::rejected 是标准 QDialog 的信号,无参数。需要确保 cancelDelete() 槽函数的签名也是无参数的,否则会导致编译失败或运行时断言错误(取决于 Qt 版本和连接方式)。
  • 状态机与逻辑闭环:在删除操作中,通常会有一个“进行中”的状态(例如禁用某些UI,防止重复点击,或者标记正在删除)。accepted 触发真正的删除,rejected 触发取消。必须确保 cancelDelete 槽函数中正确地恢复了这些状态,否则用户点击取消后,UI 可能会卡在不可交互的状态。

2. 代码质量

  • 内存泄漏隐患(严重):观察上下文,confirmDelDlg 是通过 new 在堆上创建的。如果 acceptedrejected 槽函数中没有 delete this 或调用 deleteLater(),这个对话框对象就会造成内存泄漏。
    • 改进建议:为 confirmDelDlg 设置 Qt::WA_DeleteOnClose 属性,让 Qt 在对话框关闭时自动回收内存。这是 GUI 对话框最常见的最佳实践。
  • 代码对称性:既然为 rejected 添加了 connect,如果 accepted 的槽函数 onconfirmDelDlgAccept 内部没有处理对话框的销毁,那么这两个槽函数在逻辑上应该是对称的,都需要负责清理或交由属性自动清理。

3. 代码性能

  • 本次修改仅增加了一行信号槽连接,运行时性能开销几乎可以忽略不计。
  • 但结合上下文,DFDeleteDialog 是在每次用户触发删除操作时 new 出来的。如果该操作在短时间内被频繁触发,频繁的内存分配和释放可能会造成轻微的性能抖动。
    • 改进建议:如果该对话框弹出非常频繁,可以考虑将其提升为类的成员变量,进行复用(通过 hide()show() 控制),而不是每次都 newdelete。不过对于一般的确认对话框,当前做法配合 WA_DeleteOnClose 已经足够。

4. 代码安全

  • 悬空指针风险:如果对话框设置了自动销毁,但在销毁前发出的信号引发了其他对象的析构或导致主窗口的某些状态异常,可能会引发悬空指针。
    • 改进建议:使用 Qt5 推荐的函数指针方式连接信号槽(当前代码已经是这样做的,这很好),并在连接时如果涉及生命周期不确定的对象,考虑使用 QPointer 来守护对话框指针。

💡 综合改进后的代码建议

结合以上分析,建议对这段代码进行如下微调,以确保代码的健壮性和无内存泄漏:

void DFontMgrMainWindow::delCurrentFont(bool activatedByRightmenu)
{
    DFDeleteDialog *confirmDelDlg = new DFDeleteDialog(this, m_menuDelCnt, m_menuSysCnt, m_menuCurCnt > 0, this);
 
    // 设置对话框关闭时自动销毁,防止内存泄漏
    confirmDelDlg->setAttribute(Qt::WA_DeleteOnClose);

    connect(confirmDelDlg, &DFDeleteDialog::accepted, this, &DFFontMgrMainWindow::onconfirmDelDlgAccept);
    connect(confirmDelDlg, &DFDeleteDialog::rejected, this, &DFontMgrMainWindow::cancelDelete);
 
    confirmDelDlg->move(this->geometry().center() - confirmDelDlg->rect().center());
    
    // 建议使用 exec() 模态显示(如果原本就是 exec,则忽略此条)
    // 或者如果是非模态 show(),确保主窗口在对话框存在期间的交互是安全的
    confirmDelDlg->show(); 
}

补充检查清单(请对照你的 cancelDeleteonconfirmDelDlgAccept 实现):

  1. cancelDelete() 中是否解除了主窗口的防重复点击状态(如重新 enable 按钮)?
  2. onconfirmDelDlgAccept() 中是否没有手动 delete confirmDelDlg?(如果加了 WA_DeleteOnClose,千万不要再手动 delete,否则会 double free)。
  3. cancelDelete() 的签名是否为 void cancelDelete();

DFDeleteDialog *confirmDelDlg = new DFDeleteDialog(this, m_menuDelCnt, m_menuSysCnt, m_menuCurCnt > 0, this);

connect(confirmDelDlg, &DFDeleteDialog::accepted, this, &DFontMgrMainWindow::onconfirmDelDlgAccept);
connect(confirmDelDlg, &DFDeleteDialog::rejected, this, &DFontMgrMainWindow::cancelDelete);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

确定下对话框来源
原来的逻辑取消是否重复。

@deepin-ci-robot

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: JWWTSL, lzwind

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

@JWWTSL

JWWTSL commented May 14, 2026

Copy link
Copy Markdown
Contributor Author

/forcemerge

@deepin-bot

deepin-bot Bot commented May 14, 2026

Copy link
Copy Markdown
Contributor

This pr force merged! (status: unstable)

@deepin-bot deepin-bot Bot merged commit 83fbe55 into linuxdeepin:master May 14, 2026
16 of 19 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