Skip to content

Conversation

@re2zero
Copy link
Contributor

@re2zero re2zero commented Jan 21, 2026

Replace custom Document API approach with pdftocairo process for better
print quality and to resolve character encoding issues. The new method
converts Linearized PDFs to embedded vector PDF format before printing.

remove excessive qCDebug/qCInfo calls in CentralDocPage,
DocSheet and AttrScrollWidget that were likely used for
debugging purposes but are not needed in production code
Replace custom Document API approach with pdftocairo process for better
print quality and to resolve character encoding issues. The new method
converts Linearized PDFs to embedded vector PDF format before printing.

Log: fix pdf printing issue.
Bug: https://pms.uniontech.com/bug-view-348017.html
@deepin-ci-robot
Copy link

deepin pr auto review

这段代码的修改主要涉及三个方面:日志输出的移除、打印逻辑的重构以及事件过滤器的日志调整。以下是针对这三个方面的详细审查意见:

1. 日志输出调整 (CentralDocPage.cpp & AttrScrollWidget.cpp)

修改内容
qCInfoqCDebug 日志输出注释掉。

审查意见

  • 代码质量良好。在 isFullScreen()eventFilter() 这种高频调用的函数中保留日志输出会导致严重的性能问题(频繁的 I/O 操作和字符串格式化),并迅速填满日志文件。将其注释掉是合理的优化。
  • 改进建议
    • 调试开关:如果这些日志对于调试特定问题至关重要,建议不要直接注释掉,而是利用 Qt 的分类日志机制,通过环境变量(如 QT_LOGGING_RULES)在运行时控制特定分类(如 appLog)的开关。例如,默认设为 appLog.debug=false,需要时开启。这样代码中保留日志,但生产环境不输出。
    • 说明:建议在提交注释日志的代码时,在 Commit Message 中明确说明是为了性能优化移除了高频日志。

2. 打印逻辑重构 (DocSheet.cpp)

修改内容
将原本使用 DocumentFactory 加载文档检查 Linearized 属性的逻辑,改为直接调用外部命令 pdftocairo 将 PDF 转换为矢量 PDF 用于打印。

审查意见

  • 语法逻辑
    • 通过。代码逻辑清晰:定义路径 -> 启动进程 -> 等待结束 -> 检查状态 -> 设置打印路径。
  • 代码性能
    • 潜在风险process.waitForFinished() 是一个阻塞调用。如果 PDF 文件很大,转换过程可能需要几秒甚至更久,这会导致主界面(UI线程)完全卡死,用户无法进行任何操作,体验极差。
    • 改进建议:建议将 pdftocairo 的执行放到子线程中(例如使用 QThreadQtConcurrent::run),或者利用 QProcess 的异步信号槽机制(started, finished),在转换完成后再调用打印预览。
  • 代码安全
    • 严重隐患
      1. 命令注入风险:虽然 process.start("pdftocairo", QStringList() << ...) 的写法避免了直接拼接 shell 命令字符串,相对安全,但如果 pdfPathtmpPdfPath 包含特殊字符或不可控的路径,可能仍会导致意外行为。
      2. 依赖性:代码强依赖系统安装了 pdftocairo 工具(通常属于 poppler-utils 包)。如果用户系统未安装该工具,打印功能将直接失效且没有友好的提示。
      3. 临时文件清理:生成的 tmpPdfPath 是一个临时文件,代码中没有看到明确的删除逻辑。长时间运行可能会堆积大量垃圾文件。
  • 代码质量
    • 改进建议
      1. 路径拼接QString tmpPdfPath = convertedFileDir() + "/pdftocairoPrint.pdf"; 这种硬编码文件名的方式不安全。如果多个打印任务同时触发,或者程序非正常退出后重启,会导致文件冲突。建议使用 QTemporaryFile 生成唯一的临时文件名。
      2. 错误处理:目前的错误处理仅记录了日志并 return。建议给用户弹出一个提示框(如 DMessageBox::warning),告知用户打印转换失败的原因。

综合改进建议代码示例 (针对 DocSheet.cpp 打印部分)

为了解决性能、安全和文件名冲突问题,建议修改如下:

#include <QTemporaryFile>
#include <QProcess>
#include <QThread>

// ... 在 onPopPrintDialog 函数中 ...

    if (Dr::PDF == fileType()) {
        QString pdfPath = filePath();
        
        // 1. 使用 QTemporaryFile 自动管理唯一的临时文件名和生命周期
        // 注意:QTemporaryFile 创建后文件名即为空,需要 open() 一下,或者使用 template 构造
        QTemporaryFile tmpFile;
        tmpFile.setAutoRemove(false); // 我们需要把路径传给打印组件,暂时不能让Qt自动删
        if (!tmpFile.open()) {
            qCWarning(appLog) << "Failed to create temp file for print conversion.";
            return;
        }
        QString tmpPdfPath = tmpFile.fileName(); 
        tmpFile.close(); // 关闭句柄,只保留路径,pdftocairo 会覆盖写入

        // 2. 启动进程,建议异步处理,这里仅展示同步写法的改进,实际项目中请务必异步化
        QProcess process;
        // 使用 -q 静默模式减少不必要的输出
        process.start("pdftocairo", QStringList() << "-pdf" << "-q" << pdfPath << tmpPdfPath);
        
        // 增加超时机制,防止永久阻塞(例如 30 秒)
        if (!process.waitForFinished(30000)) {
            qCWarning(appLog) << "pdftocairo timeout or crashed:" << process.errorString();
            // 可选:清理残留文件
            QFile::remove(tmpPdfPath);
            return;
        }

        if (process.exitCode() != 0) {
            qCWarning(appLog) << "pdftocairo failed:" << process.readAllStandardError();
            // 可选:清理残留文件
            QFile::remove(tmpPdfPath);
            return;
        }
        
        preview->setPrintFromPath(tmpPdfPath);
        
        // 3. 注意:这里需要确保 tmpPdfPath 在打印任务完成后被删除。
        // 这可能需要在打印结束的回调槽函数中处理,或者依赖系统临时目录清理机制。
    }

总结

这段代码移除高频日志是正确的优化,但打印逻辑的改动引入了 UI 阻塞、外部依赖和临时文件管理的问题。建议重点优化 QProcess 的调用方式(改为异步)并增加对临时文件生命周期的管理。

@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

@re2zero
Copy link
Contributor Author

re2zero commented Jan 22, 2026

/merge

@deepin-bot deepin-bot bot merged commit da254c7 into linuxdeepin:master Jan 22, 2026
6 checks passed
@re2zero re2zero deleted the bugfix branch January 22, 2026 01:09
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