fix: prevent duplicate QML import paths #1239
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 check to avoid prepending DDE_SHELL_QML_INSTALL_DIR if it already exists in import path list 2. This prevents duplicate paths and maintains correct search order for QML imports 3. The change ensures consistent behavior when environment variables add the same path 4. Fixes issue where duplicate paths could cause import resolution problems fix: 防止 QML 导入路径重复 1. 添加检查避免在导入路径列表中已存在 DDE_SHELL_QML_INSTALL_DIR 时重复 添加 2. 防止路径重复并保持 QML 导入的正确搜索顺序 3. 确保当环境变量添加相同路径时行为一致 4. 修复重复路径可能导致导入解析问题的情况
18202781743
requested review from
BLumia,
fly602,
mhduiy,
robertkill,
wjyrich and
yixinshark
deepin pr auto review这段代码是对QML引擎导入路径设置的改进，我来分析一下：
建议：
改进后的代码质量有明显提升，主要改进点在于增加了重复路径检查，使代码更加健壮和安全。这是一个很好的改进，避免了潜在的问题。 |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis patch enhances the QML engine setup by adding guard checks before inserting key import paths into the engine’s path list. It ensures DDE_SHELL_QML_INSTALL_DIR and the local plugins directory are only prepended if they’re not already present, preventing duplicate entries and preserving the intended search order. Sequence diagram for QML import path setup with duplicate preventionsequenceDiagram
participant DQmlEnginePrivate
participant QQmlEngine
participant QCoreApplication
participant QDir
DQmlEnginePrivate->>QQmlEngine: importPathList()
QQmlEngine-->>DQmlEnginePrivate: returns paths
DQmlEnginePrivate->>DQmlEnginePrivate: Check if paths contains DDE_SHELL_QML_INSTALL_DIR
alt DDE_SHELL_QML_INSTALL_DIR not in paths
DQmlEnginePrivate->>QQmlEnginePrivate: Prepend DDE_SHELL_QML_INSTALL_DIR
end
DQmlEnginePrivate->>QCoreApplication: applicationDirPath()
QCoreApplication-->>DQmlEnginePrivate: returns rootDir
DQmlEnginePrivate->>QDir: cd("../plugins")
QDir-->>DQmlEnginePrivate: success/failure
alt pluginDir.cd success
DQmlEnginePrivate->>DQmlEnginePrivate: Check if paths contains pluginDir.absolutePath()
alt pluginDir.absolutePath() not in paths
DQmlEnginePrivate->>QQmlEnginePrivate: Prepend pluginDir.absolutePath()
end
end
DQmlEnginePrivate->>QQmlEngine: setImportPathList(paths)
DQmlEnginePrivate->>DQmlEnginePrivate: qCDebug(dsLog()) << importPathList()
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
mhduiy
approved these changes
Sep 4, 2025
![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:
- Consider normalizing import paths (e.g. via QDir::cleanPath or canonicalPath) before checking contains to avoid duplicates caused by differing path formats.
- Instead of manually checking for duplicates before each prepend, consider assembling the list first and then using QStringList::removeDuplicates or a small helper to ensure uniqueness more succinctly.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider normalizing import paths (e.g. via QDir::cleanPath or canonicalPath) before checking contains to avoid duplicates caused by differing path formats.
- Instead of manually checking for duplicates before each prepend, consider assembling the list first and then using QStringList::removeDuplicates or a small helper to ensure uniqueness more succinctly.
## Individual Comments
### Comment 1
<location> `frame/qmlengine.cpp:47` </location>
<code_context>
QDir pluginDir(rootDir);
if (pluginDir.cd("../plugins")) {
- paths.prepend(pluginDir.absolutePath());
+ if (!paths.contains(pluginDir.absolutePath()))
+ paths.prepend(pluginDir.absolutePath());
}
s_engine->setImportPathList(paths);
</code_context>
<issue_to_address>
Absolute path comparison may not account for symbolic links or path normalization.
Consider using canonicalPath() or normalizing paths before comparison to prevent duplicate entries due to symbolic links or differing path formats.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
QDir pluginDir(rootDir);
if (pluginDir.cd("../plugins")) {
if (!paths.contains(pluginDir.absolutePath()))
paths.prepend(pluginDir.absolutePath());
}
=======
QDir pluginDir(rootDir);
if (pluginDir.cd("../plugins")) {
QString pluginCanonicalPath = pluginDir.canonicalPath();
QStringList canonicalPaths;
for (const QString &path : paths) {
QDir dir(path);
canonicalPaths << dir.canonicalPath();
}
if (!canonicalPaths.contains(pluginCanonicalPath))
paths.prepend(pluginCanonicalPath);
}
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
45
to
49
| QDir pluginDir(rootDir); | ||
| if (pluginDir.cd("../plugins")) { | ||
| paths.prepend(pluginDir.absolutePath()); | ||
| if (!paths.contains(pluginDir.absolutePath())) | ||
| paths.prepend(pluginDir.absolutePath()); | ||
| } |
There was a problem hiding this comment.
suggestion (bug_risk): Absolute path comparison may not account for symbolic links or path normalization.
Consider using canonicalPath() or normalizing paths before comparison to prevent duplicate entries due to symbolic links or differing path formats.
Suggested change
| QDir pluginDir(rootDir); | |
| if (pluginDir.cd("../plugins")) { | |
| paths.prepend(pluginDir.absolutePath()); | |
| if (!paths.contains(pluginDir.absolutePath())) | |
| paths.prepend(pluginDir.absolutePath()); | |
| } | |
| QDir pluginDir(rootDir); | |
| if (pluginDir.cd("../plugins")) { | |
| QString pluginCanonicalPath = pluginDir.canonicalPath(); | |
| QStringList canonicalPaths; | |
| for (const QString &path : paths) { | |
| QDir dir(path); | |
| canonicalPaths << dir.canonicalPath(); | |
| } | |
| if (!canonicalPaths.contains(pluginCanonicalPath)) | |
| paths.prepend(pluginCanonicalPath); | |
| } |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 18202781743, mhduiy 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
Author
|
/forcemerge |
|
This pr force merged! (status: blocked) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
already exists in import path list
QML imports
the same path
problems
fix: 防止 QML 导入路径重复
添加
Summary by Sourcery
Bug Fixes: