Ubuntu Application Launcher

Merge lp:~ted/ubuntu-app-launch/snap-icon-unbreak into lp:ubuntu-app-launch

  1. snap-icon-unbreak
  2. Merge into trunk.17.04
Proposed by Ted Gould
Status: Rejected
Rejected by: Ted Gould
Proposed branch: lp:~ted/ubuntu-app-launch/snap-icon-unbreak
Merge into: lp:ubuntu-app-launch
Prerequisite: lp:~ted/ubuntu-app-launch/install-root
Diff against target: 40 lines (+30/-0)
1 file modified
libubuntu-app-launch/application-impl-snap.cpp (+30/-0)
To merge this branch: bzr merge lp:~ted/ubuntu-app-launch/snap-icon-unbreak
Reviewer Review Type Date Requested Status
dobey (community) Disapprove
unity-api-1-bot continuous-integration Needs Fixing
Michael Terry Approve
Review via email: mp+311224@code.launchpad.net

Commit message

Handle some common mistakes in making snap desktop files

Description of the change

:-( Handle bad snaps because they're everywhere :-(

To post a comment you must log in.
Revision history for this message
unity-api-1-bot (unity-api-1-bot) wrote :

PASSED: Continuous integration, rev:271
https://jenkins.canonical.com/unity-api-1/job/lp-ubuntu-app-launch-ci/129/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build/1107
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-0-fetch/1114
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=xenial+overlay/905
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=xenial+overlay/905/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=zesty/905
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=zesty/905/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=xenial+overlay/905
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=xenial+overlay/905/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=zesty/905
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=zesty/905/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=xenial+overlay/905
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=xenial+overlay/905/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=zesty/905
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=zesty/905/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://jenkins.canonical.com/unity-api-1/job/lp-ubuntu-app-launch-ci/129/rebuild

review: Approve (continuous-integration)
Revision history for this message
Michael Terry (mterry) wrote :

Maybe use a trailing slash for the has_prefix checks? Ideally we wouldn't match Exec=/snap/XXX/currentlybroken, although obviously such Exec lines are already troubled.

Revision history for this message
unity-api-1-bot (unity-api-1-bot) wrote :

PASSED: Continuous integration, rev:272
https://jenkins.canonical.com/unity-api-1/job/lp-ubuntu-app-launch-ci/130/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build/1125
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-0-fetch/1132
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=xenial+overlay/923
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=xenial+overlay/923/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=zesty/923
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=zesty/923/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=xenial+overlay/923
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=xenial+overlay/923/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=zesty/923
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=zesty/923/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=xenial+overlay/923
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=xenial+overlay/923/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=zesty/923
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=zesty/923/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://jenkins.canonical.com/unity-api-1/job/lp-ubuntu-app-launch-ci/130/rebuild

review: Approve (continuous-integration)
Revision history for this message
unity-api-1-bot (unity-api-1-bot) wrote :

PASSED: Continuous integration, rev:273
https://jenkins.canonical.com/unity-api-1/job/lp-ubuntu-app-launch-ci/131/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build/1147
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-0-fetch/1154
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=xenial+overlay/942
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=xenial+overlay/942/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=zesty/942
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=zesty/942/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=xenial+overlay/942
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=xenial+overlay/942/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=zesty/942
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=zesty/942/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=xenial+overlay/942
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=xenial+overlay/942/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=zesty/942
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=zesty/942/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://jenkins.canonical.com/unity-api-1/job/lp-ubuntu-app-launch-ci/131/rebuild

review: Approve (continuous-integration)
Revision history for this message
Michael Terry (mterry) wrote :

This looks good to me, and seems to work fine in the u8 snap. Thanks!

review: Approve
Revision history for this message
dobey (dobey) wrote :

Why does this depend on a series of incomplete and unapproved branches? Cannot this change be made independently?

review: Needs Information
lp:~ted/ubuntu-app-launch/snap-icon-unbreak updated
274. By Ted Gould

Pull through updates

Revision history for this message
unity-api-1-bot (unity-api-1-bot) wrote :

FAILED: Continuous integration, rev:274
https://jenkins.canonical.com/unity-api-1/job/lp-ubuntu-app-launch-ci/148/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/unity-api-1/job/build/1326/console
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-0-fetch/1333
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=xenial+overlay/1110
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=xenial+overlay/1110/artifact/output/*zip*/output.zip
    FAILURE: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=zesty/1110/console
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=xenial+overlay/1110
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=xenial+overlay/1110/artifact/output/*zip*/output.zip
    FAILURE: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=zesty/1110/console
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=xenial+overlay/1110
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=xenial+overlay/1110/artifact/output/*zip*/output.zip
    FAILURE: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=zesty/1110/console

Click here to trigger a rebuild:
https://jenkins.canonical.com/unity-api-1/job/lp-ubuntu-app-launch-ci/148/rebuild

review: Needs Fixing (continuous-integration)
Revision history for this message
dobey (dobey) wrote :

Now there are a lot of completely unrelated changes.

review: Needs Fixing
Revision history for this message
dobey (dobey) wrote :

I've proposed https://code.launchpad.net/~dobey/ubuntu-app-launch/snap-icon-unbreak/+merge/314461 instead, which takes the original 40 line change to strip the prefixes from Icon entries, and also added tests, as well as handling a couple more cases I've found that weren't handled.

Revision history for this message
dobey (dobey) :
review: Disapprove
lp:~ted/ubuntu-app-launch/snap-icon-unbreak updated
275. By Ted Gould

Merge install-root

Unmerged revisions

275. By Ted Gould

Merge install-root

274. By Ted Gould

Pull through updates

273. By Ted Gould

Need a slash in both

272. By Ted Gould

Put slashes in to be extra safe

271. By Ted Gould

Unhack some common snap icon mistakes

270. By Ted Gould

Grab install root

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'libubuntu-app-launch/application-impl-snap.cpp'
2--- libubuntu-app-launch/application-impl-snap.cpp 2017-01-12 14:56:40 +0000
3+++ libubuntu-app-launch/application-impl-snap.cpp 2017-01-12 14:56:41 +0000
4@@ -77,6 +77,36 @@
5 "' because: " + perror.get()->message);
6 }
7
8+ /* For bad reasons the Icon values in snaps have gotten to be a
9+ bit crazy. We're going to try to un-fu-bar a few common patterns
10+ here, but eh, we're just encouraging bad behavior */
11+ auto iconvalue = g_key_file_get_string(keyfile.get(), "Desktop Entry", "Icon", nullptr);
12+ if (iconvalue != nullptr)
13+ {
14+ const gchar* prefix{nullptr};
15+ if (g_str_has_prefix(iconvalue, "${SNAP}/"))
16+ {
17+ /* There isn't environment parsing in desktop file values :-( */
18+ prefix = "${SNAP}/";
19+ }
20+
21+ std::string currentdir = std::string{"/snap/"} + appid.package.value() + "/current/";
22+ if (g_str_has_prefix(iconvalue, currentdir.c_str()))
23+ {
24+ /* What? Why would we encode the snap path from root in a package
25+ format that is supposed to be relocatable? */
26+ prefix = currentdir.c_str();
27+ }
28+
29+ if (prefix != nullptr)
30+ {
31+ g_key_file_set_string(keyfile.get(), "Desktop Entry", "Icon", iconvalue + strlen(prefix) - 1);
32+ /* -1 to leave trailing slash */
33+ }
34+
35+ g_free(iconvalue);
36+ }
37+
38 return keyfile;
39 }(),
40 snapDir,

Subscribers

People subscribed via source and target branches