← Back to issue list

fix(desktop-file): verify theme icons before rewriting icon

View original Github issue

Metadata

Project
snapcraft
Number
#6374
Type
pull request
State
open
Author
soumyaDghosh
Labels
Created
Updated
Closed

Current evaluation

PR to fix desktop file icon theme warning; diff targets outdated code and doesn't address the described bug

Suggested action: close not mergeable

Reason: The PR's diff targets code that no longer exists — the strip operations were refactored into `_normalize_icon_path()` in commit f702705c6 (PR #6385). Additionally, the diff only refactors string operations and doesn't address the actual bug described in the body (checking `icon` instead of `icon_path` after theme resolution). The underlying issue (#6364) remains open.

Impact: 30 Quick Win: 25.5 Staleness: 75 Complexity: 15 Confidence: 70

Issue body

Theme-resolved icon path was written to the desktop file without checking the file exists, and the subsequent existence check ran against the bare icon name stored in `icon` variable, emitting a spurious "not found" warning. Assign the resolved path to the local var first so the same check covers theme icons. Fixes #6364 IMO, this complete function should be refactored, including early return when icon is None, converting these 2 lines into a single one liner and remove any prefixed `$SNAP` as well ```diff @@ -89,11 +89,8 @@ class DesktopFile: if icon_path is not None: icon = icon_path - # Strip any leading slash. - icon = icon[1:] if icon.startswith("/") else icon - - # Strip any leading ${SNAP}. - icon = icon[8:] if icon.startswith("${SNAP}") else icon + # Strip any leading slash, ${SNAP} and $SNAP variable + icon.removeprefix("/").removeprefix("${SNAP}").removeprefix("$SNAP") # If icon is just a name (no path separator), try to resolve it from the hicolor icon theme. if "/" not in icon: ``` --- - [x] I've followed the [contribution guidelines](https://github.com/canonical/snapcraft/blob/main/CONTRIBUTING.md). - [x] I've signed the [CLA](http://www.ubuntu.com/legal/contributors/). - [x] I've successfully run `make lint && make test`. - [x] I've added or updated any relevant documentation. - [x] In documents I changed, I [added a meta description](https://canonical-starflow.readthedocs-hosted.com/how-to/add-a-page-meta-description/) if one was missing. - [x] I've updated the relevant release notes.

Evaluation history

Date Model Scores Action Summary
qwen3.6-35b-a3b-mtp-q6
Impact: 30
Quick Win: 25.5
Staleness: 75
Complexity: 15
Confidence: 70
close not mergeable PR to fix desktop file icon theme warning; diff targets outdated code and doesn't address the described bug
qwen/qwen3.6-35b-a3b
Staleness: 5
Complexity: 10
Confidence: 90
needs review Fixes a bug where theme-resolved icon paths were written to desktop files without existence checks, causing spurious warnings. Currently awaiting maintainer review with CI mostly passing.
qwen3.6-35b-a3b-mtp-q6
Staleness: 5
Complexity: 5
Confidence: 85
needs review Fixes a bug where theme-resolved icon paths were written to desktop files without existence checks, causing spurious warnings. Currently pending maintainer review with CI passing except for a one-time CLA check.

Update history

Date Change
updated
created

Related work

  • Related To: canonical/snapcraft#6364 (confidence 95%)

    The PR claims to fix this issue, but the diff is outdated and doesn't address the root cause. Issue remains open.

  • Blocked By: canonical/snapcraft#6385 (confidence 85%)

    PR #6385 refactored desktop file handling, making this PR's diff obsolete.

Related issues

No related issues found above the similarity threshold.