Skip to content

Conversation

@robertkill
Copy link
Contributor

@robertkill robertkill commented Sep 11, 2025

The original implementation was re-encoding the JPEG image when saving to both background.jpg and background_in_theme.jpg files, which could cause quality degradation and performance issues. This fix adds a copyFile function to duplicate the file without re-encoding, preserving image quality and improving efficiency.

Log: Fixed background image quality issue in GRUB theme

Influence:

  1. Test that background images are properly displayed in GRUB menu
  2. Verify both background.jpg and background_in_theme.jpg files are created
  3. Check that image quality is maintained without artifacts
  4. Test with different image formats and sizes
  5. Verify the copy operation works correctly with file permissions

fix: 复制背景图片避免重复编码

原实现在保存到 background.jpg 和 background_in_theme.jpg 文件时都会重新 编码 JPEG 图像,这可能导致质量下降和性能问题。此修复添加了 copyFile 函数
来复制文件而不重新编码,保持图像质量并提高效率。

Log: 修复了 GRUB 主题中的背景图片质量问题

Influence:

  1. 测试 GRUB 菜单中背景图片是否正确显示
  2. 验证 background.jpg 和 background_in_theme.jpg 文件是否都创建成功
  3. 检查图像质量是否保持,没有出现伪影
  4. 使用不同格式和大小的图片进行测试
  5. 验证复制操作是否正确处理文件权限

PMS: BUG-315809

Summary by Sourcery

Use file copying instead of re-encoding for GRUB background images to preserve quality and improve efficiency.

Bug Fixes:

  • Prevent JPEG quality degradation by avoiding redundant re-encoding of background images.

Enhancements:

  • Introduce copyFile function to duplicate background.jpg as background_in_theme.jpg without re-encoding.

The original implementation was re-encoding the JPEG image when saving
to both background.jpg and background_in_theme.jpg files, which could
cause quality degradation and performance issues. This fix adds a
copyFile function to duplicate the file without re-encoding, preserving
image quality and improving efficiency.

Log: Fixed background image quality issue in GRUB theme

Influence:
1. Test that background images are properly displayed in GRUB menu
2. Verify both background.jpg and background_in_theme.jpg files are
created
3. Check that image quality is maintained without artifacts
4. Test with different image formats and sizes
5. Verify the copy operation works correctly with file permissions

fix: 复制背景图片避免重复编码

原实现在保存到 background.jpg 和 background_in_theme.jpg 文件时都会重新
编码 JPEG 图像,这可能导致质量下降和性能问题。此修复添加了 copyFile 函数
来复制文件而不重新编码,保持图像质量并提高效率。

Log: 修复了 GRUB 主题中的背景图片质量问题

Influence:
1. 测试 GRUB 菜单中背景图片是否正确显示
2. 验证 background.jpg 和 background_in_theme.jpg 文件是否都创建成功
3. 检查图像质量是否保持,没有出现伪影
4. 使用不同格式和大小的图片进行测试
5. 验证复制操作是否正确处理文件权限

PMS: BUG-315809
@sourcery-ai
Copy link

sourcery-ai bot commented Sep 11, 2025

Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

Refactor setBackground to preserve image quality and improve performance by copying the JPEG file instead of re-encoding it for the second output, with explicit error handling for each step.

Sequence diagram for the updated background image saving process

sequenceDiagram
    participant setBackground
    participant os
    participant logger
    participant copyFile
    setBackground->os: Stat(fallbackDir)
    os-->>setBackground: exists
    setBackground->saveJpeg: saveJpeg(bgImg, background.jpg)
    saveJpeg-->>setBackground: error or success
    alt saveJpeg error
        setBackground->logger: Fatal(err)
    else saveJpeg success
        setBackground->copyFile: copyFile(background.jpg, background_in_theme.jpg)
        copyFile-->>setBackground: error or success
        alt copyFile error
            setBackground->logger: Fatal(err)
        end
    end
Loading

Class diagram for the new copyFile function integration

classDiagram
    class setBackground {
        +setBackground(bgFile string)
    }
    class copyFile {
        +copyFile(src string, dst string) (written int64, err error)
    }
    setBackground --> copyFile: uses
    class logger {
        +Fatal(err error)
    }
    setBackground --> logger: error handling
    class saveJpeg {
        +saveJpeg(img, filename string) error
    }
    setBackground --> saveJpeg: saves JPEG
Loading

File-Level Changes

Change Details Files
Use file copying to avoid re-encoding the background image
  • Save the background image once with saveJpeg
  • Call copyFile to duplicate background.jpg as background_in_theme.jpg
  • Separate error checks and logger.Fatal calls for saveJpeg and copyFile
adjust-grub-theme/main.go

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

这段代码的修改看起来是为了优化背景图片的处理方式,但存在一些可以改进的地方:

  1. 错误处理改进:

    • 当前代码在两个地方都使用了 logger.Fatal(err),这会导致程序直接退出。更好的做法是统一错误处理,可以考虑返回错误让调用者决定如何处理。
  2. 代码结构优化:

    • backgroundFilebackgroundInThemeFile 的定义可以更靠近它们的使用位置,提高代码的可读性。
    • 可以考虑将文件路径的构建提取为一个单独的函数,提高代码的可维护性。
  3. 性能考虑:

    • 使用 copyFile 而不是重新编码是正确的做法,这确实能提高性能并避免质量损失。
    • 但应该确保 copyFile 函数本身是高效的,最好使用缓冲区来复制文件。
  4. 代码安全:

    • 文件操作前应该检查目标目录是否存在且有写入权限。
    • copyFile 函数应该检查源文件是否存在。
  5. 命名规范:

    • 变量名 bgImg 可以更具描述性,比如 backgroundImage

建议的改进代码:

func setBackground(bgFile string) error {
    // ... 其他代码保持不变 ...

    fallbackDir := getFallbackDir()
    _, err := os.Stat(fallbackDir)
    if err == nil {
        // 构建文件路径
        backgroundFile := filepath.Join(fallbackDir, "background.jpg")
        backgroundInThemeFile := filepath.Join(fallbackDir, "background_in_theme.jpg")
        
        // 保存JPEG图片
        if err := saveJpeg(bgImg, backgroundFile); err != nil {
            return fmt.Errorf("failed to save background image: %v", err)
        }
        
        // 复制文件到主题目录
        if err := copyFileWithBuffer(backgroundFile, backgroundInThemeFile); err != nil {
            return fmt.Errorf("failed to copy background image: %v", err)
        }
    }
    
    return nil
}

// 带缓冲区的文件复制函数
func copyFileWithBuffer(src, dst string) error {
    sourceFile, err := os.Open(src)
    if err != nil {
        return err
    }
    defer sourceFile.Close()

    destFile, err := os.Create(dst)
    if err != nil {
        return err
    }
    defer destFile.Close()

    // 使用4KB的缓冲区
    buf := make([]byte, 4096)
    _, err = io.CopyBuffer(destFile, sourceFile, buf)
    return err
}

这样的改进提高了代码的健壮性、可维护性和性能,同时提供了更好的错误处理机制。

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

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mhduiy, robertkill

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

@robertkill robertkill merged commit a697f8d into linuxdeepin:master Sep 11, 2025
16 of 17 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