Skip to content

Conversation

@add-uos
Copy link
Contributor

@add-uos add-uos commented Sep 10, 2025

Added validation for write permissions before file operations. The code now checks if the target directory exists and is writable, issuing a warning when write access is denied.

Log: Improve file save error handling
Bug: https://pms.uniontech.com/bug-view-332349.html

Added validation for write permissions before file operations. The code
now checks if the target directory exists and is writable, issuing a
warning when write access is denied.

Log: Improve file save error handling
Bug: https://pms.uniontech.com/bug-view-332349.html
@deepin-ci-robot
Copy link

deepin pr auto review

我对这段代码进行审查,并提出以下改进意见:

  1. 代码逻辑和安全性改进:
  • 添加了目录写入权限检查,这是个很好的改进,可以避免因权限不足导致的程序崩溃
  • 建议在检查目录可写性之前,先检查目录是否存在,这样可以避免不必要的检查
  1. 代码质量改进:
  • 建议将目录权限检查封装成一个单独的函数,提高代码复用性和可读性
  • 错误信息建议使用更具体的错误码或枚举类型,而不是简单的字符串输出
  • 建议添加日志记录级别,区分警告和错误信息
  1. 性能优化:
  • currentTime的获取时机可以提前,避免在字符串拼接时重复获取
  • formatStr.toLatin1().data() 调用可以优化,避免重复转换
  1. 代码健壮性改进:
  • 建议添加对savePath和saveFileName的有效性检查
  • 建议添加异常处理机制,处理可能出现的文件操作异常
  • 建议检查目录创建是否成功,而不是仅仅检查目录是否存在

改进后的代码建议如下:

bool ScreenGrabber::quickFullScreenshot()
{
    // ... 前面的代码 ...

    // 检查并创建目录
    if (!saveDir.exists()) {
        if (!saveDir.mkpath(savePath)) {
            qCritical() << "Failed to create directory: " << savePath;
            return false;
        }
    }

    // 检查目录权限
    if (!isDirectoryWritable(savePath)) {
        qCritical() << "No write permissions for directory: " << savePath;
        return false;
    }

    // 生成文件名
    QString currentTime = QDateTime::currentDateTime().toString("yyyyMMdd_hhmmss");
    saveFileName = QString("%1/%2_%3.%4")
        .arg(savePath, functionTypeStr, currentTime, formatSuffix);

    // 保存截图
    if (!screenshot.save(saveFileName, formatStr.toLatin1().constData())) {
        qCritical() << "Failed to save screenshot to: " << saveFileName;
        return false;
    }

    return true;
}

bool ScreenGrabber::isDirectoryWritable(const QString& path)
{
    QFileInfo dirInfo(path);
    if (!dirInfo.exists() || !dirInfo.isDir()) {
        return false;
    }
    return dirInfo.isWritable();
}

这些改进可以提高代码的健壮性、可维护性和安全性。

@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: add-uos, max-lvs

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

@add-uos
Copy link
Contributor Author

add-uos commented Sep 10, 2025

/merge

@deepin-bot deepin-bot bot merged commit ebf0462 into linuxdeepin:release/eagle Sep 10, 2025
7 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