Skip to content
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

{snap,wrappers}: improve desktop file to snap app matching #14503

Merged

Conversation

ZeyadYasser
Copy link
Contributor

Now that desktop-file-ids are supported, the simple $PREFIX_$APP.desktop heuristic when looking up snap app's desktop file won't cut it.

Instead, Loop through desktop-file-ids desktop interface plug attribute in order to have deterministic output and return the first matching desktop file found if (the newly added) X-SnapAppName entry matches the app name and fallback to the simple heuristic if no matches are found.

This follows the SD170 - Custom desktop file IDs for snaps spec.

This allows associating a desktop file with a specific snap app easily.

Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>
Now that desktop-file-ids are supported, the simple "$PREFIX_$APP.desktop"
heuristic when looking up snap app's desktop file won't cut it.

Instead, Loop through desktop-file-ids desktop interface plug attribute
in order to have deterministic output and return the first matching desktop
file found if X-SnapAppName entry matches the app name and fallback to the
simple heuristic if no matches are found.

Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>
@ZeyadYasser ZeyadYasser added the Needs Samuele review Needs a review from Samuele before it can land label Sep 13, 2024
@ZeyadYasser ZeyadYasser added this to the 2.66 milestone Sep 13, 2024
Copy link
Collaborator

@pedronis pedronis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question

snap/info.go Outdated
@@ -1422,9 +1423,48 @@ func (app *AppInfo) SecurityTag() string {
return AppSecurityTag(app.Snap.InstanceName(), app.Name)
}

func snapAppNameFromDesktopFile(desktopFile string) (string, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't we use code from desktopentry for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved parsing to desktopentry, Thanks!

Copy link
Collaborator

@pedronis pedronis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you

…sktopentry package

desktopentry can now parse X-SnapInstanceName and X-SnapAppName

Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>
@ZeyadYasser ZeyadYasser force-pushed the improve-desktop-file-app-matching branch from f4d4863 to ee90032 Compare September 18, 2024 07:10
Copy link
Member

@Meulengracht Meulengracht left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a question, the desktop files are regenerated when? Is it when snapd starts? There won't be an issue with pre-existing desktop files that don't have the X-SnapAppName or X-SnapInstanceName

snap/info.go Outdated Show resolved Hide resolved
wrappers/desktop.go Outdated Show resolved Hide resolved
@ZeyadYasser
Copy link
Contributor Author

the desktop files are regenerated when? Is it when snapd starts?

Yes, desktop files are generated when snapd starts

m.ensureDesktopFilesUpdated(),

Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>
Copy link
Member

@Meulengracht Meulengracht left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you

Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>
Copy link

codecov bot commented Sep 18, 2024

Codecov Report

Attention: Patch coverage is 81.08108% with 7 lines in your changes missing coverage. Please review.

Project coverage is 78.83%. Comparing base (3492595) to head (377fc65).
Report is 159 commits behind head on master.

Files with missing lines Patch % Lines
snap/info.go 65.00% 6 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #14503      +/-   ##
==========================================
+ Coverage   78.77%   78.83%   +0.05%     
==========================================
  Files        1073     1078       +5     
  Lines      143693   145210    +1517     
==========================================
+ Hits       113191   114473    +1282     
- Misses      23401    23568     +167     
- Partials     7101     7169      +68     
Flag Coverage Δ
unittests 78.83% <81.08%> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ZeyadYasser ZeyadYasser merged commit 97f3ec6 into canonical:master Sep 19, 2024
56 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Samuele review Needs a review from Samuele before it can land
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants