Skip to content

Conversation

@myk1343
Copy link
Contributor

@myk1343 myk1343 commented May 15, 2025

修复关闭窗口保存提示

Bug: https://pms.uniontech.com/bug-view-315439.html
https://pms.uniontech.com/bug-view-315425.html
Log: 修复关闭窗口保存提示,与v20保持一致

Summary by Sourcery

Implement saveAllFiles to prompt saving modified files on window close and integrate it into the close event, and prevent auto-backup from running when no windows are open.

Bug Fixes:

  • Restore prompting to save unsaved files when closing a window to match expected behavior.
  • Prevent the auto-backup routine from running when there are no open windows.

Enhancements:

  • Add a saveAllFiles() method in the Window class to centralize save-on-close logic.

修复关闭窗口保存提示

Bug: https://pms.uniontech.com/bug-view-315439.html
     https://pms.uniontech.com/bug-view-315425.html
Log: 修复关闭窗口保存提示,与v20保持一致
@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: myk1343

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

@deepin-ci-robot
Copy link

deepin pr auto review

代码审查意见:

  1. StartManager::autoBackupFile函数中,新增了一个条件判断if (m_windows.isEmpty()) { return; }。这个改动是合理的,可以避免在m_windows为空时执行不必要的操作。

  2. Window::saveAllFiles函数中,使用了QMap<QString, EditWrapper *> wrappers = m_wrappers;来复制m_wrappers。这个做法虽然可以避免直接修改原始数据,但每次调用saveAllFiles时都会创建一个新的QMap对象,这可能会影响性能。建议在函数内部直接使用m_wrappers,或者将m_wrappers的引用作为参数传递给函数。

  3. Window::saveAllFiles函数中,对于每个文件保存操作,都使用了m_tabbar->setCurrentIndex(i);来切换到当前文件所在的标签页。这个操作可能会影响性能,因为每次切换标签页都需要重新布局和渲染界面。建议在保存文件之前保存当前活动的标签页索引,保存完成后恢复该索引。

  4. Window::saveAllFiles函数中,对于每个文件保存操作,都使用了QFileInfo fileInfo(filePath);来获取文件信息。这个操作可能会影响性能,因为每次获取文件信息都需要访问文件系统。建议将文件信息存储在EditWrapper对象中,避免重复访问文件系统。

  5. Window::saveAllFiles函数中,对于每个文件保存操作,都使用了if (res == 0 || res == -1) { return false; }来处理用户取消或关闭弹窗的情况。这个做法可能会导致函数提前返回,从而无法保存所有文件。建议将保存操作放在一个循环中,即使某个文件保存失败,也应该继续尝试保存其他文件。

  6. Window::saveAllFiles函数中,对于每个文件保存操作,都使用了if (res == 2) { m_tabbar->setCurrentIndex(i); ... }来处理用户选择保存的情况。这个做法可能会导致界面频繁切换,影响用户体验。建议在保存文件之前保存当前活动的标签页索引,保存完成后恢复该索引。

  7. Window::closeEvent函数中,新增了一个条件判断if (!saveAllFiles()) { backupFile(); e->ignore(); return; }。这个改动是合理的,可以确保在关闭窗口之前保存所有文件。但是,这个改动可能会导致backupFile函数被多次调用,影响性能。建议在backupFile函数中添加一个标志位,避免重复备份。

  8. Window::closeEvent函数中,新增了一个条件判断if (!saveAllFiles()) { backupFile(); e->ignore(); return; }。这个改动是合理的,可以确保在关闭窗口之前保存所有文件。但是,这个改动可能会导致backupFile函数被多次调用,影响性能。建议在backupFile函数中添加一个标志位,避免重复备份。

总体来说,代码改动是合理的,但是需要注意性能和用户体验的问题。建议在保存文件和备份文件时,尽量减少对文件系统的访问,避免频繁切换界面。

@github-actions
Copy link

  • 敏感词检查失败, 检测到1个文件存在敏感词
详情
{
    "src/widgets/window.cpp": [
        {
            "line": "    QString key = \"base/enable\";",
            "line_number": 372,
            "rule": "S106",
            "reason": "Var naming | 64f28539d9"
        }
    ]
}

@sourcery-ai
Copy link

sourcery-ai bot commented May 15, 2025

Reviewer's Guide

Introduces a centralized saveAllFiles routine in Window to prompt and persist each modified file before closing, integrates it into the closeEvent flow to allow canceling or proceeding with backups, and adds a guard in StartManager to skip backup logic when no windows are open.

File-Level Changes

Change Details Files
Add saveAllFiles to handle per-file save prompts on window close
  • Declare saveAllFiles in Window header
  • Implement iterative wrapper loop with logging
  • Prompt user to save/skip/abort for each modified file
  • Branch between draft, backup, or normal save with fallback to Save As
src/widgets/window.h
src/widgets/window.cpp
Integrate saveAllFiles into closeEvent to control backup and cancellation
  • Invoke saveAllFiles before backupFile
  • On saveAllFiles failure ignore close event and trigger backup
  • Proceed to backupFile when saveAllFiles returns success
src/widgets/window.cpp
Guard against empty window list in autoBackupFile
  • Check if m_windows is empty and return early
src/startmanager.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

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 @myk1343 - I've reviewed your changes and they look great!

Here's what I looked at during the review
  • 🟡 General issues: 3 issues found
  • 🟢 Testing: all looks good
  • 🟡 Complexity: 1 issue found
  • 🟢 Documentation: all looks good

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.

@deepin-bot
Copy link
Contributor

deepin-bot bot commented Jun 18, 2025

TAG Bot

New tag: 6.5.26
DISTRIBUTION: unstable
Suggest: synchronizing this PR through rebase #359

@deepin-bot
Copy link
Contributor

deepin-bot bot commented Jun 23, 2025

TAG Bot

New tag: 6.5.27
DISTRIBUTION: unstable
Suggest: synchronizing this PR through rebase #362

@deepin-bot
Copy link
Contributor

deepin-bot bot commented Jul 2, 2025

TAG Bot

New tag: 6.5.28
DISTRIBUTION: unstable
Suggest: synchronizing this PR through rebase #368

@deepin-bot
Copy link
Contributor

deepin-bot bot commented Jul 10, 2025

TAG Bot

New tag: 6.5.29
DISTRIBUTION: unstable
Suggest: synchronizing this PR through rebase #370

@deepin-bot
Copy link
Contributor

deepin-bot bot commented Jul 10, 2025

TAG Bot

New tag: 6.5.30
DISTRIBUTION: unstable
Suggest: synchronizing this PR through rebase #372

@deepin-bot
Copy link
Contributor

deepin-bot bot commented Jul 16, 2025

TAG Bot

New tag: 6.5.31
DISTRIBUTION: unstable
Suggest: synchronizing this PR through rebase #373

@deepin-bot
Copy link
Contributor

deepin-bot bot commented Jul 18, 2025

TAG Bot

New tag: 6.5.32
DISTRIBUTION: unstable
Suggest: synchronizing this PR through rebase #374

@deepin-bot
Copy link
Contributor

deepin-bot bot commented Aug 7, 2025

TAG Bot

New tag: 6.5.33
DISTRIBUTION: unstable
Suggest: synchronizing this PR through rebase #376

@deepin-bot
Copy link
Contributor

deepin-bot bot commented Aug 13, 2025

TAG Bot

New tag: 6.5.34
DISTRIBUTION: unstable
Suggest: synchronizing this PR through rebase #379

@deepin-bot
Copy link
Contributor

deepin-bot bot commented Aug 22, 2025

TAG Bot

New tag: 6.5.35
DISTRIBUTION: unstable
Suggest: synchronizing this PR through rebase #380

@deepin-bot
Copy link
Contributor

deepin-bot bot commented Aug 27, 2025

TAG Bot

New tag: 6.5.36
DISTRIBUTION: unstable
Suggest: synchronizing this PR through rebase #383

@deepin-bot
Copy link
Contributor

deepin-bot bot commented Sep 4, 2025

TAG Bot

New tag: 6.5.37
DISTRIBUTION: unstable
Suggest: synchronizing this PR through rebase #386

@deepin-bot
Copy link
Contributor

deepin-bot bot commented Sep 17, 2025

TAG Bot

New tag: 6.5.38
DISTRIBUTION: unstable
Suggest: synchronizing this PR through rebase #387

@deepin-bot
Copy link
Contributor

deepin-bot bot commented Oct 16, 2025

TAG Bot

New tag: 6.5.39
DISTRIBUTION: unstable
Suggest: synchronizing this PR through rebase #393

@deepin-bot
Copy link
Contributor

deepin-bot bot commented Oct 30, 2025

TAG Bot

New tag: 6.5.40
DISTRIBUTION: unstable
Suggest: synchronizing this PR through rebase #396

@deepin-bot
Copy link
Contributor

deepin-bot bot commented Nov 6, 2025

TAG Bot

New tag: 6.5.41
DISTRIBUTION: unstable
Suggest: synchronizing this PR through rebase #398

@deepin-bot
Copy link
Contributor

deepin-bot bot commented Nov 20, 2025

TAG Bot

New tag: 6.5.42
DISTRIBUTION: unstable
Suggest: synchronizing this PR through rebase #402

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.

2 participants