Skip to content

Conversation

@deepin-mozart
Copy link
Contributor

@deepin-mozart deepin-mozart commented Nov 6, 2025

Log:
Change-Id: I381db0c3c2bb8694731691515bb5ba22b45d55bb

Summary by Sourcery

Enhancements:

  • Remove the find_program and execute_process commands for lupdate in assets/CMakeLists.txt

Log:
Change-Id: I381db0c3c2bb8694731691515bb5ba22b45d55bb
@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deepin-mozart

The full list of commands accepted by this bot can be found here.

The pull request process is described 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

@sourcery-ai
Copy link

sourcery-ai bot commented Nov 6, 2025

Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

Simplifies CMake translation step by removing all lupdate discovery and execution, leaving only lrelease for processing TS files.

Class diagram for CMake translation processing logic update

classDiagram
    class TranslationProcessor {
        - lupdate discovery and execution (removed)
        + lrelease execution
    }
Loading

Flow diagram for updated translation processing in CMake

flowchart TD
    A["Start Release Build"] --> B["Find all .ts files in translations/"]
    B --> C["For each TS file"]
    C --> D["Run lrelease on TS file"]
    D --> E["End"]
Loading

File-Level Changes

Change Details Files
Removed lupdate invocation in the CMake translation workflow
  • Deleted find_program(LUPDATE_COMMAND ...) and its search paths
  • Removed execute_process call for lupdate and related error checking
  • Eliminated status and fatal_error messages tied to lupdate
assets/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

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 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.

@deepin-ci-robot
Copy link

deepin pr auto review

我来对这个 CMakeLists.txt 的变更进行审查:

  1. 代码逻辑分析:

    • 这段代码处理 Qt 翻译文件(.ts 文件)
    • 原代码在 Release 模式下会先使用 lupdate 更新翻译文件,然后使用 lrelease 编译翻译文件
    • 新代码移除了 lupdate 相关的处理,只保留了 lrelease 步骤
  2. 改进建议:

    a) 代码质量:

    • 移除 lupdate 步骤可能会导致翻译文件过时,建议:
      1. 如果确实不需要 lupdate,应该添加注释说明原因
      2. 或者将 lupdate 步骤移到单独的脚本中,由开发者手动执行
      3. 或者添加一个 CMake 选项来控制是否执行 lupdate

    b) 错误处理:

    • lrelease 命令缺少错误处理,建议添加:
      execute_process(COMMAND lrelease ${TS_FILE}
          RESULT_VARIABLE RES
          ERROR_VARIABLE ERROR)
      if (RES)
          message(FATAL_ERROR "Failed to process ${TS_FILE}: ${ERROR}")
      endif()

    c) 代码性能:

    • 可以考虑并行处理翻译文件:
      include(ProcessorCount)
      ProcessorCount(N)
      if (N)
          set(PARALLEL_ARGS -j ${N})
      endif()
      execute_process(COMMAND lrelease ${PARALLEL_ARGS} ${TS_FILE})

    d) 代码安全:

    • lrelease 命令路径是硬编码的,建议使用 find_program 查找:
      find_program(LRELEASE_COMMAND
          NAMES
              lrelease-Qt6
              lrelease
              lrelease-qt${QT_VERSION_MAJOR}
      )
      if (NOT LRELEASE_COMMAND)
          message(FATAL_ERROR "lrelease command not found")
      endif()
  3. 建议的改进版本:

if (CMAKE_BUILD_TYPE STREQUAL "Release")
    file(GLOB TS_FILES "translations/*.ts")
    
    # 查找 lrelease 命令
    find_program(LRELEASE_COMMAND
        NAMES
            lrelease-Qt6
            lrelease
            lrelease-qt${QT_VERSION_MAJOR}
    )
    if (NOT LRELEASE_COMMAND)
        message(FATAL_ERROR "lrelease command not found")
    endif()

    # 支持并行处理
    include(ProcessorCount)
    ProcessorCount(N)
    if (N)
        set(PARALLEL_ARGS -j ${N})
    endif()

    foreach(TS_FILE IN LISTS TS_FILES)
        message(STATUS "Processing translation file: ${TS_FILE}")
        
        execute_process(COMMAND ${LRELEASE_COMMAND} ${PARALLEL_ARGS} ${TS_FILE}
            RESULT_VARIABLE RES
            ERROR_VARIABLE ERROR)
            
        if (RES)
            message(FATAL_ERROR "Failed to process ${TS_FILE}: ${ERROR}")
        endif()
    endforeach()
endif()
  1. 其他建议:
    • 考虑添加一个 CMake 选项来控制是否处理翻译文件:
      option(PROCESS_TRANSLATIONS "Process translation files" ON)
      if (PROCESS_TRANSLATIONS AND CMAKE_BUILD_TYPE STREQUAL "Release")
          # 现有的翻译处理代码
      endif()
    • 考虑将翻译文件处理作为一个单独的目标,这样可以更好地集成到构建系统中:
      add_custom_target(process_translations ALL
          COMMAND ${CMAKE_COMMAND} -P ${CMAKE_CURRENT_SOURCE_DIR}/process_translations.cmake
          DEPENDS ${TS_FILES}
      )

这些改进将使代码更加健壮、可维护,并提供更好的错误处理和性能。

@deepin-mozart
Copy link
Contributor Author

/forcemerge

@deepin-bot
Copy link

deepin-bot bot commented Nov 6, 2025

This pr force merged! (status: blocked)

@deepin-bot deepin-bot bot merged commit 2be2562 into linuxdeepin:master Nov 6, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants