add UAB file thumbnail support #3389
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
1. Added UAB MIME type definition and thumbnail generation support 2. Implemented UAB thumbnail extraction from linglong.icon ELF section using readelf 3. Enhanced MIME type support system with strategy-based checking 4. Added UAB to unconditional thumbnail support category (like AppImage) 5. Created utility function to skip thumbnail frame for specific file types 6. Improved file validation logic in thumbnail creators 7. Fixed PPTX MIME type checking to include parent types Log: Added thumbnail support for UAB files Influence: 1. Test UAB file thumbnail generation with valid files containing linglong.icon section 2. Verify UAB files without linglong.icon section show no thumbnail 3. Check that UAB files skip thumbnail frame display like AppImage 4. Test thumbnail generation with different UAB file sizes 5. Verify PPTX thumbnail generation still works correctly 6. Test that existing thumbnail functionality for other file types remains unchanged 7. Check thumbnail support settings in preferences affect UAB files appropriately feat: 添加 UAB 文件缩略图支持 1. 添加 UAB MIME 类型定义和缩略图生成支持 2. 实现从 UAB 文件的 linglong.icon ELF 节提取缩略图，使用 readelf 工具 3. 增强 MIME 类型支持系统，采用基于策略的检查机制 4. 将 UAB 添加到无条件缩略图支持类别（与 AppImage 类似） 5. 创建工具函数用于跳过特定文件类型的缩略图底板 6. 改进缩略图生成器中的文件验证逻辑 7. 修复 PPTX MIME 类型检查以包含父类型 Log: 新增 UAB 文件缩略图支持功能 Influence: 1. 测试包含 linglong.icon 节的 UAB 文件缩略图生成 2. 验证没有 linglong.icon 节的 UAB 文件不显示缩略图 3. 检查 UAB 文件是否像 AppImage 一样跳过缩略图底板显示 4. 测试不同大小 UAB 文件的缩略图生成 5. 验证 PPTX 缩略图生成功能仍然正常工作 6. 测试其他文件类型的现有缩略图功能是否保持不变 7. 检查首选项中的缩略图支持设置是否正确影响 UAB 文件
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Johnson-zs The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Reviewer's GuideAdds thumbnail generation and UI handling for UAB application packages, refactors MIME-type based thumbnail support logic into a strategy map, and makes minor robustness/behavior tweaks (AppImage/PPTX checks, detail view URL validation, and group text font formatting). Sequence diagram for generating and displaying UAB file thumbnailssequenceDiagram
actor User
participant View as FileView
participant Delegate as BaseItemDelegate
participant Helper as ThumbnailHelper
participant Factory as ThumbnailFactory
participant Creators as ThumbnailCreators
participant IconUtils
participant Readelf as QProcess_readelf
participant UabFile as QFile_uab
User ->> View: Open folder containing UAB file
View ->> Delegate: data(index)
Note over Delegate: Decide whether to use thumbnail icon
Delegate ->> Helper: canGenerateThumbnail(fileUrl)
Helper ->> Helper: checkMimeTypeSupport(fileMimeType)
Helper ->> Helper: evaluateStrategy(strategy)
Helper -->> Delegate: canGenerateThumbnail = true
Note over View,Factory: Thumbnail generation for UAB mime type
View ->> Factory: requestThumbnail(fileUrl, size)
Factory ->> Creators: uabThumbnailCreator(filePath, size)
Note over Creators: Validate file and mime type
Creators ->> Creators: InfoFactory::create FileInfo
Creators -->> Creators: verify exists and mimeType == kTypeAppUab
Note over Creators,Readelf: Locate linglong.icon ELF section
Creators ->> Readelf: start(readelf, [-W, -S, filePath])
Readelf -->> Creators: stdout(section headers), stderr, exitCode
Creators ->> Creators: parse linglong.icon line
Creators -->> Creators: sectionOffset, sectionSize
Note over Creators,UabFile: Read icon data from UAB file
Creators ->> UabFile: open(ReadOnly)
Creators ->> UabFile: seek(sectionOffset)
Creators ->> UabFile: read(sectionSize)
UabFile -->> Creators: iconData
Creators ->> UabFile: close()
Creators ->> Creators: icon.loadFromData(iconData)
Creators ->> Creators: scale to ThumbnailSize if needed
Creators -->> Factory: QImage icon
Factory -->> View: QImage icon
Note over Delegate,IconUtils: Decide thumbnail frame for UAB
Delegate ->> IconUtils: shouldSkipThumbnailFrame(mimeType)
IconUtils -->> Delegate: true (for kTypeAppUab)
Delegate ->> View: paint icon without thumbnail frame
View ->> User: Show UAB application icon thumbnail
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In
uabThumbnailCreator, the secondif (!info || info->nameOf(...))condition is redundant because!infohas already been handled above; simplifying this to just check the MIME type will make the logic clearer. - The
readelf-based implementation inuabThumbnailCreatorassumes thereadelfbinary is available on PATH and parses its human-readable output; consider handling the case wherereadelfis missing and making the parsing more robust to format changes (or using a library/format more directly if available). - Running
readelfsynchronously with a 5-second timeout for every UAB thumbnail generation could become a performance bottleneck when browsing directories with many UAB files; consider reducing the timeout or making this extraction asynchronous/cached to avoid UI stalls.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `uabThumbnailCreator`, the second `if (!info || info->nameOf(...))` condition is redundant because `!info` has already been handled above; simplifying this to just check the MIME type will make the logic clearer.
- The `readelf`-based implementation in `uabThumbnailCreator` assumes the `readelf` binary is available on PATH and parses its human-readable output; consider handling the case where `readelf` is missing and making the parsing more robust to format changes (or using a library/format more directly if available).
- Running `readelf` synchronously with a 5-second timeout for every UAB thumbnail generation could become a performance bottleneck when browsing directories with many UAB files; consider reducing the timeout or making this extraction asynchronous/cached to avoid UI stalls.
## Individual Comments
### Comment 1
<location> `src/dfm-base/utils/thumbnail/thumbnailcreators.cpp:721-722` </location>
<code_context>
+ << "bytes of icon data from UAB file:" << filePath;
+
+ // 6. Load image directly from extracted data (no temporary file needed)
+ QImage icon;
+ if (!icon.loadFromData(iconData)) {
+ qCWarning(logDFMBase) << "thumbnail: failed to decode image from extracted icon data for:" << filePath;
+ return QImage();
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Potentially large `sectionSize` is used directly for `read` without a sanity check.
Since `sectionSize` ultimately comes from an untrusted file, a malformed or malicious input could advertise an extremely large value, causing excessive allocation for `iconData`. Please enforce a reasonable upper bound on `sectionSize` (e.g., aligned with existing thumbnail size limits) before allocating/reading to prevent potential DoS via memory exhaustion.
Suggested implementation:
```cpp
// Sanity check: section size should already have been validated before allocating/reading iconData.
// If not, bail out here as a secondary safeguard.
if (sectionSize <= 0) {
qCWarning(logDFMBase) << "thumbnail: invalid icon section size:" << sectionSize
<< "bytes for:" << filePath;
return QImage();
}
if (iconData.size() != sectionSize) {
qCWarning(logDFMBase) << "thumbnail: incomplete section data read - expected:" << sectionSize
<< "bytes, got:" << iconData.size() << "bytes for:" << filePath;
return QImage();
}
qCDebug(logDFMBase) << "thumbnail: successfully extracted" << iconData.size()
<< "bytes of icon data from UAB file:" << filePath;
// 6. Load image directly from extracted data (no temporary file needed)
QImage icon;
if (!icon.loadFromData(iconData)) {
qCWarning(logDFMBase) << "thumbnail: failed to decode image from extracted icon data for:" << filePath;
return QImage();
}
return icon;
// 2. Check if file is AppImage type and has executable permission
```
To fully implement the requested protection against large `sectionSize` leading to excessive allocation, you should also:
1. **Introduce a max icon section size constant** in this translation unit (preferably near the top, in the same anonymous namespace where other thumbnail-related constants likely live):
```c++
namespace {
// ... existing constants ...
// Maximum number of bytes allowed for an icon section extracted from a UAB.
// Adjust this value to align with your existing thumbnail size limits.
static constexpr qint64 kMaxUabIconSectionSize = 16 * 1024 * 1024; // 16 MiB
}
```
2. **Enforce the upper bound *before* allocating/reading `iconData`**. Locate the code where `iconData` is allocated and filled from the UAB file; it will look roughly like this (names may differ in your codebase):
```c++
QByteArray iconData;
iconData.resize(sectionSize);
const qint64 bytesRead = uabFile.read(iconData.data(), sectionSize);
```
and update it to:
```c++
if (sectionSize <= 0 || sectionSize > kMaxUabIconSectionSize) {
qCWarning(logDFMBase) << "thumbnail: icon section size out of bounds - size:" << sectionSize
<< "bytes for:" << filePath;
return QImage();
}
QByteArray iconData;
iconData.resize(sectionSize);
const qint64 bytesRead = uabFile.read(iconData.data(), sectionSize);
```
3. **Remove or adjust any duplicated sanity checks**:
- Once the `sectionSize` upper bound check is in place *before* allocation, the secondary `sectionSize <= 0` check I added in the visible snippet can be either kept as a defensive assertion-like guard or simplified.
- Ensure you don’t have conflicting or redundant checks for `sectionSize` elsewhere in the function.
These additional changes ensure that no large allocation for `iconData` is attempted when a malicious or malformed UAB advertises an excessive `sectionSize`, fulfilling your DoS/memory-exhaustion concern.
</issue_to_address>
### Comment 2
<location> `src/dfm-base/utils/thumbnail/thumbnailhelper.cpp:34` </location>
<code_context>
ThumbnailHelper::ThumbnailHelper()
{
+ initMimeTypeSupport();
+}
+
</code_context>
<issue_to_address>
**issue (complexity):** Consider replacing the SupportCheckStrategy enum with a direct MimeSupportRule map from MIME types to either an unconditional flag or the specific Application::GenericAttribute to check, simplifying initialization and evaluation logic.
You can keep the centralized initialization and wildcard behavior, but remove a whole level of indirection by mapping MIME keys directly to “what to check” instead of to a `SupportCheckStrategy` enum.
### 1. Replace `SupportCheckStrategy` with a direct rule map
Instead of:
```cpp
QHash<QString, SupportCheckStrategy> mimeTypeSupportStrategy;
```
use a small rule struct that encodes either “unconditional” or “check this attribute”:
```cpp
struct MimeSupportRule {
bool unconditional = false;
Application::GenericAttribute attr = Application::kPreviewImage; // dummy default
};
QHash<QString, MimeSupportRule> mimeSupportRules;
```
Then `initMimeTypeSupport()` becomes:
```cpp
void ThumbnailHelper::initMimeTypeSupport()
{
// === Image types ===
mimeSupportRules.insert("image/*", { false, Application::kPreviewImage });
// === Audio types ===
mimeSupportRules.insert("audio/*", { false, Application::kPreviewAudio });
// === Video types ===
mimeSupportRules.insert("video/*", { false, Application::kPreviewVideo });
// === Text types ===
mimeSupportRules.insert(Mime::kTypeTextPlain, { false, Application::kPreviewTextFile });
// === Document types ===
mimeSupportRules.insert(Mime::kTypeAppPdf, { false, Application::kPreviewDocumentFile });
mimeSupportRules.insert(Mime::kTypeAppCRRMedia,{ false, Application::kPreviewDocumentFile });
mimeSupportRules.insert(Mime::kTypeAppMxf, { false, Application::kPreviewDocumentFile });
mimeSupportRules.insert(Mime::kTypeAppPptx, { false, Application::kPreviewDocumentFile });
// === Executable package types (unconditional icon display) ===
mimeSupportRules.insert(Mime::kTypeAppAppimage, { true, Application::kPreviewImage });
mimeSupportRules.insert(Mime::kTypeAppUab, { true, Application::kPreviewImage });
qCInfo(logDFMBase) << "thumbnail: initialized MIME type support rules for"
<< mimeSupportRules.size() << "types";
}
```
This keeps the same central configuration, and you no longer need `SupportCheckStrategy` or `evaluateStrategy()`.
### 2. Inline strategy evaluation and avoid the extra enum → attribute hop
`checkMimeTypeSupport()` can then resolve a single rule and evaluate it directly:
```cpp
bool ThumbnailHelper::checkMimeTypeSupport(const QMimeType &mime)
{
const QString mimeName = mime.name();
QStringList candidateTypes { mimeName };
candidateTypes.append(mime.parentMimeTypes());
// Special check: MimeTypeDisplayManager audio/video types
if (MimeTypeDisplayManager::instance()->supportAudioMimeTypes().contains(mimeName)) {
return Application::instance()
->genericAttribute(Application::kPreviewAudio).toBool();
}
if (MimeTypeDisplayManager::instance()->supportVideoMimeTypes().contains(mimeName)) {
return Application::instance()
->genericAttribute(Application::kPreviewVideo).toBool();
}
// Find matching rule (exact first, then wildcard)
const MimeSupportRule *rule = nullptr;
for (const QString &type : std::as_const(candidateTypes)) {
// 1. Exact match
auto it = mimeSupportRules.constFind(type);
if (it != mimeSupportRules.constEnd()) {
rule = &it.value();
break;
}
// 2. Wildcard match: image/*, audio/*, video/*
const int slashIndex = type.indexOf(QLatin1Char('/'));
if (slashIndex <= 0)
continue;
const QString wildcardKey = type.left(slashIndex) + QLatin1String("/*");
it = mimeSupportRules.constFind(wildcardKey);
if (it != mimeSupportRules.constEnd()) {
rule = &it.value();
break;
}
}
if (!rule)
return false;
if (rule->unconditional)
return true;
return Application::instance()->genericAttribute(rule->attr).toBool();
}
```
**Benefits:**
- Removes `SupportCheckStrategy` and `evaluateStrategy()` entirely.
- Eliminates the “MIME → enum → attribute” double mapping; the logic becomes “MIME → rule → result”.
- Keeps your centralized initialization and wildcard handling intact.
- Keeps the existing `MimeTypeDisplayManager` special cases but makes the rest of the flow linear and easier to follow.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Changed the service dependencies from dde-session-pre.target to dde- session-core.target This ensures the file manager daemon starts after the core session services are ready The change aligns with the updated session initialization sequence Influence: 1. Verify dde-file-manager-daemon starts after dde-session-core.target 2. Check that file manager functionality works correctly with new dependency order 3. Test session startup sequence and service initialization timing 4. Confirm no regressions in file management operations chore: 更新 dde-file-manager-daemon 服务依赖项 将服务依赖从 dde-session-pre.target 更改为 dde-session-core.target 确保文件管理器守护进程在核心会话服务准备就绪后启动 此更改与更新的会话初始化顺序保持一致 Influence: 1. 验证 dde-file-manager-daemon 在 dde-session-core.target 之后启动 2. 检查文件管理器功能在新的依赖顺序下正常工作 3. 测试会话启动顺序和服务初始化时序 4. 确认文件管理操作没有回归问题
Contributor
deepin pr auto review我来对这个git diff进行代码审查：
改进建议：
// 建议将readelf命令的路径定义为常量
static const QString kReadElfCommand = "readelf";
// 建议将超时时间定义为常量
static const int kReadElfTimeout = 5000; // 5 seconds
// 在使用时
readelfProcess.start(kReadElfCommand, { "-W", "-S", filePath });
if (!readelfProcess.waitForFinished(kReadElfTimeout)) {
// ...
}
// 建议将策略检查逻辑抽取为单独的函数
bool ThumbnailHelper::isSpecialMimeType(const QString &mimeName) const {
return MimeTypeDisplayManager::instance()->supportAudioMimeTypes().contains(mimeName)
|| MimeTypeDisplayManager::instance()->supportVideoMimeTypes().contains(mimeName);
}
// 建议为shouldSkipThumbnailFrame添加文档注释
/**
* @brief Checks if the thumbnail frame should be skipped for the given MIME type
* @param mimeType The MIME type to check
* @return true if the thumbnail frame should be skipped, false otherwise
*/
bool shouldSkipThumbnailFrame(const QString &mimeType);
// 在uabThumbnailCreator中添加
static const qint64 kMaxUABSize = 1024 * 1024 * 1024; // 1GB
if (info->size() > kMaxUABSize) {
qCWarning(logDFMBase) << "thumbnail: UAB file too large:" << filePath;
return QImage();
}
QString nativePath = QDir::toNativeSeparators(filePath);
// 添加对parts大小的更严格检查
if (parts.size() < 7) {
qCWarning(logDFMBase) << "thumbnail: Invalid readelf output format for:" << filePath;
continue;
}这些改进建议主要针对代码的可维护性、健壮性和性能优化。整体来说，这个diff的实现质量较高，主要需要关注的是错误处理的完善性和代码的模块化程度。 |
Contributor
Author
|
/forcemerge |
Contributor
|
This pr force merged! (status: behind) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Add thumbnail generation and UI handling for UAB application packages and refactor MIME-based thumbnail support.
New Features:
Enhancements: