Merge lp:~shnatsel/noise/contractor-client-new-api into lp:~elementary-apps/noise/trunk

[Sergey "Shnatsel" Davidoff]

Add Contractor support

Show Contractor actions into Noise's right-click menu, so that you can perform actions on songs without the need to dig up the relevant files in file manager.

This code requires the new convenience functions in Granite's Contractor API (https://code.launchpad.net/~shnatsel/granite/get_contracts_for_files/+merge/188432) and the latest Contractor with proper handling of empty string mimetypes.

Since I check file existence in this code anyway, I'd like to indicate missing files in the UI (like playback attempts do). Is this a good idea and how do I go about it?

[Sergey "Shnatsel" Davidoff (shnatsel)]

Oh wait now it crashes mysteriously

review: Needs Fixing

[Sergey "Shnatsel" Davidoff (shnatsel)]

Looks like this code triggers some bug in Noise's TreeView that was not triggered before.
This code gets null Noise.Media's from get_selected_medias (), and the TreeView is visibly corrupted. The diffs from last working revision (1541) to first broken revisions (1542 and 1544) are innocious and cannot have such side effects as TreeView corruption.

[Sergey "Shnatsel" Davidoff (shnatsel)]

ListView corruption in action: https://dl.dropboxusercontent.com/u/5279564/noise-wtf.mp4

[Victor Martinez (victored)]

Looks like the actual media objects are being freed after your ContractMenuItems are destroyed (when the menu is closed). This is a common problem whenever you use GLib.List to store your objects, as it doesn't reference-count objects.

I only know two trivial ways to deal with this kind of issues:

1) Use "GLib.List<unowned Noise.Media>" in ContractMenuItem, so that the Media objects are not freed along with the list. "medias.copy()" should not be necessary after that.

2) Use a Gee data structure, as it reference-counts GObjects. I'd recommend using a Gee.ArrayList to store the elements.

This modification should suffice:

public class Noise.ContractMenuItem : Gtk.MenuItem {
    private Granite.Services.Contract contract;
    private Gee.List<Media> medias = new Gee.ArrayList<Media>;

    public ContractMenuItem (Granite.Services.Contract contract, GLib.List<Noise.Media> medias) {
 this.contract = contract;

 foreach (var m in medias)
     this.medias.add (m);

        // ...
    }
[...]

review: Needs Fixing

[Victor Martinez (victored)]

BTW, thank you for crediting me twice :D

[Sergey "Shnatsel" Davidoff (shnatsel)]

W00T! This is why it didn't work! Thanks a lot! Me and Tom have spent 5 hours debugging it with no luck XD

I wonder if I should use the newly-merged file-oriented API or leave it as it is?
Here's the difference between APIs: http://bazaar.launchpad.net/~shnatsel/noise/contractor-client-new-api/revision/1548/src/Views/ListView/Lists/MusicListView.vala

[Victor Martinez (victored)]

No problem.

I'd definitely use the new API. Less code is always better.

[Sergey "Shnatsel" Davidoff (shnatsel)]

I decided to stick to the old API for now, it's probably better suited for Noise's situation of not knowing of a file actually exists or not but still keeping its entry around.

I've fixed the double crediting and this is now ready for actual review.

Also it appears I've blown up the diff by regenerating the translation template. Oops! Scroll to the very bottom to see the actual code.

[Sergey "Shnatsel" Davidoff (shnatsel)]

review: Abstain

[Victor Martinez (victored)]

I fail to see the advantages of duplicating the code from Granite here. I would suggest using the new API method you introduced (in Granite) instead.

Also, I've noticed multiple calls like: "File.new_for_uri (m.uri);". Please note that Noise.Media has a "file" member you can access directly, like "m.file" [1]

Other than that and some coding-style issues, it looks good :)

[1] http://bazaar.launchpad.net/~elementary-apps/noise/trunk/view/head:/core/Media.vala#L59

review: Needs Fixing

[Sergey "Shnatsel" Davidoff (shnatsel)]

review: Needs Fixing

[lp:~shnatsel/noise/contractor-client-new-api]

1553. By Sergey "Shnatsel" Davidoff on 2013-10-09

when encountering a missing file, indicate that it's missing in the UI

[Sergey "Shnatsel" Davidoff (shnatsel)]

I'm now completely satisfied with this branch, and all the concerns voiced so far should be addressed.

review: Approve

[lp:~shnatsel/noise/contractor-client-new-api]

1554. By Sergey "Shnatsel" Davidoff on 2013-10-09

fix coding style in lines introduced by Contractor-related work

[Victor Martinez (victored)]

It's working nicely here.

There are indentation issues and an extra blank line between diff lines 142 - 148.

Also, after updating the "unique_status_image" property for each media object, is there any successive call to "queue_draw" on the view (and outside the foreach loop) so that the change is visually reflected?

review: Needs Fixing

[Sergey "Shnatsel" Davidoff (shnatsel)]

I can't see anything wrong with indentation on diff lines 142 - 148. If you

There are no queue_draw calls in this code, yet the change *is* visually reflected, instantly. Shall I add one? I'm not sure on which object shall I call it.

[Sergey "Shnatsel" Davidoff (shnatsel)]

I can't see anything wrong with indentation on diff lines 142 - 148. If you think that something is wrong there, you're welcome to fix it.

There are no queue_draw calls in this code, yet the change *is* visually reflected, instantly. Shall I add one? I'm not sure on which object shall I call it.

[Victor Martinez (victored)]

Hmmm. Something wrong with the diff tool maybe?

- Diff line 148: blank line. We don't leave this kind of blank space in Vala code.
- Diff lines 144 and 145: Code has been indented using 8 spaces instead of 4.
- Diff line 146: Closing bracket has been indented using 4 spaces. Notice how it does not match the indentation level of "foreach".

Logic wise this is OK. Please add the corrections.

[Victor Martinez (victored)]

Regarding queue_draw, yeah it's often not needed as gtk+ currently seems to re-draw the view when the menu is closed, or when you hover the cursor over an item in the list.

Anyway, it's better to avoid assuming GTK+ will always do it automatically and queue a re-draw explicitly. This may prevent future issues.

[Sergey "Shnatsel" Davidoff (shnatsel)]

I've fixed the indentation now. Thanks for spotting the mistakes!

Am I calling the redraws on the right object?

[lp:~shnatsel/noise/contractor-client-new-api]

1555. By Sergey "Shnatsel" Davidoff on 2013-10-13

coding style tweak + a redraw call

[Victor Martinez (victored)]

Yes, you are.

review: Approve

=== modified file 'src/Views/ListView/Lists/MusicListView.vala'
--- src/Views/ListView/Lists/MusicListView.vala	2013-06-13 19:59:30 +0000
+++ src/Views/ListView/Lists/MusicListView.vala	2013-10-13 20:59:26 +0000
@@ -14,19 +14,53 @@
  * statement from your version.
  *
  * Authored by: Scott Ringwelski <[e-mail]>,
- *              Victor Eduardo <[e-mail]>,
- *              Corentin Noël <[e-mail]>
+ *              Corentin Noël <[e-mail]>,
+ *              Lucas Baudin <[e-mail]>,
+ *              ammonkey <[e-mail]>,
+ *              Victor Martinez <[e-mail]>,
+ *              Sergey Davidoff <[e-mail]>
  */
 
 using Gee;
 using Gtk;
 
+public class Noise.ContractMenuItem : Gtk.MenuItem {
+    private Granite.Services.Contract contract;
+    private Gee.List<Media> medias = new Gee.LinkedList<Media> ();
+
+    public ContractMenuItem (Granite.Services.Contract contract, GLib.List<Noise.Media> medias) {
+        this.contract = contract;
+
+        foreach (var m in medias)
+            this.medias.add (m);
+
+        label = contract.get_display_name ();
+    }
+
+    public override void activate () {
+        File[] files = {};
+        foreach (Media m in this.medias) {
+            files += m.file;
+            debug("Added file to pass to Contractor: %s", m.uri);
+        }
+
+        try {
+            debug ("Executing contract \"%s\"", contract.get_display_name ());
+            contract.execute_with_files (files);
+        } catch (Error err) {
+            warning ("Error executing contract \"%s\": %s",
+                     contract.get_display_name (), err.message);
+        }
+    }
+}
+
 public class Noise.MusicListView : GenericList {
 
     //for media list right click
     Gtk.Menu mediaActionMenu;
     Gtk.MenuItem mediaEditMedia;
     Gtk.MenuItem mediaFileBrowse;
+    Gtk.MenuItem mediaMenuContractorEntry; // make menu on fly
     Gtk.MenuItem mediaTopSeparator;
     Gtk.MenuItem mediaMenuQueue;
     Gtk.MenuItem mediaMenuAddToPlaylist; // make menu on fly
@@ -34,6 +68,7 @@
     Gtk.MenuItem mediaRemove;
     Gtk.MenuItem importToLibrary;
     Gtk.MenuItem mediaScrollToCurrent;
+    Gtk.MenuItem mediaScrollToCurrentSeparator;
     bool is_queue = false;
     bool read_only = false;
 
@@ -87,8 +122,11 @@
         button_release_event.connect(viewClickRelease);
 
         mediaScrollToCurrent = new Gtk.MenuItem.with_label(_("Scroll to Current Song"));
+        mediaScrollToCurrentSeparator = new SeparatorMenuItem ();
+        mediaTopSeparator = new SeparatorMenuItem ();
         mediaEditMedia = new Gtk.MenuItem.with_label(_("Edit Song Info"));
         mediaFileBrowse = new Gtk.MenuItem.with_label(_("Show in File Browser"));
+        mediaMenuContractorEntry = new Gtk.MenuItem.with_label(_("Other actions"));
         mediaMenuQueue = new Gtk.MenuItem.with_label(C_("Action item (verb)", "Queue"));
         mediaMenuAddToPlaylist = new Gtk.MenuItem.with_label(_("Add to Playlist"));
         mediaRemove = new Gtk.MenuItem.with_label(_("Remove Song"));
@@ -103,17 +141,19 @@
         if(hint != ViewWrapper.Hint.ALBUM_LIST) {
             //mediaActionMenu.append(browseSame);
             mediaActionMenu.append(mediaScrollToCurrent);
+            mediaActionMenu.append(mediaScrollToCurrentSeparator);
         }
 
-        mediaTopSeparator = new SeparatorMenuItem ();
-
         if (read_only == false) {
             mediaActionMenu.append(mediaEditMedia);
         }
+
         mediaActionMenu.append(mediaFileBrowse);
+        mediaActionMenu.append(mediaMenuContractorEntry);
         if (read_only == false) {
             mediaActionMenu.append(mediaRateMedia);
         }
+
         mediaActionMenu.append(mediaTopSeparator);
         mediaActionMenu.append(mediaMenuQueue);
         if (read_only == false) {
@@ -177,7 +217,7 @@
             var mediaMenuNewPlaylist = new Gtk.MenuItem.with_label(_("New Playlist…"));
             mediaMenuNewPlaylist.activate.connect(mediaMenuNewPlaylistClicked);
             addToPlaylistMenu.append (mediaMenuNewPlaylist);
-            if(parent_wrapper.library.support_playlists () == false) {
+            if (parent_wrapper.library.support_playlists () == false) {
                 mediaMenuNewPlaylist.set_visible(false);
             }
             foreach (var playlist in parent_wrapper.library.get_playlists ()) {
@@ -235,6 +275,45 @@
 
             mediaRateMedia.rating_value = set_rating;
 
+            //remove the previous "Other Actions" submenu and create a new one
+            var contractorSubMenu = new Gtk.Menu ();
+            contractorSubMenu.show_all ();
+            mediaMenuContractorEntry.submenu = contractorSubMenu;
+            mediaMenuContractorEntry.sensitive = true;
+
+            try {
+                var files = new HashSet<File> (); //for automatic deduplication
+                var selected_medias = get_selected_medias (); //to avoid querying it every time
+                debug ("Number of selected medias obtained by MusicListView class: %u\n", selected_medias.length ());
+
+                foreach (var media in selected_medias) {
+                    if (media.file.query_exists ()) {
+                        files.add (media.file);
+                        //if the file was marked nonexistent, update its status
+                        if (media.location_unknown && media.unique_status_image != null) {
+                            media.unique_status_image = null;
+                            media.location_unknown = false;
+                        }
+                    } else {
+                        warning ("File %s does not exist, ignoring it", media.uri);
+                        //indicate that the file doesn't exist in the UI
+                        media.unique_status_image = Icons.PROCESS_ERROR.render(Gtk.IconSize.MENU);
+                        media.location_unknown = true;
+                    }
+                }
+
+                var contracts = Granite.Services.ContractorProxy.get_contracts_for_files (files.to_array ());
+                foreach (var contract in contracts) {
+                    var menu_item = new ContractMenuItem (contract, selected_medias);
+                    contractorSubMenu.append (menu_item);
+                }
+                this.queue_draw ();
+                contractorSubMenu.show_all ();
+            } catch (Error err) {
+                warning ("Failed to obtain Contractor actions: %s", err.message);
+                mediaMenuContractorEntry.sensitive = false;
+            }
+
             mediaActionMenu.popup (null, null, null, 3, get_current_event_time());
 
             return true;
@@ -321,7 +400,7 @@
         var to_edit = new LinkedList<int>();
         var to_edit_med = new LinkedList<Media>();
 
-        foreach(Media m in get_selected_medias()) {
+        foreach (Media m in get_selected_medias()) {
             to_edit.add(m.rowid);
             if(to_edit.size == 1)
                 to_edit_med.add(m);
@@ -360,7 +439,7 @@
     }
 
     protected void mediaFileBrowseClicked() {
-        foreach(Media m in get_selected_medias()) {
+        foreach (Media m in get_selected_medias()) {
             try {
                 var file = File.new_for_uri(m.uri);
                 Gtk.show_uri(null, file.get_parent().get_uri(), 0);
@@ -399,7 +478,7 @@
         var los = new LinkedList<Media>();
         int new_rating = mediaRateMedia.rating_value;
 
-        foreach(Media m in get_selected_medias()) {
+        foreach (Media m in get_selected_medias()) {
             m.rating = new_rating;
             los.add(m);
         }
@@ -431,7 +510,7 @@
     void importToLibraryClicked() {
         var to_import = new Gee.LinkedList<Media>();
 
-        foreach(Media m in get_selected_medias()) {
+        foreach (Media m in get_selected_medias()) {
             to_import.add (m);
         }
 
@@ -441,7 +520,7 @@
     protected virtual void onDragDataGet(Gdk.DragContext context, Gtk.SelectionData selection_data, uint info, uint time_) {
         string[] uris = null;
 
-        foreach(Media m in get_selected_medias()) {
+        foreach (Media m in get_selected_medias()) {
             debug("adding %s\n", m.uri);
             uris += (m.uri);
         }