fix: check local file before creating file info in property dialog #3531
+6
−4
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
Added local file validation before attempting to create FileInfo objects for property dialog. Previously, the code would try to create FileInfo for any URL path without checking if it's a local file, which could cause issues with non-local file paths. Now only local files are processed for property dialog display, preventing potential crashes or errors when handling remote or special URLs. Log: Fixed property dialog opening failure with non-local file paths Influence: 1. Test opening property dialog with local files - should work normally 2. Test with remote URLs (ftp, http) - should skip gracefully without errors 3. Test with invalid local paths - should show warning and skip 4. Verify that valid local files still display property dialog correctly 5. Check console logs for proper warning messages when skipping non- local files fix: 在属性对话框创建文件信息前检查是否为本地文件 添加了本地文件验证，在尝试为属性对话框创建FileInfo对象之前进行检查。之 前代码会为任何URL路径尝试创建FileInfo，而不检查是否为本地文件，这可能导 致处理远程或特殊URL时出现问题。现在只有本地文件会被处理用于属性对话框显 示，防止处理远程URL时出现潜在崩溃或错误。 Log: 修复了使用非本地文件路径时属性对话框打开失败的问题 Influence: 1. 测试使用本地文件打开属性对话框 - 应正常工作 2. 测试使用远程URL（ftp、http） - 应优雅跳过且不报错 3. 测试使用无效本地路径 - 应显示警告并跳过 4. 验证有效的本地文件仍能正确显示属性对话框 5. 检查控制台日志，确保跳过非本地文件时有正确的警告信息 Bug: https://pms.uniontech.com/bug-view-348227.html
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. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds a local-file check before creating FileInfo objects in the property dialog command parser so that only local, existing files are processed, preventing errors or crashes when handling remote or invalid URLs. Sequence diagram for property dialog file validation flowsequenceDiagram
participant User
participant CommandParser
participant QUrl
participant InfoFactory
participant FileInfo
User->>CommandParser: showPropertyDialog(paths)
loop for each path in paths
CommandParser->>QUrl: fromUserInput(path)
QUrl-->>CommandParser: url
alt url.isLocalFile is false
CommandParser->>CommandParser: skip url (no FileInfo created)
else url.isLocalFile is true
CommandParser->>QUrl: isLocalFile()
CommandParser->>InfoFactory: create_FileInfo(url)
InfoFactory-->>CommandParser: fileInfo
alt !fileInfo or !fileInfo.exists
CommandParser->>CommandParser: log warning invalid path or file not exists
CommandParser->>CommandParser: continue
else valid local file
CommandParser->>CommandParser: add url to urlList
end
end
end
CommandParser->>CommandParser: open property dialog for urlList
Class diagram for updated CommandParser property dialog logicclassDiagram
class CommandParser {
+showPropertyDialog()
-QStringList paths
-QList~QUrl~ urlList
}
class QUrl {
+fromUserInput(path)
+isLocalFile() bool
+path() QString
}
class InfoFactory {
+create_FileInfo(url) FileInfoPointer
}
class FileInfo {
+exists() bool
}
class FileInfoPointer {
+operator*() FileInfo
+operator->() FileInfo*
}
CommandParser --> QUrl : uses
CommandParser --> InfoFactory : uses
InfoFactory --> FileInfoPointer : creates
FileInfoPointer --> FileInfo : points_to
CommandParser --> FileInfo : checks_exists
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
deepin pr auto review这段代码的修改主要是增加了对 1. 语法逻辑
2. 代码质量
3. 代码性能
4. 代码安全
5. 其他建议
总结这段代码的改进是合理的，主要增加了对 URL 类型的检查，提高了代码的健壮性。建议进一步确认后续代码对非本地文件的处理逻辑，并考虑优化变量作用域和日志记录。 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The current change only guards FileInfo creation for local files but still allows non-local QUrls to fall through the loop; if the intent is to skip non-local URLs entirely for the property dialog, consider explicitly
continue-ing (and optionally logging) when!url.isLocalFile(). - The warning message is only emitted for invalid local paths now; if remote or special URLs are being skipped by design, you may want a separate log path so operators can distinguish between "non-local URL skipped" and "local path invalid or missing".
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The current change only guards FileInfo creation for local files but still allows non-local QUrls to fall through the loop; if the intent is to skip non-local URLs entirely for the property dialog, consider explicitly `continue`-ing (and optionally logging) when `!url.isLocalFile()`.
- The warning message is only emitted for invalid local paths now; if remote or special URLs are being skipped by design, you may want a separate log path so operators can distinguish between "non-local URL skipped" and "local path invalid or missing".
## Individual Comments
### Comment 1
<location> `src/apps/dde-file-manager/commandparser.cpp:229-230` </location>
<code_context>
- if (!fileInfo || !fileInfo->exists()) {
- qCWarning(logAppFileManager) << "Failed to show property dialog: invalid path or file not exists -" << path;
- continue;
+ if (url.isLocalFile()) {
+ const FileInfoPointer &fileInfo = InfoFactory::create<FileInfo>(url);
+ if (!fileInfo || !fileInfo->exists()) {
+ qCWarning(logAppFileManager) << "Failed to show property dialog: invalid path or file not exists -" << path;
+ continue;
</code_context>
<issue_to_address>
**question (bug_risk):** Non-local URLs now bypass existence validation and logging; confirm this behavior is intended.
Previously every `path` went through `InfoFactory::create<FileInfo>(url)` and an existence check, which also logged invalid/non-existent inputs (including any non-local schemes `InfoFactory` supports). With the `url.isLocalFile()` guard, non-local URLs now skip this validation and logging and go straight to `url.path()`/`urlList`, so invalid remote/virtual URLs may reach downstream logic without diagnostics. Consider adding explicit handling for non-local URLs here or clearly relying on validation later in the pipeline.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+229
to
+230
| if (url.isLocalFile()) { | ||
| const FileInfoPointer &fileInfo = InfoFactory::create<FileInfo>(url); |
There was a problem hiding this comment.
question (bug_risk): Non-local URLs now bypass existence validation and logging; confirm this behavior is intended.
Previously every path went through InfoFactory::create<FileInfo>(url) and an existence check, which also logged invalid/non-existent inputs (including any non-local schemes InfoFactory supports). With the url.isLocalFile() guard, non-local URLs now skip this validation and logging and go straight to url.path()/urlList, so invalid remote/virtual URLs may reach downstream logic without diagnostics. Consider adding explicit handling for non-local URLs here or clearly relying on validation later in the pipeline.
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.
Added local file validation before attempting to create FileInfo objects for property dialog. Previously, the code would try to create FileInfo for any URL path without checking if it's a local file, which could cause issues with non-local file paths. Now only local files are processed for property dialog display, preventing potential crashes or errors when handling remote or special URLs.
Log: Fixed property dialog opening failure with non-local file paths
Influence:
fix: 在属性对话框创建文件信息前检查是否为本地文件
添加了本地文件验证，在尝试为属性对话框创建FileInfo对象之前进行检查。之
前代码会为任何URL路径尝试创建FileInfo，而不检查是否为本地文件，这可能导
致处理远程或特殊URL时出现问题。现在只有本地文件会被处理用于属性对话框显
示，防止处理远程URL时出现潜在崩溃或错误。
Log: 修复了使用非本地文件路径时属性对话框打开失败的问题
Influence:
Bug: https://pms.uniontech.com/bug-view-348227.html
Summary by Sourcery
Bug Fixes: