fix: handle file selection when traversing to file path #3465
+9
−27
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
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Kakueeen 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 |
Contributor
deepin pr auto review我来对这段代码变更进行详细审查：
改进后的代码建议： // 定义常量
static const QString kSelectUrlQuery = QStringLiteral("selectUrl=");
void travers_prehandler::doChangeCurrentUrl(quint64 winId, const QString &mpt, const QString &subPath)
{
// ... 前面的代码保持不变 ...
QUrl url = QUrl::fromLocalFile(targetPath);
FileInfoPointer fileInfo = InfoFactory::create<FileInfo>(url, Global::kCreateFileInfoSync);
if (!fileInfo) {
qWarning() << "Failed to create FileInfo for url:" << url;
return;
}
if (fileInfo->isAttributes(FileInfo::FileIsType::kIsFile)) {
// 打开文件
dpfSignalDispatcher->publish(GlobalEventType::kOpenFiles, winId, QList<QUrl> { url });
// 获取父目录URL并设置选中查询参数
QUrl parentUrl = fileInfo->urlOf(UrlInfoType::kParentUrl);
QString encodedUrl = QUrl::toPercentEncoding(url.toString());
parentUrl.setQuery(kSelectUrlQuery + encodedUrl);
url = parentUrl;
}
dpfSignalDispatcher->publish(GlobalEventType::kChangeCurrentUrl, winId, url);
// ... 后面的代码保持不变 ...
}这些改进主要关注：
|
When traversing to a file path in SMB browser, the original implementation would incorrectly navigate to the parent directory without selecting the file. This fix ensures that when a file path is encountered during traversal, the file is properly opened and the parent directory is navigated to with the file pre-selected. The changes include: 1. Using synchronous file info creation to ensure immediate availability 2. Publishing kOpenFiles signal to open the target file 3. Setting the parent URL with a query parameter to pre-select the file 4. Maintaining the navigation to parent directory but with file selection Log: Fixed file selection issue when navigating to file paths in SMB browser Influence: 1. Test navigating to file paths in SMB shares - should open file and navigate to parent with file selected 2. Verify file opening functionality works correctly 3. Check that directory navigation still functions normally 4. Test with various file types and SMB share configurations 5. Verify query parameter handling for file selection fix: 修复遍历到文件路径时的文件选择问题 当在SMB浏览器中遍历到文件路径时，原实现会错误地导航到父目录而不选择文 件。此修复确保当在遍历过程中遇到文件路径时，文件被正确打开并且导航到父目 录时文件被预选中。 修改包括： 1. 使用同步文件信息创建以确保即时可用性 2. 发布kOpenFiles信号以打开目标文件 3. 设置父URL的查询参数以预选文件 4. 保持导航到父目录但带有文件选择功能 Log: 修复了SMB浏览器中导航到文件路径时的文件选择问题 Influence: 1. 测试在SMB共享中导航到文件路径 - 应打开文件并导航到父目录且文件被选中 2. 验证文件打开功能正常工作 3. 检查目录导航功能仍然正常 4. 测试各种文件类型和SMB共享配置 5. 验证文件选择的查询参数处理 BUG: https://pms.uniontech.com/bug-view-342967.html
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdjusts SMB browser traversal to treat file paths specially: on successful mount it navigates directly using the raw subPath, and when the target is a file it creates file info synchronously, opens the file via kOpenFiles, and navigates to the parent directory with the file pre‑selected via a query parameter. Sequence diagram for SMB file path traversal and file openingsequenceDiagram
actor User
participant SmbBrowserUI
participant travers_prehandler
participant DevMngIns
participant DFMMOUNT
participant InfoFactory
participant FileInfo
participant dpfSignalDispatcher
User->>SmbBrowserUI: NavigateToSmbPath(winId, netUrl)
SmbBrowserUI->>travers_prehandler: networkAccessPrehandler(winId, netUrl, after)
travers_prehandler->>DevMngIns: mountNetworkDeviceAsync(mountSource, callback)
DevMngIns-->>travers_prehandler: callback(ok, err, mpt)
alt mountPointAvailable
travers_prehandler->>travers_prehandler: doChangeCurrentUrl(winId, mpt, subPath, netUrl)
travers_prehandler->>InfoFactory: create_FileInfo(urlFrom(mpt + subPath), kCreateFileInfoSync)
InfoFactory-->>travers_prehandler: fileInfo
alt targetIsFile
travers_prehandler->>dpfSignalDispatcher: publish(kOpenFiles, winId, [fileUrl])
travers_prehandler->>FileInfo: urlOf(kParentUrl)
FileInfo-->>travers_prehandler: parentUrl
travers_prehandler->>travers_prehandler: parentUrl.setQuery(selectUrl=fileUrl)
travers_prehandler->>dpfSignalDispatcher: publish(kChangeCurrentUrl, winId, parentUrlWithSelection)
else targetIsDirectoryOrNoInfo
travers_prehandler->>dpfSignalDispatcher: publish(kChangeCurrentUrl, winId, dirUrl)
end
else alreadyMountedOrError
travers_prehandler->>travers_prehandler: onSmbRootMounted(mountSource, after) or handleError
end
dpfSignalDispatcher-->>SmbBrowserUI: kChangeCurrentUrl event
dpfSignalDispatcher-->>SmbBrowserUI: kOpenFiles event (when file)
SmbBrowserUI-->>User: Parent directory shown with file opened and preselected
Flow diagram for updated doChangeCurrentUrl behaviorflowchart TD
A["start doChangeCurrentUrl(winId, mpt, subPath, netUrl)"] --> B["Build targetPath from mpt and subPath"]
B --> C["Create url from targetPath"]
C --> D["InfoFactory::create FileInfo(url, kCreateFileInfoSync)"]
D --> E{fileInfo exists and isFile?}
E -- Yes --> F["publish kOpenFiles(winId, [url])"]
F --> G["parentUrl = fileInfo.urlOf(kParentUrl)"]
G --> H["parentUrl.setQuery(selectUrl=url.toString)"]
H --> I["publish kChangeCurrentUrl(winId, parentUrl)"]
E -- No --> J["publish kChangeCurrentUrl(winId, url)"]
I --> K[end]
J --> K[end]
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 - I've found 2 issues, and left some high level feedback:
- Building the query as
parentUrl.setQuery("selectUrl=" + url.toString());risks breaking when the file URL contains?,&or non-ASCII characters; consider usingQUrlQueryandaddQueryItem("selectUrl", url.toString())to ensure proper encoding and preservation of any existing query parameters. - The removal of the
recordSubPath/readSubPathlogic changes behavior whensubPathis empty (no longer restoring the last visited sub-path for a share); if this persistence is still desired in some scenarios, it may be worth explicitly deciding whether this regression is acceptable or reintroducing a scoped version of that logic. - Switching
InfoFactory::create<FileInfo>(url)to synchronous creation viaGlobal::kCreateFileInfoSyncmay introduce UI blocking on slow SMB shares; if this code runs on the UI thread, consider at least guarding for long-running calls or adding a comment/rationale where this trade-off is acceptable.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Building the query as `parentUrl.setQuery("selectUrl=" + url.toString());` risks breaking when the file URL contains `?`, `&` or non-ASCII characters; consider using `QUrlQuery` and `addQueryItem("selectUrl", url.toString())` to ensure proper encoding and preservation of any existing query parameters.
- The removal of the `recordSubPath`/`readSubPath` logic changes behavior when `subPath` is empty (no longer restoring the last visited sub-path for a share); if this persistence is still desired in some scenarios, it may be worth explicitly deciding whether this regression is acceptable or reintroducing a scoped version of that logic.
- Switching `InfoFactory::create<FileInfo>(url)` to synchronous creation via `Global::kCreateFileInfoSync` may introduce UI blocking on slow SMB shares; if this code runs on the UI thread, consider at least guarding for long-running calls or adding a comment/rationale where this trade-off is acceptable.
## Individual Comments
### Comment 1
<location> `src/plugins/filemanager/dfmplugin-smbbrowser/events/traversprehandler.cpp:97` </location>
<code_context>
- if (jumpTo.isEmpty())
- jumpTo = readSubPath(mountSource);
- doChangeCurrentUrl(winId, mpt, jumpTo, netUrl);
+ doChangeCurrentUrl(winId, mpt, subPath, netUrl);
} else if (ok || err.code == DFMMOUNT::DeviceError::kGIOErrorAlreadyMounted) {
if (isSmb) onSmbRootMounted(mountSource, after);
</code_context>
<issue_to_address>
**question (bug_risk):** Removing the persisted `subPath` fallback changes navigation semantics for callers that pass an empty `subPath`.
Previously, an empty `subPath` caused us to reuse the last recorded subPath for that `mountSource` via `readSubPath`; now we always navigate using the (possibly empty) `subPath`, so flows that relied on “remember last location on this share” will instead land at the root. Is this behavior change intentional for all call sites? If not, consider reintroducing the fallback in a more explicit/centralized way.
</issue_to_address>
### Comment 2
<location> `src/plugins/filemanager/dfmplugin-smbbrowser/events/traversprehandler.cpp:159-160` </location>
<code_context>
+ if (fileInfo && fileInfo->isAttributes(FileInfo::FileIsType::kIsFile)) {
+ dpfSignalDispatcher->publish(GlobalEventType::kOpenFiles, winId, QList<QUrl> { url });
+
+ QUrl parentUrl = fileInfo->urlOf(UrlInfoType::kParentUrl);
+ parentUrl.setQuery("selectUrl=" + url.toString());
+ url = parentUrl;
+ }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Building the `selectUrl` query parameter via string concatenation may produce incorrect or lossy URLs.
`url.toString()` is being interpolated directly into the query, which can mis-handle characters like spaces, `&`, `?`, or `#`, and it also drops any existing query on `parentUrl`. Consider using `QUrlQuery` to add a `selectUrl` parameter with proper encoding, and explicitly choose the encoding for the URL (`toString()` with a mode or `toEncoded()`) to avoid malformed URLs and unintentionally overwriting other query parameters.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
itsXuSt
approved these changes
Jan 4, 2026
Johnson-zs
approved these changes
Jan 4, 2026
Contributor
|
/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
4 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.
When traversing to a file path in SMB browser, the original
implementation would incorrectly navigate to the parent directory
without selecting the file. This fix ensures that when a file path is
encountered during traversal, the file is properly opened and the parent
directory is navigated to with the file pre-selected.
The changes include:
selection
Log: Fixed file selection issue when navigating to file paths in SMB
browser
Influence:
navigate to parent with file selected
fix: 修复遍历到文件路径时的文件选择问题
当在SMB浏览器中遍历到文件路径时，原实现会错误地导航到父目录而不选择文
件。此修复确保当在遍历过程中遇到文件路径时，文件被正确打开并且导航到父目
录时文件被预选中。
修改包括：
Log: 修复了SMB浏览器中导航到文件路径时的文件选择问题
Influence:
BUG: https://pms.uniontech.com/bug-view-342967.html
Summary by Sourcery
Fix SMB browser traversal so navigating to a file path opens the file and moves to its parent directory with the file pre-selected.
Bug Fixes:
Enhancements: