-
Notifications
You must be signed in to change notification settings - Fork 448
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
desktop: stop renaming desktop file #5150
base: feature/desktop-files
Are you sure you want to change the base?
desktop: stop renaming desktop file #5150
Conversation
dffe5e6
to
af19add
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know what effect this will have when running a snap on a system with snapd<2.66?
As I tested before, snapd will install the file with a <snap_name>, which snapd will do here also if the desktop file names aren't explicitly mentioned before hand. It was never really an issue to fix with. |
I think I'll need to fix the spread tests too. |
af19add
to
32dea06
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## feature/desktop-files #5150 +/- ##
========================================================
Coverage ? 89.71%
========================================================
Files ? 342
Lines ? 22641
Branches ? 0
========================================================
Hits ? 20312
Misses ? 2329
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
7b7c0d5
to
2883282
Compare
With snapd 2.66, it supports custom desktop file names. This patch allows it to happen from snapcraft side.
2883282
to
99f4e85
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After discussing during the Starcraft clinic, this should be a safe change to make. The only environment that wouldn't locate these new files is unity 8.
Approving to land in a feature branch so it can be published and tested before landing on main. Landing on main will require a new PR and approvals.
target = gui_dir / f"{self._app_name}.desktop" | ||
desktop_filename = os.path.basename(self._filename) | ||
|
||
# Stop renaming the desktop file. From snapd 2.66 onwards, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this comment is very good, but I think we should drop the first sentence - it's awkward and potentially confusing to refer to something the code no longer does
# in the format of {SNAP_NAME}_{APP_NAME}.desktop | ||
# https://snapcraft.io/docs/desktop-interface | ||
target = gui_dir / desktop_filename | ||
if not (desktop_filename.endswith(".desktop")): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can replace this code with a with_suffix(".desktop")
call
>>> Path("file").with_suffix(".desktop")
PosixPath('file.desktop')
>>> Path("file.desktop").with_suffix(".desktop")
PosixPath('file.desktop')
|
||
prime_dir = Path(f"{new_dir}/meta/gui") | ||
|
||
yield prime_dir |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yield prime_dir | |
return prime_dir |
This PR has a blocker
|
With snapd 2.66, it supports custom desktop file names. This patch allows it to happen from snapcraft side.
tox run -m lint
?tox run -e test-py310
? (supported versions:py39
,py310
,py311
,py312
)