fix: improve desktop file path resolution in content command#1615
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an issue in the Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly improves the content command by correctly resolving desktop file paths, considering both default and overlay shared directories. The introduction of the resolveDesktopFileExportPath method in OSTreeRepo and its integration into the cli.cpp ensures that desktop files are found in their actual exported locations. Additionally, the filtering logic for exported content has been refactored for better efficiency. Comprehensive unit tests have been added to cover various scenarios, ensuring the robustness of the new functionality.
| contents.append(entryPath); | ||
| } | ||
|
|
||
| // replace $LINGLONG_ROOT/layers/appid/verison/arch/module/entries to ${LINGLONG_ROOT}/entires |
There was a problem hiding this comment.
The comment here describes a general replacement, but the replaceInStrings call specifically targets entriesDir.absolutePath(). While the current implementation works, updating the comment to reflect the exact paths being replaced would improve clarity for future maintainers.
| return defaultDesktopPath; | ||
| } | ||
|
|
||
| return overlayDesktopPath; |
There was a problem hiding this comment.
The resolveDesktopFileExportPath method returns overlayDesktopPath even if neither the overlay nor the default desktop file exists. While the calling code in cli.cpp filters out non-existent paths, it might be more explicit for this method to return an empty QString or an std::optional<QString> if no existing file is found. This would clearly communicate that no valid, existing path was resolved, making the method's contract more robust for future uses.
There was a problem hiding this comment.
Pull request overview
This PR improves the content command to properly resolve desktop file paths by checking both the default and overlay shared directories. Previously, desktop files from the layer entries were treated as regular files without considering their actual exported location. The PR also improves the filtering loop to avoid an index-shifting bug in the original removeAt approach.
Changes:
- Added
resolveDesktopFileExportPathmethod toOSTreeRepothat checks both default and overlay shared directories for desktop files - Modified the
contentcommand inClito use this new method for.desktopfiles, and refactored the export filtering to use a separate list instead of in-place removal - Added unit tests for both default-preferred and overlay-fallback desktop file resolution scenarios
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
libs/linglong/src/linglong/repo/ostree_repo.h |
Declared the new resolveDesktopFileExportPath public method |
libs/linglong/src/linglong/repo/ostree_repo.cpp |
Implemented resolveDesktopFileExportPath to resolve desktop files from default or overlay shared dirs |
libs/linglong/src/linglong/cli/cli.cpp |
Updated content() to resolve desktop file paths and improved export filtering logic |
libs/linglong/tests/ll-tests/src/linglong/cli/cli_test.cpp |
Added tests for default-preferred and overlay-fallback desktop file resolution, plus new mock methods |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| using ::testing::ElementsAre; | ||
| using ::testing::IsEmpty; | ||
| using ::testing::Return; | ||
| using ::testing::UnorderedElementsAre; |
| if (overlayDesktopPath != defaultDesktopPath && QFileInfo::exists(overlayDesktopPath)) { | ||
| return overlayDesktopPath; | ||
| } | ||
|
|
||
| if (QFileInfo::exists(defaultDesktopPath)) { | ||
| return defaultDesktopPath; | ||
| } | ||
|
|
dengbo11
force-pushed
the
fix-content
branch
2 times, most recently
from
902350f to
bf74fe1
Compare
|
pre-commit.ci autofix |
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
dengbo11
force-pushed
the
fix-content
branch
2 times, most recently
from
bd19f3a to
3185f8f
Compare
Enhanced the content command to properly resolve desktop file paths by checking both default and overlay shared directories. Previously, desktop files were always treated as regular files without considering the actual exported locations. Key changes: 1. Added resolveDesktopFileExportPath method to OSTreeRepo that checks both default and overlay directories 2. Modified content command to use this method for desktop files while keeping regular files unchanged 3. Improved content filtering logic to only show existing exported files 4. Added comprehensive unit tests covering both default and overlay directory scenarios The fix ensures that desktop files are correctly resolved to their actual exported locations, preventing issues where desktop files might not be found if they exist in overlay directories but not in default directories. Influence: 1. Test content command with applications that have desktop files in default shared directory 2. Test content command with applications that have desktop files only in overlay directory 3. Verify both cases work correctly and show proper paths 4. Test with applications that have no desktop files to ensure regular file handling remains unchanged 5. Verify the output only includes existing files after path resolution
|
pre-commit.ci autofix |
dengbo11
force-pushed
the
fix-content
branch
2 times, most recently
from
00ff6f5 to
9eecaa1
Compare
|
pre-commit.ci autofix |
Replace all uses of mkdtemp() with the TempDir utility class across multiple test files. This change improves test reliability by ensuring proper cleanup of temporary directories and provides better error handling. The TempDir class automatically handles directory creation and cleanup, reducing the risk of leftover test artifacts and making test code more maintainable. Key changes: 1. Replace mkdtemp calls with std::make_unique<TempDir> in application singleton test 2. Update builder mock to use TempDir for OSTree repository 3. Refactor layer packager test to use TempDir for layer directories 4. Improve UAB file test with proper TempDir management 5. Update file utility tests to use TempDir for source and destination 6. Enhance package info handler test with TempDir for temporary files Influence: 1. Verify all tests still pass with the new TempDir implementation 2. Check that temporary directories are properly cleaned up after tests 3. Test directory creation and validation in each test case 4. Verify no leftover temporary directories after test execution 5. Confirm error handling works correctly when directory creation fails
deepin pr auto review这段代码主要涉及对应用层导出路径（entries）的处理逻辑，特别是针对 以下是对代码的详细审查和改进意见： 1. 语法逻辑与代码质量文件：
文件：
文件：
2. 代码性能
3. 代码安全
4. 测试代码改进文件：
文件：
总结这段代码的主要改动是对应用内容导出逻辑的重构和测试代码的资源管理优化。
具体修改建议： |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dengbo11, myml 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 |
Enhanced the content command to properly resolve desktop file paths by checking both default and overlay shared directories. Previously, desktop files were always treated as regular files without considering the actual exported locations.
Key changes:
The fix ensures that desktop files are correctly resolved to their actual exported locations, preventing issues where desktop files might not be found if they exist in overlay directories but not in default directories.
Influence: