fix: Specify plugin type for zip file handling in ArchiveManager#364
Conversation
Updated the ArchiveManager to explicitly assign the libzip plugin when adding zip files, addressing compatibility issues with the pzip plugin. Enhanced logging to include the plugin type used during the interface creation. bug: https://pms.uniontech.com/bug-view-350661.html
Reviewer's guide (collapsed on small PRs)Reviewer's GuideArchiveManager now explicitly selects the libzip plugin when appending to ZIP archives and logs the chosen plugin type when creating the archive interface, avoiding compatibility issues with the pzip plugin. Sequence diagram for ArchiveManager addFiles with explicit plugin selectionsequenceDiagram
actor Caller
participant ArchiveManager
participant EventLogUtils
participant UiTools
participant MimeDetector as determineMimeType
participant CustomMimeType
participant ArchiveInterface
Caller->>ArchiveManager: addFiles(strArchiveFullPath, fileList)
ArchiveManager->>EventLogUtils: writeLogs(eventObject)
ArchiveManager->>MimeDetector: determineMimeType(strArchiveFullPath)
MimeDetector-->>ArchiveManager: CustomMimeType
ArchiveManager->>CustomMimeType: name()
CustomMimeType-->>ArchiveManager: "application/zip" or other
alt is_zip
ArchiveManager->>ArchiveManager: eType = APT_Libzip
else not_zip
ArchiveManager->>ArchiveManager: eType = APT_Auto
end
ArchiveManager->>UiTools: createInterface(strArchiveFullPath, true, eType)
UiTools-->>ArchiveManager: m_pTempInterface: ArchiveInterface
alt interface_created
ArchiveManager->>ArchiveInterface: create AddJob and append files
else interface_not_created
ArchiveManager->>ArchiveManager: handle error
end
ArchiveManager-->>Caller: return success or failure
Class diagram for ArchiveManager plugin selection and UiTools interface creationclassDiagram
class ArchiveManager {
+bool addFiles(QString strArchiveFullPath, QList~FileInfo~ fileList)
-ArchiveInterface* m_pTempInterface
}
class UiTools {
+ArchiveInterface* createInterface(QString archiveFullPath, bool appendMode, AssignPluginType pluginType)
}
class AssignPluginType {
<<enumeration>>
+APT_Auto
+APT_Libzip
}
class CustomMimeType {
+QString name()
}
class ArchiveInterface {
+void appendFiles(QList~FileInfo~ fileList)
}
ArchiveManager --> UiTools : uses
ArchiveManager --> CustomMimeType : uses
ArchiveManager --> AssignPluginType : selects
UiTools --> AssignPluginType : configures
ArchiveManager --> ArchiveInterface : creates
UiTools --> ArchiveInterface : returns
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
deepin pr auto review这段代码的修改主要是为了解决 ZIP 文件追加文件时的兼容性问题。下面是对这段代码的详细审查和改进建议: 1. 代码逻辑审查当前逻辑: 潜在问题:
改进建议:
2. 代码质量审查优点:
改进建议:
3. 代码性能审查当前性能:
改进建议:
4. 代码安全审查潜在问题:
改进建议:
5. 改进后的代码示例bool ArchiveManager::addFiles(const QString &strArchiveFullPath, const QList<FileData> &files) {
// 日志记录
QVariantMap obj = {
{"describe", QString("Add File to package: ") + strArchiveFullPath}
};
EventLogUtils::get().writeLogs(obj);
// 安全检查
QString normalizedPath = QFileInfo(strArchiveFullPath).canonicalFilePath();
if (normalizedPath.isEmpty()) {
qWarning() << "Invalid archive path:" << strArchiveFullPath;
return false;
}
// 插件类型选择
UiTools::AssignPluginType pluginType = UiTools::APT_Auto;
CustomMimeType mimeType = determineMimeType(normalizedPath);
if (isZipFormat(mimeType)) { // 提取为独立函数
pluginType = UiTools::APT_Libzip;
}
qDebug() << "Creating interface for adding files, plugin type:" << pluginType;
m_pTempInterface = UiTools::createInterface(normalizedPath, true, pluginType);
if (m_pTempInterface) {
qDebug() << "Creating AddJob";
// ... 后续代码
}
return true;
}
// 辅助函数(可放在类的私有部分或工具类中)
bool ArchiveManager::isZipFormat(const CustomMimeType &mimeType) {
static const QLatin1String zipMime("application/zip");
return mimeType.name() == zipMime;
}6. 总结
以上改进建议可以根据实际项目需求选择性采纳。 |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The plugin selection checks only for an exact
application/zipMIME name; consider using a more robust check (e.g.,inheritsor handling common aliases) so zip-like archives are consistently routed tolibzipwhen needed. - If
determineMimeTypeis already used elsewhere for the same path, consider reusing or caching the result to avoid redundant MIME detection work when adding files.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The plugin selection checks only for an exact `application/zip` MIME name; consider using a more robust check (e.g., `inherits` or handling common aliases) so zip-like archives are consistently routed to `libzip` when needed.
- If `determineMimeType` is already used elsewhere for the same path, consider reusing or caching the result to avoid redundant MIME detection work when adding files.
## Individual Comments
### Comment 1
<location> `src/source/archivemanager/archivemanager.cpp:150-158` </location>
<code_context>
+ // workaround:
+ // pzip 仅支持新建压缩,目前先将zip 追加时显式指定使用 libzip 插件
+ UiTools::AssignPluginType eType = UiTools::APT_Auto;
+ CustomMimeType mimeType = determineMimeType(strArchiveFullPath);
+ if (mimeType.name() == QLatin1String("application/zip")) {
+ eType = UiTools::APT_Libzip;
+ }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider handling additional ZIP MIME types or using a more robust ZIP detection.
Currently only `application/zip` is considered, but ZIPs are often reported as `application/x-zip-compressed` or other aliases. To avoid missing valid ZIP archives, consider normalizing MIME types and checking a small set of known ZIP variants, or falling back to an extension-based `.zip` check when MIME detection doesn’t match exactly.
```suggestion
// workaround:
// pzip 仅支持新建压缩,目前先将zip 追加时显式指定使用 libzip 插件
UiTools::AssignPluginType eType = UiTools::APT_Auto;
CustomMimeType mimeType = determineMimeType(strArchiveFullPath);
// 兼容更多 ZIP MIME 类型,并在 MIME 识别失败时回退到扩展名判断
const QString mimeName = mimeType.name().toLower();
const bool isZipMime =
mimeName == QLatin1String("application/zip") ||
mimeName == QLatin1String("application/x-zip-compressed") ||
mimeName == QLatin1String("application/x-zip") ||
mimeName == QLatin1String("application/x-compressed-zip");
const bool isZipByExt = strArchiveFullPath.endsWith(QLatin1String(".zip"), Qt::CaseInsensitive);
if (isZipMime || isZipByExt) {
eType = UiTools::APT_Libzip;
}
qDebug() << "Creating interface for adding files, plugin type:" << eType;
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dengzhongyuan365-dev, lzwind The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/forcemerge |
Updated the ArchiveManager to explicitly assign the libzip plugin when adding zip files, addressing compatibility issues with the pzip plugin. Enhanced logging to include the plugin type used during the interface creation.
bug: https://pms.uniontech.com/bug-view-350661.html
Summary by Sourcery
Ensure zip archives use a compatible plugin when appending files and improve visibility into plugin selection during interface creation.
Bug Fixes:
Enhancements: