Skip to content

Conversation

@Kakueeen
Copy link
Contributor

@Kakueeen Kakueeen commented Dec 17, 2025

Remove unnecessary find_program call for lrelease in Qt5 build
configuration
Use direct lrelease command instead of QT_LRELEASE_EXECUTABLE variable
Remove redundant add_custom_target for translations since QM files are
already handled as dependencies
This simplifies the build configuration and avoids potential issues with
lrelease executable detection

Influence:

  1. Verify Qt5 builds still generate translation files correctly
  2. Test that language switching works properly in the application
  3. Confirm build process completes without lrelease-related errors
  4. Check that all translation files are properly included in the final
    binary

refactor: 简化 Qt5 翻译生成流程

移除 Qt5 构建配置中不必要的 lrelease 程序查找调用
直接使用 lrelease 命令替代 QT_LRELEASE_EXECUTABLE 变量
移除冗余的 translations 自定义目标,因为 QM 文件已作为依赖项处理
这简化了构建配置并避免了 lrelease 可执行文件检测的潜在问题

Influence:

  1. 验证 Qt5 构建仍能正确生成翻译文件
  2. 测试应用程序中的语言切换功能正常工作
  3. 确认构建过程不会出现 lrelease 相关错误
  4. 检查所有翻译文件是否正确包含在最终二进制文件中

Summary by Sourcery

Simplify Qt5 translation generation by invoking lrelease directly and relying on existing QM file dependencies instead of a separate translations target.

Build:

  • Remove the explicit lrelease-qt5/lrelease program lookup in the Qt5 CMake configuration and invoke the lrelease command directly for QM generation.
  • Drop the redundant translations custom target since QM files are already tracked as build outputs and dependencies.

Remove unnecessary find_program call for lrelease in Qt5 build
configuration
Use direct lrelease command instead of QT_LRELEASE_EXECUTABLE variable
Remove redundant add_custom_target for translations since QM files are
already handled as dependencies
This simplifies the build configuration and avoids potential issues with
lrelease executable detection

Influence:
1. Verify Qt5 builds still generate translation files correctly
2. Test that language switching works properly in the application
3. Confirm build process completes without lrelease-related errors
4. Check that all translation files are properly included in the final
binary

refactor: 简化 Qt5 翻译生成流程

移除 Qt5 构建配置中不必要的 lrelease 程序查找调用
直接使用 lrelease 命令替代 QT_LRELEASE_EXECUTABLE 变量
移除冗余的 translations 自定义目标,因为 QM 文件已作为依赖项处理
这简化了构建配置并避免了 lrelease 可执行文件检测的潜在问题

Influence:
1. 验证 Qt5 构建仍能正确生成翻译文件
2. 测试应用程序中的语言切换功能正常工作
3. 确认构建过程不会出现 lrelease 相关错误
4. 检查所有翻译文件是否正确包含在最终二进制文件中
@sourcery-ai
Copy link

sourcery-ai bot commented Dec 17, 2025

Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

Refactors the Qt5 translation generation in CMake to directly invoke lrelease when producing .qm files and removes now-unnecessary build configuration, simplifying the translation build path while relying on existing targets/outputs for integration.

Flow diagram for updated Qt5 translation generation in CMake

flowchart TD
    A[Configure_project CMake] --> B{QT_VERSION_MAJOR}
    B -->|6| C[Use Qt6 built_in_translation_support]
    B -->|5| D[Qt5_manual_QM_generation]

    subgraph Qt5_translation_generation
        D --> E[Iterate_TS_FILES]
        E --> F[Compute_QM_file_path_for_each_TS]
        F --> G[add_custom_command
COMMAND lrelease TS_FILE -qm QM_FILE
DEPENDS TS_FILE]
        G --> H[Collect_QM_FILES]
        H --> I[QM_FILES_used_as_dependencies_in_other_targets]
    end

    C --> J[Build_targets]
    I --> J
    J --> K[Application_binary_with_translations]
Loading

File-Level Changes

Change Details Files
Simplify Qt5 translation generation by invoking lrelease directly and removing redundant build targets.
  • Remove discovery of QT_LRELEASE_EXECUTABLE via find_program and stop using that variable in translation rules
  • Change custom command for generating QM files to call lrelease directly with TS input and QM output
  • Remove the dedicated translations custom target that depended on generated QM files, relying instead on the QM files as build outputs/dependencies
