Skip to content

fix: prefix desktop filename with linyaps in uninstall cleanup#53

Merged
wjyrich merged 1 commit into
linuxdeepin:masterfrom
wjyrich:fix-bug-317501
Aug 26, 2025
Merged

fix: prefix desktop filename with linyaps in uninstall cleanup#53
wjyrich merged 1 commit into
linuxdeepin:masterfrom
wjyrich:fix-bug-317501

Conversation

@wjyrich

@wjyrich wjyrich commented Aug 22, 2025

Copy link
Copy Markdown
Contributor

Changed the postUninstallCleanUp parameter to prefix the desktop filename with "linyaps-"
This ensures proper cleanup when uninstalling applications by matching the expected format
The previous implementation used just the filename which could cause cleanup issues

fix: 在卸载清理中为桌面文件名添加 linyaps 前缀

修改 postUninstallCleanUp 参数,为桌面文件名添加 "linyaps-" 前缀
这确保了在卸载应用程序时能够正确清理,匹配预期的格式
之前的实现仅使用文件名,可能导致清理问题

Pms: BUG-317501

Summary by Sourcery

Bug Fixes:

  • Prefix desktop shortcut filename with "linyaps-" to ensure proper cleanup of the expected file format

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

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.

@sourcery-ai

sourcery-ai Bot commented Aug 22, 2025

Copy link
Copy Markdown
Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

Enhance postUninstallCleanUp to detect and remove desktop shortcut files prefixed with "linyaps-" in addition to the original filename, ensuring correct cleanup.

Class diagram for updated postUninstallCleanUp logic

classDiagram
    class postUninstallCleanUp {
        +void postUninstallCleanUp(QString desktopId)
        // Now checks and removes both <desktopId> and linyaps-<desktopId> files
    }
    class QFile {
        +bool exists()
        +bool remove()
    }
    class QStandardPaths {
        +QString writableLocation(enum locationType)
    }
    postUninstallCleanUp --> QFile
    postUninstallCleanUp --> QStandardPaths
Loading

File-Level Changes

Change Details Files
Support removal of desktop shortcuts prefixed with "linyaps-" during uninstall cleanup
  • Introduce linyapsDesktopPath variable for the prefixed filename
  • Change cleanup logic to else-if checking for the prefixed file
  • Add debug output when removing the prefixed shortcut
dbus/launcher1compat.cpp

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

@BLumia BLumia left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

按我理解这个应该让玲珑来改,他们不该加那个前缀,这个前缀和 app id / desktop id 是不一致的。

一定要按这个临时方案的话,下面是此 PR 的修改建议:

Comment thread dbus/launcher1compat.cpp Outdated
Changed the postUninstallCleanUp parameter to prefix the desktop
filename with "linyaps-"
This ensures proper cleanup when uninstalling applications by matching
the expected format
The previous implementation used just the filename which could cause
cleanup issues

fix: 在卸载清理中为桌面文件名添加 linyaps 前缀

修改 postUninstallCleanUp 参数,为桌面文件名添加 "linyaps-" 前缀
这确保了在卸载应用程序时能够正确清理,匹配预期的格式
之前的实现仅使用文件名,可能导致清理问题

Pms: BUG-317501
@deepin-ci-robot

Copy link
Copy Markdown

deepin pr auto review

代码审查意见

总体评价

这个代码补丁主要是对 launcher1compat.cpplauncher1compat.h 文件进行了修改,主要涉及对卸载后清理功能的增强,特别是增加了对玲珑包的特殊处理。整体来看,代码结构清晰,功能明确,但有一些可以改进的地方。

具体分析

1. 语法逻辑

  1. 新增的 PackageType 枚举

    • 在头文件中新增的 PackageType 枚举定义清晰,注释适当。
    • 建议在枚举定义后添加分号,这是 C++ 的标准做法。
  2. postUninstallCleanUp 函数修改

    • 函数签名从 void postUninstallCleanUp(const QString & desktopId) 改为 void postUninstallCleanUp(const QString & desktopId, PackageType packageType),增加了包类型参数。
    • 逻辑处理上增加了对玲珑包的特殊处理,这是合理的。

2. 代码质量

  1. 注释问题

    • 代码中有多处 FIXME 注释,如 // FIXME: THIS IS NOT DESktop ID// FIXME: the filename of the desktop file MIGHT NOT be its desktopId in freedesktop spec.,这些应该被修复而不是保留在代码中。
  2. 日志输出

    • 在删除文件时添加了日志输出,这是好的实践,但可以更详细一些,例如记录删除操作的原因和时间戳。
  3. 重复代码

    • 多个 uninstall* 函数中都有相似的清理和通知逻辑,可以考虑提取为公共函数。

3. 代码性能

  1. 文件操作

    • 代码中多次使用 QFile 检查文件是否存在后再删除,这是高效的。
    • 但可以考虑使用 QFileInfo 一次性检查和删除,减少系统调用。
  2. 字符串操作

    • 字符串拼接使用了 + 操作符,对于频繁操作的场景,建议使用 QString::appendQStringBuilder 提高性能。

4. 代码安全

  1. 路径处理

    • 代码中直接拼接用户桌面路径,没有进行路径规范化或安全检查,可能存在路径遍历风险。
    • 建议使用 QDir::cleanPath 或类似函数对路径进行处理。
  2. 文件权限

    • 删除文件前没有检查文件权限,可能导致删除失败。
    • 建议在删除前检查文件是否可写。

改进建议

  1. 修复 FIXME 注释

    // 当前
    // FIXME: THIS IS NOT DESKTOP ID
    
    // 建议
    // 使用完整的桌面文件路径而非文件名作为清理依据
    const QString desktopId = extractDesktopIdFromFilePath(m_desktopFilePath);
    postUninstallCleanUp(desktopId, packageType);
  2. 增强日志记录

    qDebug() << "Removing desktop shortcut:" << linyapsDesktopPath 
             << "for package:" << desktopId 
             << "at time:" << QDateTime::currentDateTime().toString(Qt::ISODate);
  3. 路径安全处理

    QString safePath = QDir::cleanPath(curDesktop + "/" + desktopId);
    if (safePath.startsWith(curDesktop)) {
        QFile file(safePath);
        // 安全操作
    }
  4. 提取公共逻辑

    void handleUninstallSuccess(const QString &pkgDisplayName, const QString &desktopFilePath, PackageType packageType) {
        QFileInfo fi(desktopFilePath);
        postUninstallCleanUp(fi.fileName(), packageType);
        emit UninstallSuccess(desktopFilePath);
        sendNotification(pkgDisplayName, true, m_base64Icon);
    }
  5. 错误处理增强

    if (!file.remove()) {
        qWarning() << "Failed to remove" << appDesktopPath << "error:" << file.errorString();
        // 可以考虑添加错误处理逻辑
    }

总结

代码整体质量良好,功能明确,但在安全性、代码复用和错误处理方面还有提升空间。建议优先处理路径安全问题和修复 FIXME 注释,然后考虑提取公共逻辑以提高代码质量。

Comment thread dbus/launcher1compat.cpp
@deepin-ci-robot

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: BLumia, wjyrich

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

@wjyrich
wjyrich merged commit f8773cd into linuxdeepin:master Aug 26, 2025
9 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.

4 participants