Merge lp:~jeremy-munsch/synapse-project/desktop-file-actions into lp:synapse-project

[Jeremy Munsch]

Update Desktop File service and Desktop File Plugin to provide applications actions

[Rico Tzschichholz (ricotz)]

Looks reasonable, but find_for_match() could use some more thinking to avoid this if-else-hell ;-)

Another issue here is the hidden bump to glib >= 2.38 (g_desktop_app_info_launch_action)

review: Needs Information

[Rico Tzschichholz (ricotz)]

Personally I am not a fan of the current code-style of synapse. Enclosing single-line scopes with brackets is overkill and just clutters the source. Similar thing with omitting a line-break for some single-if-statements.

[Rico Tzschichholz (ricotz)]

Would be nice to state the relation of this source to
https://code.launchpad.net/~donadigo/slingshot/search-quicklist/+merge/271755

review: Needs Information

[Jeremy Munsch (jeremy-munsch)]

Argh, dammit for the bump, wish some more info on versions could be provided on http://valadoc.org.
I guess this needs a workaround.
I'll try to make things cleaner on find_for_match.
As for you last comment about stating the relation of this source to the link provided, i am not sure to understand what you meant.
But i had a quick look i think the linked source is also adding the ability to search any action of any app, whereas this patch only add applications actions to their respective alternative panel.

I'll have a quick look tomorrow. I made a push with some code cleaning following your recommendations i believe.

Also i believe that i should not follow entirely the overall coding style of elementary-codestyle. I'm asking because the newbie that i am is a bit lost on that.

[Rico Tzschichholz (ricotz)]

> Argh, dammit for the bump, wish some more info on versions could be provided
> on http://valadoc.org.

Simply use devhelp which shows you versions besides nice documentations.

> I guess this needs a workaround.

I am not totally opposed to bump the glib requirements to 2.40.

> I'll try to make things cleaner on find_for_match.
> As for you last comment about stating the relation of this source to the link
> provided, i am not sure to understand what you meant.
> But i had a quick look i think the linked source is also adding the ability to
> search any action of any app, whereas this patch only add applications actions
> to their respective alternative panel.

You linked to the bug, so it is hard to believe you didnt notice the other merge-proposal. I just want to know if your work is based on it while using some parts. In this case you should reference it and mention its author.

> I'll have a quick look tomorrow. I made a push with some code cleaning
> following your recommendations i believe.

Do not touch code-parts which you didnt change. And if-else-clauses are either fully wrapped in brackets or none. So don't mix that.

> Also i believe that i should not follow entirely the overall coding style of
> elementary-codestyle. I'm asking because the newbie that i am is a bit lost on
> that.

Synapse has nothing to do with elementary so do not apply their codestyle in any way.

[Jeremy Munsch (jeremy-munsch)]

As for GLib, the bump would only leave precise (12.04LTS) unsupported since it is distributed with GLib 2.32.4. I only check Ubuntu because i believe the edge ppa of synapse has only releases to this distribution.

I used no code of the related commit, I originally linked the bug because that is the reason i made this fix and i got a mail notice of the bug.

I make my contributions on Synapse mostly by mimicking the original code for doing what i want since i'm definitely not a Vala expert. But i'll make sure if i use another code to make appropriate link to the original author.

Thank you for the devhelp tip, i'll use this from now :)

So what i'm going to fix are:
coding style for things I'have changed
cleaning find_for_match code

[Jeremy Munsch (jeremy-munsch)]

Hey just bumping the thread in case you have time to review, decided whether a bump for glib / merge is possibly going to happen.

[Rico Tzschichholz (ricotz)]

I have taken a look and came up with a bunch of changes
  https://paste.debian.net/plain/329998

DesktopActions can be translated so the user should see the translated string in the ui.

Why did you drop this?
  /* If there's more than one application, fill the ow list */
  if (list_for_mimetype.size > 1)

review: Needs Fixing

[Rico Tzschichholz (ricotz)]

Do not change the codestyle and alter unrelated parts.

If really needed then such cleaning should be done in a separate task while not changing any behavior.

[Jeremy Munsch (jeremy-munsch)]

Ok, i understand your point, though it would have been more readable.
So i updated from your patch made a small fix, and added tranlations by using GLib.DesktopAppInfo.get_action_name.

As for
  /* If there's more than one application, fill the ow list */
  if (list_for_mimetype.size > 1)

I removed it because, whitout your recommendations, i felt like it was non sense to not display the option for the only program that can open the file type. But, yes, i should not have done that. It is just hard to not touch things when you fell it could be improved, never mind though.

[Rico Tzschichholz (ricotz)]

https://paste.debian.net/plain/330250

* Don't add those verbose outputs.
* No need to do "actions_map[app_match.filename]" three times. (I already removed that in my first patch)

review: Needs Fixing

[Jeremy Munsch (jeremy-munsch)]

I just applied the patch and pushed, nothing else.

[Rico Tzschichholz (ricotz)]

Look fine now. I will keep that in the queue until I thought about dropping support for Precise/12.04

review: Approve

[lp:~jeremy-munsch/synapse-project/desktop-file-actions]

614. By Jeremy Munsch on 2015-11-14

desktop-file-service: add desktop file actions gathering
desktop-file-plugin :
  add new action OpenAppAction with supported translations for display actions
  add ApplicationMatch handler in find_for_match

[Jeremy Munsch (jeremy-munsch)]

I just made very small changes where thing were done wrong:
http://paste.ubuntu.com/13265410/

Also i found a bug with ChromePlugin that makes exceptions in desktop files actions.
The problem is ChromePlugin extends UriMatch without assigning uri properly.
It still seems to be working, however i will make a PR to make proper corrections to ChromePlugin.

I hope I'm not harassing you Rico, i know you have a tons of other projects to maintain to have time to spend in here, and on my noobish mystakes, since you seem to be the only maintainer left active.
If you feel that i'm making too much changes that are not necessary, please tell me.
I wish to contribute to this software even it seems pretty dead (no offence i have to say you're doing great job maintaining this), how ever if this is inappropriate, i can stop contributing to this project. I'm just not sure if these are welcome.

Best regards.

=== modified file 'src/core/desktop-file-service.vala'
--- src/core/desktop-file-service.vala	2015-07-23 08:18:50 +0000
+++ src/core/desktop-file-service.vala	2015-11-14 18:39:51 +0000
@@ -91,6 +91,7 @@
     public bool is_hidden { get; private set; default = false; }
     public bool is_valid { get; private set; default = true; }
 
+    public string[] actions = null;
     public string[] mime_types = null;
 
     private string? name_folded = null;
@@ -179,6 +180,8 @@
                                              KeyFileDesktop.KEY_NOT_SHOW_IN));
           show_in = DesktopEnvironmentType.ALL ^ not_show;
         }
+        if (keyfile.has_key (KeyFileDesktop.GROUP, KeyFileDesktop.KEY_ACTIONS))
+          actions = keyfile.get_string_list (KeyFileDesktop.GROUP, KeyFileDesktop.KEY_ACTIONS);
 
         // special case these, people are using them quite often and wonder
         // why they don't appear

=== modified file 'src/plugins/desktop-file-plugin.vala'
--- src/plugins/desktop-file-plugin.vala	2014-07-10 11:39:28 +0000
+++ src/plugins/desktop-file-plugin.vala	2015-11-14 18:39:51 +0000
@@ -84,6 +84,7 @@
     {
       desktop_files = new Gee.ArrayList<DesktopFileMatch> ();
       mimetype_map = new Gee.HashMap<string, OpenWithAction> ();
+      actions_map = new Gee.HashMap<string, OpenAppAction> ();
 
       var dfs = DesktopFileService.get_default ();
       dfs.reload_started.connect (() => {
@@ -244,7 +245,8 @@
           var app_info = new DesktopAppInfo.from_filename (desktop_info.filename);
           List<File> files = new List<File> ();
           files.prepend (f);
-          app_info.launch (files, null);
+          var display = Gdk.Display.get_default ();
+          app_info.launch (files, display.get_app_launch_context ());
         }
         catch (Error err)
         {
@@ -258,39 +260,107 @@
       }
     }
 
+    private class OpenAppAction : Action
+    {
+      public DesktopFileInfo desktop_info { get; construct; }
+      public string action { get; construct; }
+
+      DesktopAppInfo app_info;
+
+      public OpenAppAction (DesktopFileInfo info, string action)
+      {
+        Object (desktop_info : info, action : action);
+      }
+
+      construct
+      {
+        app_info = new DesktopAppInfo.from_filename (desktop_info.filename);
+        var display_action = app_info.get_action_name (action);
+        title = display_action;
+        icon_name = desktop_info.icon_name;
+        description = _("Launch action '%s'").printf (display_action);
+      }
+
+      public override void do_execute (Match match, Match? target = null)
+      {
+          var display = Gdk.Display.get_default ();
+          app_info.launch_action (action, display.get_app_launch_context ());
+          RelevancyService.get_default ().application_launched (app_info);
+      }
+
+      public override bool valid_for_match (Match match)
+      {
+        return (match is ApplicationMatch);
+      }
+    }
+
     private Gee.Map<string, Gee.List<OpenWithAction>> mimetype_map;
+    private Gee.Map<string, Gee.List<OpenAppAction>> actions_map;
 
     public ResultSet? find_for_match (ref Query query, Match match)
     {
-      unowned UriMatch? uri_match = match as UriMatch;
-      if (uri_match == null || uri_match.mime_type == null) return null;
+      unowned UriMatch? uri_match = null;
+      unowned ApplicationMatch? app_match = null;
+      Gee.List<Action>? any_list = null;
 
-      Gee.List<OpenWithAction> ow_list = mimetype_map[uri_match.mime_type];
-      /* Query DesktopFileService only if is necessary */
-      if (ow_list == null)
+      if ((uri_match = match as UriMatch) != null)
       {
-        /* Initialize ow_list */
-        ow_list = new Gee.LinkedList<OpenWithAction> ();
-        mimetype_map[uri_match.mime_type] = ow_list;
         var dfs = DesktopFileService.get_default ();
         var list_for_mimetype = dfs.get_desktop_files_for_type (uri_match.mime_type);
         /* If there's more than one application, fill the ow list */
         if (list_for_mimetype.size > 1)
         {
-          foreach (var entry in list_for_mimetype)
-          {
-            ow_list.add (new OpenWithAction (entry));
-          }
-        }
-        else return null;
-      }
-      else if (ow_list.size == 0) return null;
+          /* Query DesktopFileService only if is necessary */
+          Gee.List<OpenWithAction>? ow_list = mimetype_map[uri_match.mime_type];
+          if (ow_list == null)
+          {
+            ow_list = new Gee.LinkedList<OpenWithAction> ();
+            mimetype_map[uri_match.mime_type] = ow_list;
+
+            foreach (var entry in list_for_mimetype)
+            {
+              ow_list.add (new OpenWithAction (entry));
+            }
+          }
+
+          any_list = ow_list;
+        }
+      }
+      else if ((app_match = match as ApplicationMatch) != null)
+      {
+        Gee.List<OpenAppAction>? oa_list = actions_map[app_match.filename];
+        if (oa_list == null)
+        {
+          oa_list = new Gee.LinkedList<OpenAppAction> ();
+          actions_map[app_match.filename] = oa_list;
+
+          var dfs = DesktopFileService.get_default ();
+          var desktop_file_info = dfs.get_desktop_file_for_id (Path.get_basename (app_match.filename));
+
+          /* There should at a result here */
+          if (desktop_file_info != null)
+          {
+            foreach (var action in desktop_file_info.actions)
+            {
+              oa_list.add (new OpenAppAction (desktop_file_info, action));
+            }
+          }
+          else
+          {
+            warning ("No DesktopInfoFile for %s", app_match.filename);
+          }
+        }
+
+        any_list = oa_list;
+      }
+
+      if (any_list == null || any_list.size == 0) return null;
 
       var rs = new ResultSet ();
 
       if (query.query_string == "")
       {
-        foreach (var action in ow_list)
+        foreach (var action in any_list)
         {
           rs.add (action, MatchScore.POOR);
         }
@@ -299,7 +369,7 @@
       {
         var matchers = Query.get_matchers_for_query (query.query_string, 0,
           RegexCompileFlags.OPTIMIZE | RegexCompileFlags.CASELESS);
-        foreach (var action in ow_list)
+        foreach (var action in any_list)
         {
           foreach (var matcher in matchers)
           {