CMakeLists.txt

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

我来帮你审查这个 CMakeLists.txt 的变更:

  1. 代码质量改进:
  • 移除了 find_program(QT_LRELEASE_EXECUTABLE NAMES lrelease-qt5 lrelease) 这一行,这可能会导致构建系统在找不到 lrelease 时失败。建议保留这个查找过程,或者至少添加错误检查。
  • 移除了 add_custom_target(translations ALL DEPENDS ${QM_FILES}),这可能会导致翻译文件不会被自动构建。建议保留这个 target,或者确保有其他方式构建翻译文件。
  1. 代码逻辑改进:
  • 直接使用 lrelease 命令可能不够健壮,因为:
    • 在不同系统上可能需要使用不同的命令名(如 lrelease-qt5)
    • 没有检查命令是否存在
    • 移除了 -compress -nounfinished -removeidentical 参数,这些参数有助于优化生成的翻译文件
  1. 建议的改进版本:
if (QT_VERSION_MAJOR MATCHES 6)
    # Qt6 的处理保持不变
else()
    # Qt5 处理翻译文件
    find_program(QT_LRELEASE_EXECUTABLE 
        NAMES lrelease-qt5 lrelease
        REQUIRED  # 确保找到命令
    )
    
    set(QM_FILES)
    foreach(TS_FILE ${TS_FILES})
        get_filename_component(TS_FILE_NAME ${TS_FILE} NAME_WE)
        set(QM_FILE "${CMAKE_CURRENT_BINARY_DIR}/${TS_FILE_NAME}.qm")
        
        # 检查源文件是否存在
        if(NOT EXISTS ${TS_FILE})
            message(WARNING "Translation source file ${TS_FILE} does not exist")
            continue()
        endif()
        
        add_custom_command(
            OUTPUT ${QM_FILE}
            COMMAND ${QT_LRELEASE_EXECUTABLE} 
                -compress 
                -nounfinished 
                -removeidentical 
                ${TS_FILE} 
                -qm ${QM_FILE}
            DEPENDS ${TS_FILE}
            COMMENT "Generating ${QM_FILE} from ${TS_FILE}"
            VERBATIM  # 确保命令正确执行
        )
        list(APPEND QM_FILES ${QM_FILE})
    endforeach()
    
    # 确保翻译文件被构建
    if(QM_FILES)
        add_custom_target(translations ALL DEPENDS ${QM_FILES})
    endif()
endif()
  1. 安全性改进:
  • 添加了 REQUIRED 标志确保 lrelease 命令存在
  • 添加了源文件存在性检查
  • 使用 VERBATIM 确保命令参数正确传递
  1. 性能改进:
  • 保留了 -compress -nounfinished -removeidentical 参数,这些参数可以:
    • 压缩生成的翻译文件
    • 跳过未完成的翻译
    • 移除重复的翻译条目
    • 最终减小翻译文件大小,提高加载速度

这些改进会使构建系统更加健壮和可靠,同时保持良好的性能。

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 - here's some feedback:

  • Directly invoking lrelease instead of using a discovered executable may break environments where only lrelease-qt5 exists or where lrelease is not on PATH; consider keeping find_program or adding a configurable override to maintain portability.
  • By removing the translations custom target, QM generation now relies solely on existing dependencies; double-check that all relevant build targets (e.g., the main executable or bundles) still depend on QM_FILES so translations are consistently generated in all build configurations.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- Directly invoking `lrelease` instead of using a discovered executable may break environments where only `lrelease-qt5` exists or where `lrelease` is not on PATH; consider keeping `find_program` or adding a configurable override to maintain portability.
- By removing the `translations` custom target, QM generation now relies solely on existing dependencies; double-check that all relevant build targets (e.g., the main executable or bundles) still depend on `QM_FILES` so translations are consistently generated in all build configurations.

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-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Kakueeen, 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

@Kakueeen
Copy link
Contributor Author

/forcemerge

@deepin-bot
Copy link
Contributor

deepin-bot bot commented Dec 17, 2025

This pr force merged! (status: unstable)

@deepin-bot deepin-bot bot merged commit d766542 into linuxdeepin:master Dec 17, 2025
16 of 18 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