Merge lp:~bilalakhtar/unity-lens-applications/fix-750262 into lp:unity-lens-applications

Proposed by Bilal Akhtar
Status: Merged
Merged at revision: 201
Proposed branch: lp:~bilalakhtar/unity-lens-applications/fix-750262
Merge into: lp:unity-lens-applications
Diff against target: 29 lines (+14/-5)
1 file modified
src/daemon.vala (+14/-5)
To merge this branch: bzr merge lp:~bilalakhtar/unity-lens-applications/fix-750262
Reviewer Review Type Date Requested Status
Mikkel Kamstrup Erlandsen (community) Approve
Review via email: mp+56526@code.launchpad.net

Description of the change

This branch fixes bug #750262 .

To post a comment you must log in.
Revision history for this message
Mikkel Kamstrup Erlandsen (kamstrup) wrote :

There's something about this implementation I don't like too much. Right now you extract the package name and pass it to software-center. This only works by chance.

The "correct" way would be to pass the full "apt:package_name" string to AppInfo.launch_default_for_uri() because the apt: scheme is already registered with GNOME as you can verify by running "gnome-open apt:xterm" in a terminal.

review: Needs Fixing
Revision history for this message
Mikkel Kamstrup Erlandsen (kamstrup) wrote :

(and note - the reason why the unity-install:// scheme doesn't work "by chance" in general is because we extract all the metadata directly from software-center's own index when constructing the URL)

202. By Bilal Akhtar

Make sure apturl runs in the case of an apt: url, and not software-center

Revision history for this message
Mikkel Kamstrup Erlandsen (kamstrup) wrote :

Awesome Bilal! I saw your ping on IRC yesterday just when my laptop's battery died. I still think that AppInfo.launch_default_for_uri() is the right choice. The apt:foo thing is mainly for power users and I am not convinced they appreciate a full S-C launched.

And using the GNOME URI handling we give the platform team the option to install the software-center as apt: URI handler if they see that as better. Which hardcoding S-C doesn't make room for.

Your patch will be included in todays tarball.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'src/daemon.vala'
--- src/daemon.vala 2011-03-31 10:48:54 +0000
+++ src/daemon.vala 2011-04-06 15:06:36 +0000
@@ -763,11 +763,20 @@
763 {763 {
764 string orig;764 string orig;
765 orig = uri.offset (15);765 orig = uri.offset (15);
766 exec_or_dir = Utils.subst_tilde (orig);766 if (orig.has_prefix ("apt:")) {
767 args = exec_or_dir.split (" ", 0);767 try {
768 for (int i = 0; i < args.length; i++)768 AppInfo.launch_default_for_uri (orig, null);
769 args[i] = Utils.subst_tilde (args[i]);769 } catch (GLib.Error error) {
770770 warning ("failed to install package %s", orig.offset(4));
771 return ActivationStatus.NOT_ACTIVATED;
772 }
773 return ActivationStatus.ACTIVATED_HIDE_DASH;
774 } else {
775 exec_or_dir = Utils.subst_tilde (orig);
776 args = exec_or_dir.split (" ", 0);
777 for (int i = 0; i < args.length; i++)
778 args[i] = Utils.subst_tilde (args[i]);
779 }
771 this.runner.add_history (orig);780 this.runner.add_history (orig);
772 }781 }
773 else782 else

Subscribers

People subscribed via source and target branches