Created attachment 275726 [details] [review]
Swaps hardware manager signals emitted on GNU/Linux

In short, this patch stops us from 'pulling the rug out from under us' with respect to MTP devices and initialisation.

Created attachment 275727 [details] [review]
Swaps hardware manager signals emitted on GNU/Linux

Ack, sorry for the bad bug URL on the prior patch...

Without your patch, with a Nexus7 device and Ubuntu 14.04, if I connect it after banshee is already launched, I get:

[1 Warn  16:16:27.450] Failed to load media-player-info file for 1
Device 0 (VID=18d1 and PID=4e41) is a Google Inc (for Asus) Nexus 7 (MTP).
[1 Warn  16:16:27.529] Failed to load media-player-info file for 1
Unable to open ~/.mtpz-data for reading, MTPZ disabled.Android device detected, assigning default bug flags
[1 Debug 16:16:35.334] Creating Pango.Layout, configuring Cairo.Context
[1 Debug 16:16:36.445] Creating Pango.Layout, configuring Cairo.Context
[1 Debug 16:16:42.115] Creating Pango.Layout, configuring Cairo.Context
[1 Debug 16:16:43.409] Creating Pango.Layout, configuring Cairo.Context
[16 Warn  16:16:46.767] Unable to get battery level from MTP device - Mtp.LibMtpException: Could not retrieve battery stats (in `Mtp')
  at Mtp.MtpDevice.GetBatteryLevel (Mtp.MtpDeviceHandle handle, System.UInt16& maxLevel, System.UInt16& currentLevel) [0x0001c] in /home/knocte/Documents/Code/OpenSource/banshee/src/Libraries/Mtp/Mtp/MtpDevice.cs:312 
  at Mtp.MtpDevice.get_BatteryLevel () [0x0000b] in /home/knocte/Documents/Code/OpenSource/banshee/src/Libraries/Mtp/Mtp/MtpDevice.cs:68 
  at Banshee.Dap.Mtp.MtpSource.DeviceInitialize (IDevice device) [0x00285] in /home/knocte/Documents/Code/OpenSource/banshee/src/Dap/Banshee.Dap.Mtp/Banshee.Dap.Mtp/MtpSource.cs:146 
[16 Debug 16:16:46.767] Found DAP support (Banshee.Dap.Mtp.MtpSource) for device Nexus 7 and Uuid /devices/pci0000:00/0000:00:1a.0/usb1/1-1/1-1.2
[17 Debug 16:16:46.768] Unmapping DAP source (/devices/pci0000:00/0000:00:1a.0/usb1/1-1/1-1.2)
[18 Error 16:16:46.771] Caught an exception - System.Threading.ThreadAbortException: Thread was being aborted (in `mscorlib')
  at (wrapper managed-to-native) System.Threading.WaitHandle:WaitOne_internal (System.Threading.WaitHandle,intptr,int,bool)
  at System.Threading.WaitHandle.WaitOne () [0x00000] in <filename unknown>:0 
  at Hyena.Data.Sqlite.HyenaSqliteCommand.WaitForResult (Hyena.Data.Sqlite.HyenaSqliteConnection conn) [0x0000d] in /home/knocte/Documents/Code/OpenSource/banshee/src/Hyena/Hyena.Data.Sqlite/Hyena.Data.Sqlite/HyenaSqliteCommand.cs:151 
  at Hyena.Data.Sqlite.HyenaSqliteConnection.Execute (Hyena.Data.Sqlite.HyenaSqliteCommand command, System.Object[] param_values) [0x00012] in /home/knocte/Documents/Code/OpenSource/banshee/src/Hyena/Hyena.Data.Sqlite/Hyena.Data.Sqlite/HyenaSqliteConnection.cs:229 
  at Banshee.Sources.PrimarySource.PurgeTracks () [0x00020] in /home/knocte/Documents/Code/OpenSource/banshee/src/Core/Banshee.Services/Banshee.Sources/PrimarySource.cs:469 
  at Banshee.Dap.DapSource.ThreadedLoadDeviceContents () [0x00003] in /home/knocte/Documents/Code/OpenSource/banshee/src/Dap/Banshee.Dap/Banshee.Dap/DapSource.cs:306 


Which I think your patch seems to fix.

Did you test connecting the device after banshee is already launched, or the other way around?

Comment on attachment 275727 [details] [review]
Swaps hardware manager signals emitted on GNU/Linux

> Subject: [PATCH] When plugging an MTP device into the system, banshee must
 unmount it to be able to use it. Since the hardware manager emits signals
 when a device is unmounted, this causes banshee to unmap a device that it is
 in the middle of configuring.

The commit message doesn't respect Gnome commit message guidelines: https://wiki.gnome.org/Git/CommitMessages . (I've fixed this, please remember for the next patches ;) )

> -            monitor.MountAdded += HandleMonitorMountAdded;
> -            monitor.MountRemoved += HandleMonitorMountRemoved;

Just in case, I adjusted the patch to not remove these hooks, but to remove their implementation, and leave a debug log (which will be useful when working in the future at fixing MTP bugs).

> -        void HandleMonitorMountAdded (object o, MountAddedArgs args)
> +        void HandleMonitorVolumeAdded (object o, VolumeAddedArgs args)

I can see you tried to adhere to the code style that was already there in this same file, but there was a nitpick that wasn't respected (we explicity put "private" accessibility modifiers in the rest of banshee codebase), so while I won't ask you to fix this nitpick in the entire file, I think we should be fixing it for new code (so I've fixed this in your patch before committing it).

> +            var device = GudevDeviceFromGioVolume (volume);
> +            volume_device_map [volume.Handle] = device;

You forgot to check if device is null here (there are other parts of this file where this is checked, just in case, and a log is written when finding a null value; and the function returns early). I fixed this.

Also, given that you changed considerable the Banshee.Hardware.Gio/LowLevel/Manager.cs file, I added copyright attribution to yourself in the file (we normally do this when the changes to a file are significant).

Overall, just little nitpicks, and an awesome patch, which is now in master! Thanks a lot for your effort and for your patience in receiving a review.

This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report and bugfix!

I'm sorry, I've had to revert this patch because it broke the load of a Dap.MassStorage device I just tested (Nokia N900, but more could be affected).

We need to find a way to still take in account Mount events, but ignore them when dealing with MTP. Ideas?

What happened with them? 

I tried my S2 in mass storage mode and it worked fine with the patch applied.

With my NokiaN900 it doesn't mount anymore, because I guess it receives mount events instead of volume events.

Created attachment 276625 [details] [review]
Gets us some useful diagnosis information

With this patch applied, in MTP mode my Samsung S2 produces this output:

*connect*
[1 Debug 20:37:20.504] (Ignored) VolumeAdded event received
[1 Debug 20:37:24.766] (Ignored) MountAdded event received
[1 Debug 20:37:24.772] (Ignored) MountAdded event received
*disconnect*
[1 Debug 20:37:34.317] (Ignored) VolumeRemoved event received
[1 Debug 20:37:34.318] (Ignored) MountRemoved event received
[1 Debug 20:37:34.345] (Ignored) MountRemoved event received

I thought when I first wrote the patch (the one that didn't work with the n900), that it might be a winner to go all the way to DriveConnected/Disconnected, that hasn't worked and I was quite disappointed, but I tried it in USB Storage Mode (Luckily, my phone's software isn't all that recent) and I got this:

*connect*
[1 Debug 20:39:28.876] Received DriveConnected
[1 Debug 20:39:28.877] GLib.DriveAdapter Has No Volumes
[1 Debug 20:39:28.877] Received DriveConnected
[1 Debug 20:39:28.878] GLib.DriveAdapter Has No Volumes
*press "Turn on USB storage" button on phone*
[1 Debug 20:39:55.647] (Ignored) VolumeAdded event received
[1 Debug 20:39:56.758] (Ignored) MountAdded event received
*eject in nautilus*
[1 Debug 20:41:54.011] (Ignored) MountRemoved event received
[1 Debug 20:41:54.081] (Ignored) VolumeRemoved event received
*cable disconnect*
[1 Debug 20:42:13.200] Received DriveDisconnected
[1 Debug 20:42:13.200] GLib.DriveAdapter Disconnected, Had No Volumes
[1 Debug 20:42:13.200] Received DriveDisconnected
[1 Debug 20:42:13.200] GLib.DriveAdapter Disconnected, Had No Volumes

Can you try it on your n900 and see what output you get?

I'm looking at the pre-patched code, and since we bail out of the MountAdded handler if there's no volume the n900 must have one, in which case why don't we get a notification when it gets added?

This is what I get with that patch, when I connect the N900:

[1 Debug 22:38:41.886] Received DriveConnected
[1 Debug 22:38:41.886] GLib.DriveAdapter Has No Volumes
[1 Debug 22:38:41.887] Received DriveConnected
[1 Debug 22:38:41.887] GLib.DriveAdapter Has No Volumes
[1 Debug 22:38:46.567] (Ignored) VolumeAdded event received
[1 Debug 22:38:46.623] (Ignored) MountAdded event received
[1 Debug 22:38:46.692] Creating Pango.Layout, configuring Cairo.Context

BTW, you could also test with your Nokia N95 :)

When I got the S2, I sent the Nokia for recycling, otherwise I would.

So, we do we get a VolumeAdded event, in that case (with the original patch) when we do (line 87)

var volume = GLib.VolumeAdapter.GetObject ((GLib.Object) args.Args [0]);

we must be getting a null pointer? Do you mind confirming that? If it is, what about the Volume property of the VolumeAddedArgs, is that also null?

(In reply to comment #11)
> When I got the S2, I sent the Nokia for recycling, otherwise I would.
> 
> So, we do we get a VolumeAdded event, in that case (with the original patch)
> when we do (line 87)
> 
> var volume = GLib.VolumeAdapter.GetObject ((GLib.Object) args.Args [0]);

volume is actually not null here! So I wonder how the hell it doesn't work? :-m

Say What?!

Then the only other place it could bail is the null check in VolumeAdded (line 116) though the assignment is 4 lines before:

var device = GudevDeviceFromGioVolume (volume);

(In reply to comment #13)
> Say What?!
> 
> Then the only other place it could bail is the null check in VolumeAdded (line
> 116) though the assignment is 4 lines before:
> 
> var device = GudevDeviceFromGioVolume (volume);

That is not null either. WTF is going on here?

Ok, I debugged it a bit, and it seems that with your patch, DapService.FindServiceSource() is catching an InvalidDeviceException that doesn't seem to happen without your patch. (Even removing the changes of your patch that modify the file MassStorageSource.cs doesn't make it work.) I'll provide more info tomorrow.

Ok, finally debugged this, and the problem is that, with your patch, source.Volume.MountPoint in MassStorageDevice.cs returns null, therefore accessing these two properties throws ArgumentNullException:

        private string IsAudioPlayerPath {
            get { return System.IO.Path.Combine (source.Volume.MountPoint, ".is_audio_player"); }
        }

        private string IsNotAudioPlayerPath {
            get { return System.IO.Path.Combine (source.Volume.MountPoint, ".is_not_audio_player"); }
        }

Created attachment 277482 [details] [review]
Revised Version

I had a bit of a play with this one this afternoon and I think I've came up with something acceptable, but I don't think it's quite push ready yet. Do you mind testing it with your Nokia and seeing how it fares?

With this applied you should be able to have them recognised when they're plugged in. MTP devices are almost there-when I tested my S2, banshee recognised it and tried to connect but nautilus mounted and stole it-at which point I was able just to unmount from nautilus and next time through the loop banshee connected! (This leads us to some points in the bigger picture here, R6 and R9 from 724243)

I think perhaps the new DeviceChangedArgs should just carry a UUID string and the OnHardwareDeviceChanged method in DapService should probably do more than just attempting to Map the device but other than those concerns I think it's about right. :)

Good news, last patch works for me loading NokiaN900, so looking forward to get the "pushable" version :) (there are some style issues in this patch, plus your concerns)

Created attachment 277583 [details] [review]
Push Ready Revision

I've updated the patch and commit message to reflect the more general approach to the problem of abstracting the hardware messages down to three events.

Also, I took the liberty of trimming a method from HardwareManager and invoking a consistent style WRT the calling of delegates.

WRT my concerns, the idea with DeviceChanged is that the Device is still available, so it's right to send a functioning object instead of just an identifier, and only when it's still there-so in the case where VolumeRemoved happens before MountRemoved (The user unplugged the device without unmounting it) the signal won't be raised.

We could cut the DeviceChangedArgs down to a string uuid, if we allow MassStorageSources to be mapped when they become available instead of just when they're mounted. Otherwise there's no source uuid to lookup in DapService, which we can poke to see if its device is still alive.

Comment on attachment 277583 [details] [review]
Push Ready Revision

(In reply to comment #19)
> Created an attachment (id=277583) [details] [review]
> Push Ready Revision

Quick review:


> @@ -49,28 +50,34 @@ namespace Banshee.Hardware.Gio
>              manager.Dispose ();
>          }
>  
> -        void HandleManagerDeviceAdded (object o, MountArgs args)
> +        private void HandleManagerDeviceAdded (object o, MountArgs args)

You're not really changing this line, so please add the improvement of code-style to a separate patch.


>          {
> -            HandleManagerDeviceAdded (args.Device);
> +            var h = DeviceAdded;
> +            if (h != null && args.Device != null) {
> +                h (this, new DeviceAddedArgs (args.Device));
> +            }
>          }
>  
> -        void HandleManagerDeviceRemoved (object o, MountArgs args)
> +        private void HandleManagerDeviceChanged (object sender, MountArgs e)

Same here.

>          {
> -            if (DeviceRemoved != null) {
> -                DeviceRemoved (this, new DeviceRemovedArgs (args.Device.Uuid));
> +            var h = DeviceChanged;
> +            if (null != h) {
> +                h (this, new DeviceChangedArgs (e.Device));
>              }
>          }

Let's try to avoid tiny variable names, use "handler" instead, like in other parts of Banshee code.

>  
> -        void HandleManagerDeviceAdded (IDevice device)
> +        private void HandleManagerDeviceRemoved (object o, MountArgs args)
>          {
> -            if (device != null && DeviceAdded != null) {
> -                DeviceAdded (this, new DeviceAddedArgs (device));
> +            var h = DeviceRemoved;
> +            if (null != h) {
> +                h (this, new DeviceRemovedArgs (args.Device.Uuid));
>              }
>          }

This is a good improvement in itself, but then it is not related to this patch. I've committed this improvement as a separated patch.


>  
>  #region IHardwareManager
>  
>          public event DeviceAddedHandler DeviceAdded;
> +        public event DeviceChangedHandler DeviceChanged;
>          public event DeviceRemovedHandler DeviceRemoved;
>  
>          public IEnumerable<IDevice> GetAllDevices ()
> diff --git a/src/Backends/Banshee.Gio/Banshee.Hardware.Gio/LowLevel/Manager.cs b/src/Backends/Banshee.Gio/Banshee.Hardware.Gio/LowLevel/Manager.cs
> index f635b93..42984ea 100644
> --- a/src/Backends/Banshee.Gio/Banshee.Hardware.Gio/LowLevel/Manager.cs
> +++ b/src/Backends/Banshee.Gio/Banshee.Hardware.Gio/LowLevel/Manager.cs
> @@ -3,8 +3,10 @@
>  //
>  // Author:
>  //   Alex Launi <alex.launi@gmail.com>
> +//   Nicholas Little <arealityfarbetween@googlemail.com>
>  //
>  // Copyright (c) 2010 Alex Launi
> +// Copyright (c) 2014 Nicholas Little
>  //
>  // Permission is hereby granted, free of charge, to any person obtaining a copy
>  // of this software and associated documentation files (the "Software"), to deal
> @@ -35,6 +37,8 @@ using GUdev;
>  using System.Runtime.InteropServices;
>  using Banshee.Hardware;
>  
> +using Log = Hyena.Log;
> +
>  namespace Banshee.Hardware.Gio
>  {
>      public class Manager : IEnumerable<IDevice>, IDisposable
> @@ -49,6 +53,7 @@ namespace Banshee.Hardware.Gio
>          private Dictionary<IntPtr, GUdev.Device> volume_device_map;
>  
>          public event EventHandler<MountArgs> DeviceAdded;
> +        public event EventHandler<MountArgs> DeviceChanged;
>          public event EventHandler<MountArgs> DeviceRemoved;
>  
>          public Manager ()
> @@ -57,6 +62,7 @@ namespace Banshee.Hardware.Gio
>              monitor = VolumeMonitor.Default;
>              monitor.MountAdded += HandleMonitorMountAdded;
>              monitor.MountRemoved += HandleMonitorMountRemoved;
> +            monitor.VolumeAdded += HandleMonitorVolumeAdded;
>              monitor.VolumeRemoved += HandleMonitorVolumeRemoved;
>              volume_device_map= new Dictionary<IntPtr, GUdev.Device> ();
>          }
> @@ -69,60 +75,87 @@ namespace Banshee.Hardware.Gio
>          }
>  #endregion
>  
> -        void HandleMonitorMountAdded (object o, MountAddedArgs args)
> +        private RawVolume createRawVolume (GLib.IVolume volume)

PascalNotation, not camelCase, please.


>          {
> -            // Manually get the mount as gio-sharp translates it to the wrong managed object
> -            // TODO: check if this workaround is still needed
> -            var mount = GLib.MountAdapter.GetObject ((GLib.Object) args.Args [0]);
> -            if (mount.Volume == null)
> -                return;
> +            GUdev.Device device = volume_device_map [volume.Handle];
> +            return new RawVolume (volume,
> +                this,
> +                new GioVolumeMetadataSource (volume),
> +                new UdevMetadataSource (device));
> +        }
>  
> -            var device = GudevDeviceFromGioMount (mount);
> -            if (device == null) {
> -                Hyena.Log.Debug (string.Format ("Tried to mount {0}/{1} with no matching udev device", mount.Volume.Name, mount.Volume.Uuid));
> -                return;
> -            }
> +        private void HandleMonitorMountAdded (object o, MountAddedArgs args)
> +        {
> +            Log.Debug ("HandleMonitorMountAdded");

Let's be a bit more verbose in logging lines like this, for example: Log.Debug ("Gio: LowLevelHardwareManager received a HandleMonitorMountAdded event");

> +            VolumeChanged (args.Mount.Volume);
> +        }
>  
> -            volume_device_map [mount.Volume.Handle] = device;
> -            var h = DeviceAdded;
> -            if (h != null) {
> -                var v = new RawVolume (mount.Volume,
> -                                          this,
> -                                          new GioVolumeMetadataSource (mount.Volume),
> -                                          new UdevMetadataSource (device));
> -                h (this, new MountArgs (HardwareManager.Resolve (new Device (v))));
> -            }
> +        private void HandleMonitorMountRemoved (object o, MountRemovedArgs args)
> +        {
> +            Log.Debug ("HandleMonitorMountRemoved");

Same here.


> +            VolumeChanged (args.Mount.Volume);
>          }
>  
> -        void HandleMonitorMountRemoved (object o, MountRemovedArgs args)
> +        private void HandleMonitorVolumeAdded (object o, VolumeAddedArgs args)
>          {
> -            // Manually get the mount as gio-sharp translates it to the wrong managed object
> -            var mount = GLib.MountAdapter.GetObject ((GLib.Object) args.Args [0]);
> -            if (mount.Volume == null) {
> +            Log.Debug ("HandleMonitorVolumeAdded");
> +            var volume = GLib.VolumeAdapter.GetObject ((GLib.Object) args.Args [0]);
> +            if (volume == null) {
> +                Log.Error ("HandleMonitorVolumeAdded: volume == null");
>                  return;
>              }
>  
> -            VolumeRemoved (mount.Volume);
> +            VolumeAdded (volume);
>          }
>  
>          void HandleMonitorVolumeRemoved (object o, VolumeRemovedArgs args)
>          {
> +            Log.Debug ("HandleMonitorVolumeRemoved");

Same here.


>              var volume = GLib.VolumeAdapter.GetObject ((GLib.Object) args.Args [0]);
>              if (volume == null) {
> +                Log.Error ("HandleMonitorVolumeRemoved: volume == null");

Same here.

>                  return;
>              }
>  
>              VolumeRemoved (volume);
>          }
>  
> +        private void VolumeAdded (GLib.IVolume volume)
> +        {
> +            var device = GudevDeviceFromGioVolume (volume);
> +            volume_device_map [volume.Handle] = device;
> +            var h = DeviceAdded;
> +            if (h != null) {
> +                if (device == null) {
> +                    Hyena.Log.ErrorFormat ("VolumeAdded: {0}/{1} with no matching udev device", volume.Name, volume.Uuid);
> +                    return;
> +                }
> +                var v = new Device (createRawVolume (volume));
> +                h (this, new MountArgs (HardwareManager.Resolve (v)));
> +            }
> +        }
> +
> +        private void VolumeChanged (GLib.IVolume volume)
> +        {
> +            if (null == volume) {
> +                Log.Error ("VolumeChanged: volume == null");
> +                return;
> +            }
> +
> +            var h = DeviceChanged;
> +            if (null != h) {
> +                var device = new Device (createRawVolume (volume));
> +                h (this, new MountArgs (HardwareManager.Resolve (device)));
> +            }
> +        }
>  
> -        void VolumeRemoved (GLib.IVolume volume)
> +        private void VolumeRemoved (GLib.IVolume volume)

In a separate patch please.


>          {
>              var h = DeviceRemoved;
>              if (h != null) {
>                  GUdev.Device device;
>                  if (!volume_device_map.TryGetValue (volume.Handle, out device)) {
> -                    Hyena.Log.Debug (string.Format ("Tried to unmount {0}/{1} with no matching udev device", volume.Name, volume.Uuid));
> +                    Hyena.Log.ErrorFormat ("VolumeRemoved: {0}/{1} with no matching udev device", volume.Name, volume.Uuid);
>                      return;
>                  }
>                  var v = new RawVolume (volume,
> diff --git a/src/Core/Banshee.Services/Banshee.Hardware/HardwareManager.cs b/src/Core/Banshee.Services/Banshee.Hardware/HardwareManager.cs
> index f15b7d8..cde0041 100644
> --- a/src/Core/Banshee.Services/Banshee.Hardware/HardwareManager.cs
> +++ b/src/Core/Banshee.Services/Banshee.Hardware/HardwareManager.cs
> @@ -42,6 +42,7 @@ namespace Banshee.Hardware
>          private Dictionary<string, ICustomDeviceProvider> custom_device_providers = new Dictionary<string, ICustomDeviceProvider> ();
>  
>          public event DeviceAddedHandler DeviceAdded;
> +        public event DeviceChangedHandler DeviceChanged;
>          public event DeviceRemovedHandler DeviceRemoved;
>  
>          public HardwareManager ()
> @@ -66,6 +67,7 @@ namespace Banshee.Hardware
>              }
>  
>              manager.DeviceAdded += OnDeviceAdded;
> +            manager.DeviceChanged += OnDeviceChanged;
>              manager.DeviceRemoved += OnDeviceRemoved;
>  
>              ServiceManager.Get<DBusCommandService> ().ArgumentPushed += OnCommandLineArgument;
> @@ -78,6 +80,7 @@ namespace Banshee.Hardware
>              lock (this) {
>                  if (manager != null) {
>                      manager.DeviceAdded -= OnDeviceAdded;
> +                    manager.DeviceChanged -= OnDeviceChanged;
>                      manager.DeviceRemoved -= OnDeviceRemoved;
>                      manager.Dispose ();
>                      manager = null;
> @@ -186,6 +189,16 @@ namespace Banshee.Hardware
>              }
>          }
>  
> +        private void OnDeviceChanged (object o, DeviceChangedArgs args)
> +        {
> +            lock (this) {
> +                var h = DeviceChanged;
> +                if (null != h) {
> +                    h (this, args);
> +                }
> +            }
> +        }
> +
>          private void OnDeviceRemoved (object o, DeviceRemovedArgs args)
>          {
>              lock (this) {
> diff --git a/src/Core/Banshee.Services/Banshee.Hardware/IHardwareManager.cs b/src/Core/Banshee.Services/Banshee.Hardware/IHardwareManager.cs
> index e1c512c..b3e516f 100644
> --- a/src/Core/Banshee.Services/Banshee.Hardware/IHardwareManager.cs
> +++ b/src/Core/Banshee.Services/Banshee.Hardware/IHardwareManager.cs
> @@ -34,6 +34,7 @@ namespace Banshee.Hardware
>      public interface IHardwareManager : IDisposable
>      {
>          event DeviceAddedHandler DeviceAdded;
> +        event DeviceChangedHandler DeviceChanged;
>          event DeviceRemovedHandler DeviceRemoved;
>  
>          IEnumerable<IDevice> GetAllDevices ();
> @@ -43,6 +44,7 @@ namespace Banshee.Hardware
>      }
>  
>      public delegate void DeviceAddedHandler (object o, DeviceAddedArgs args);
> +    public delegate void DeviceChangedHandler (object o, DeviceChangedArgs args);
>      public delegate void DeviceRemovedHandler (object o, DeviceRemovedArgs args);
>  
>      public sealed class DeviceAddedArgs : EventArgs
> @@ -59,6 +61,20 @@ namespace Banshee.Hardware
>          }
>      }
>  
> +    public sealed class DeviceChangedArgs : EventArgs
> +    {
> +        private IDevice device;
> +
> +        public DeviceChangedArgs (IDevice device)
> +        {
> +            this.device = device;
> +        }
> +
> +        public IDevice Device {
> +            get { return device; }
> +        }
> +    }
> +
>      public sealed class DeviceRemovedArgs : EventArgs
>      {
>          private string device_uuid;
> diff --git a/src/Dap/Banshee.Dap.MassStorage/Banshee.Dap.MassStorage/MassStorageSource.cs b/src/Dap/Banshee.Dap.MassStorage/Banshee.Dap.MassStorage/MassStorageSource.cs
> index 3c54f36..08654fd 100644
> --- a/src/Dap/Banshee.Dap.MassStorage/Banshee.Dap.MassStorage/MassStorageSource.cs
> +++ b/src/Dap/Banshee.Dap.MassStorage/Banshee.Dap.MassStorage/MassStorageSource.cs
> @@ -63,16 +63,24 @@ namespace Banshee.Dap.MassStorage
>              base.DeviceInitialize (device);
>  
>              volume = device as IVolume;
> -            if (volume == null || !volume.IsMounted || (usb_device = volume.ResolveRootUsbDevice ()) == null) {
> +
> +            if (volume == null || (usb_device = volume.ResolveRootUsbDevice ()) == null) {
>                  throw new InvalidDeviceException ();
>              }
>  
> +            if (!volume.IsMounted && !volume.CanMount) {
> +                throw new InvalidDeviceException ();
> +            } else if (!volume.IsMounted) {
> +                volume.Mount ();
> +            }
> +
>              ms_device = DeviceMapper.Map (this);
>              try {
>                  if (ms_device.ShouldIgnoreDevice () || !ms_device.LoadDeviceConfiguration ()) {
>                      ms_device = null;
>                  }
> -            } catch {
> +            } catch (Exception e) {
> +                Log.Exception (e);

This is a good improvement in itself, but not related to this patch. I've pushed it as a separate commit.


>                  ms_device = null;
>              }
>  
> diff --git a/src/Dap/Banshee.Dap/Banshee.Dap/DapService.cs b/src/Dap/Banshee.Dap/Banshee.Dap/DapService.cs
> index bfb55d4..7ceb59b 100644
> --- a/src/Dap/Banshee.Dap/Banshee.Dap/DapService.cs
> +++ b/src/Dap/Banshee.Dap/Banshee.Dap/DapService.cs
> @@ -72,6 +72,7 @@ namespace Banshee.Dap
>                  AddinManager.AddExtensionNodeHandler ("/Banshee/Dap/DeviceClass", OnExtensionChanged);
>  
>                  ServiceManager.HardwareManager.DeviceAdded += OnHardwareDeviceAdded;
> +                ServiceManager.HardwareManager.DeviceChanged += OnHardwareDeviceChanged;
>                  ServiceManager.HardwareManager.DeviceRemoved += OnHardwareDeviceRemoved;
>                  ServiceManager.HardwareManager.DeviceCommand += OnDeviceCommand;
>                  ServiceManager.SourceManager.SourceRemoved += OnSourceRemoved;
> @@ -294,6 +295,11 @@ namespace Banshee.Dap
>              MapDevice (args.Device);
>          }
>  
> +        private void OnHardwareDeviceChanged (object o, DeviceChangedArgs args)
> +        {
> +            MapDevice (args.Device);
> +        }
> +
>          private void OnHardwareDeviceRemoved (object o, DeviceRemovedArgs args)
>          {
>              UnmapDevice (args.DeviceUuid);


This patch still works for mounting my N900, I'll test now with a Nexus7!

(In reply to comment #20)
> I'll test now with a Nexus7!

Just connected it, while banshee was running, and nautilus told me it couldn't mount the device (maybe because Banshee claimed it first?), but Banshee didn't show the device either. I got this in the log:

[1 Debug 14:32:53.830] HandleMonitorVolumeAdded
[1 Warn  14:32:53.872] Failed to load media-player-info file for 1
[20 Info  14:32:53.876] AppleDeviceSource is ignoring unmounted volume Nexus 7
Device 0 (VID=18d1 and PID=4e41) is a Google Inc (for Asus) Nexus 7 (MTP).
PTP_ERROR_IO: failed to open session, trying again after resetting USB interface
LIBMTP libusb: Attempt to reset device

I then tried to quit Banshee, and it froze, so I decided to kill with -s QUIT to check for deadlocks, this is the log:

[1 Debug 14:33:14.432] Service disposed (LastfmStreamingService)
[1 Debug 14:33:14.438] Service disposed (PodcastService)
[1 Debug 14:33:14.439] Service disposed (MediaPanelService)
[1 Debug 14:33:14.440] Service disposed (DvdService)
[1 Debug 14:33:14.441] Service disposed (AudioCdService)
[1 Debug 14:33:14.443] Service disposed (AudioscrobblerService)
[1 Debug 14:33:14.445] Service disposed (MultimediaKeysService)
[1 Debug 14:33:14.445] Service disposed (GStreamerCoreService)
[1 Debug 14:33:14.447] Service disposed (NotificationAreaService)
LIBMTP PANIC: failed to open session on second attempt
[20 Debug 14:33:54.269] Failed to connect to mtp device. Trying 4 more times...
ignoring libusb_claim_interface() = -6PTP_ERROR_IO: failed to open session, trying again after resetting USB interface
LIBMTP libusb: Attempt to reset device
Unable to open ~/.mtpz-data for reading, MTPZ disabled.Android device detected, assigning default bug flags
Full thread dump:

"Threadpool worker" tid=0x0x2b55ddc52700 this=0x0x2b55dd5b02e0 thread handle 0x68e state : not waiting owns ()


"Threadpool monitor" tid=0x0x2b55b52bd700 this=0x0x2b559da06118 thread handle 0x4b8 state : interrupted state owns ()


"Threadpool worker" tid=0x0x2b55dda51700 this=0x0x2b55dd5b0178 thread handle 0x68c state : interrupted state owns ()


"Threadpool worker" tid=0x0x2b55c3d2f700 this=0x0x2b559da0fe98 thread handle 0x688 state : interrupted state owns ()


"DAAP Proxy" tid=0x0x2b55dd850700 this=0x0x2b559da0fd30 thread handle 0x686 state : interrupted state owns ()
  at <unknown> <0xffffffff>
  at (wrapper managed-to-native) System.Net.Sockets.Socket.Accept_internal (intptr,int&,bool) <IL 0x0000f, 0xffffffff>
  at System.Net.Sockets.Socket.Accept () <IL 0x00044, 0x00083>
  at Banshee.Web.BaseHttpServer.ServerLoop () [0x00091] in /home/knocte1404/banshee/src/Core/Banshee.Services/Banshee.Web/BaseHttpServer.cs:161
  at System.Threading.Thread.StartInternal () <IL 0x00021, 0x00088>
  at (wrapper runtime-invoke) object.runtime_invoke_void__this__ (object,intptr,intptr,intptr) <IL 0x0004e, 0xffffffff>


"Threadpool worker" tid=0x0x2b55c1409700 this=0x0x2b559da06280 thread handle 0x4ba state : interrupted state owns ()
  at <unknown> <0xffffffff>
  at (wrapper managed-to-native) System.Threading.WaitHandle.WaitAny_internal (System.Threading.WaitHandle[],int,bool) <IL 0x0000f, 0xffffffff>
  at System.Threading.WaitHandle.WaitAny (System.Threading.WaitHandle[],System.TimeSpan,bool) <IL 0x0003e, 0x0008f>
  at System.Threading.RegisteredWaitHandle.Wait (object) <IL 0x00021, 0x000f3>
  at (wrapper runtime-invoke) <Module>.runtime_invoke_void__this___object (object,intptr,intptr,intptr) <IL 0x00052, 0xffffffff>


"HyenaSqliteConnection (/home/knocte1404/.config/banshee-1/banshee.db)" tid=0x0x2b55b5800700 this=0x0x2b559da00b50 thread handle 0x47f state : interrupted state owns ()
  at <unknown> <0xffffffff>
  at (wrapper managed-to-native) System.Threading.WaitHandle.WaitOne_internal (System.Threading.WaitHandle,intptr,int,bool) <IL 0x0001c, 0xffffffff>
  at System.Threading.WaitHandle.WaitOne () <IL 0x00023, 0x0007f>
  at Hyena.Data.Sqlite.HyenaSqliteConnection.ProcessQueue () [0x00132] in /home/knocte1404/banshee/src/Hyena/Hyena.Data.Sqlite/Hyena.Data.Sqlite/HyenaSqliteConnection.cs:454
  at System.Threading.Thread.StartInternal () <IL 0x00021, 0x00088>
  at (wrapper runtime-invoke) object.runtime_invoke_void__this__ (object,intptr,intptr,intptr) <IL 0x0004e, 0xffffffff>


"Banshee.Kernel Job Thread" tid=0x0x2b55c0e07700 this=0x0x2b55dd5b0880 thread handle 0x696 state : interrupted state owns ()
  at <unknown> <0xffffffff>
  at (wrapper managed-to-native) System.Threading.Monitor.try_enter_with_atomic_var (object,int,bool&) <IL 0x0000f, 0xffffffff>
  at System.Threading.Monitor.TryEnter (object,int,bool&) <IL 0x00044, 0x00063>
  at System.Threading.Monitor.Enter (object,bool&) <IL 0x00003, 0x0003b>
  at (wrapper unknown) System.Threading.Monitor.FastMonitorEnterV4 (object,bool&) <IL 0x00059, 0xffffffff>
  at Banshee.ServiceStack.ServiceManager.Get (string) [0x00001] in /home/knocte1404/banshee/src/Core/Banshee.Services/Banshee.ServiceStack/ServiceManager.cs:344
  at Banshee.ServiceStack.ServiceManager.get_DbConnection () [0x00006] in /home/knocte1404/banshee/src/Core/Banshee.Services/Banshee.ServiceStack/ServiceManager.cs:420
  at Banshee.Dap.DapSource.PurgeTemporaryPlaylists () [0x00001] in /home/knocte1404/banshee/src/Dap/Banshee.Dap/Banshee.Dap/DapSource.cs:136
  at Banshee.Dap.DapSource.Initialize () [0x00002] in /home/knocte1404/banshee/src/Dap/Banshee.Dap/Banshee.Dap/DapSource.cs:186
  at Banshee.Dap.Mtp.MtpSource.DeviceInitialize (Banshee.Hardware.IDevice) [0x001bb] in /home/knocte1404/banshee/src/Dap/Banshee.Dap.Mtp/Banshee.Dap.Mtp/MtpSource.cs:128
  at Banshee.Dap.DapService.FindDeviceSource (Banshee.Hardware.IDevice) [0x0002b] in /home/knocte1404/banshee/src/Dap/Banshee.Dap/Banshee.Dap/DapService.cs:156
  at Banshee.Dap.DapService/MapDeviceJob.Run () [0x000e7] in /home/knocte1404/banshee/src/Dap/Banshee.Dap/Banshee.Dap/DapService.cs:212
  at Banshee.Kernel.Scheduler.ProcessJobThread () [0x0008b] in /home/knocte1404/banshee/src/Core/Banshee.Core/Banshee.Kernel/Scheduler.cs:233
  at System.Threading.Thread.StartInternal () <IL 0x00021, 0x00088>
  at (wrapper runtime-invoke) object.runtime_invoke_void__this__ (object,intptr,intptr,intptr) <IL 0x0004e, 0xffffffff>


"Threadpool worker" tid=0x0x2b55c3f30700 this=0x0x2b559da0fbc8 thread handle 0x67f state : interrupted state owns ()
  at <unknown> <0xffffffff>
  at (wrapper managed-to-native) Mono.Unix.Native.Syscall.read (int,intptr,ulong) <IL 0x00019, 0xffffffff>
  at Mono.Unix.Native.Syscall.read (int,void*,ulong) <IL 0x00008, 0x0003b>
  at Mono.Unix.UnixStream.Read (byte[],int,int) <IL 0x00042, 0x000a7>
  at NDesk.DBus.Connection.ReadMessage () <IL 0x00014, 0x00072>
  at NDesk.DBus.Connection.Iterate () <IL 0x0000c, 0x0006f>
  at Mono.Zeroconf.Providers.AvahiDBus.DBusManager.IterateThread (object) <IL 0x00033, 0x000b3>
  at (wrapper runtime-invoke) <Module>.runtime_invoke_void__this___object (object,intptr,intptr,intptr) <IL 0x00052, 0xffffffff>


"Main Thread" tid=0x0x2b55994ad580 this=0x0x2b559da00010 thread handle 0x403 state : waiting on 0x6a0 : Sem  owns ()
  at <unknown> <0xffffffff>
  at (wrapper managed-to-native) System.Threading.Monitor.try_enter_with_atomic_var (object,int,bool&) <IL 0x0000f, 0xffffffff>
  at System.Threading.Monitor.TryEnter (object,int,bool&) <IL 0x00044, 0x00063>
  at System.Threading.Monitor.Enter (object,bool&) <IL 0x00003, 0x0003b>
  at (wrapper unknown) System.Threading.Monitor.FastMonitorEnterV4 (object,bool&) <IL 0x00059, 0xffffffff>
  at Banshee.Dap.DapService.Dispose () [0x00010] in /home/knocte1404/banshee/src/Dap/Banshee.Dap/Banshee.Dap/DapService.cs:127
  at Banshee.ServiceStack.ServiceManager.Shutdown () [0x0002a] in /home/knocte1404/banshee/src/Core/Banshee.Services/Banshee.ServiceStack/ServiceManager.cs:297
  at Banshee.ServiceStack.Application.Dispose () [0x0000c] in /home/knocte1404/banshee/src/Core/Banshee.Services/Banshee.ServiceStack/Application.cs:255
  at Banshee.ServiceStack.Application.Shutdown () [0x0006b] in /home/knocte1404/banshee/src/Core/Banshee.Services/Banshee.ServiceStack/Application.cs:128
  at Banshee.Gui.GlobalActions.OnQuit (object,System.EventArgs) [0x00001] in /home/knocte1404/banshee/src/Core/Banshee.ThickClient/Banshee.Gui/GlobalActions.cs:209
  at (wrapper runtime-invoke) <Module>.runtime_invoke_void__this___object_object (object,intptr,intptr,intptr) <IL 0x0005c, 0xffffffff>
  at <unknown> <0xffffffff>
  at (wrapper managed-to-native) System.Reflection.MonoMethod.InternalInvoke (System.Reflection.MonoMethod,object,object[],System.Exception&) <IL 0x0001c, 0xffffffff>
  at System.Reflection.MonoMethod.Invoke (object,System.Reflection.BindingFlags,System.Reflection.Binder,object[],System.Globalization.CultureInfo) <IL 0x0005a, 0x000eb>
  at System.Reflection.MethodBase.Invoke (object,object[]) <IL 0x00006, 0x00047>
  at System.Delegate.DynamicInvokeImpl (object[]) <IL 0x000dc, 0x00213>
  at System.MulticastDelegate.DynamicInvokeImpl (object[]) <IL 0x0001a, 0x0004f>
  at System.Delegate.DynamicInvoke (object[]) <IL 0x00002, 0x00037>
  at GLib.Signal.ClosureInvokedCB (object,GLib.ClosureInvokedArgs) [0x00039] in /home/knocte/Documents/Code/OpenSource/gtk-sharpINSTALLED/glib/Signal.cs:200
  at GLib.SignalClosure.Invoke (GLib.ClosureInvokedArgs) [0x00019] in /home/knocte/Documents/Code/OpenSource/gtk-sharpINSTALLED/glib/SignalClosure.cs:126
  at GLib.SignalClosure.MarshalCallback (intptr,intptr,uint,intptr,intptr,intptr) [0x00084] in /home/knocte/Documents/Code/OpenSource/gtk-sharpINSTALLED/glib/SignalClosure.cs:154
  at (wrapper native-to-managed) GLib.SignalClosure.MarshalCallback (intptr,intptr,uint,intptr,intptr,intptr) <IL 0x00028, 0xffffffff>
  at <unknown> <0xffffffff>
  at (wrapper managed-to-native) Gtk.Application.gtk_main () <IL 0x0000e, 0xffffffff>
  at Gtk.Application.Run () [0x00001] in /home/knocte/Documents/Code/OpenSource/gtk-sharpINSTALLED/gtk/Application.cs:133
  at Banshee.Gui.GtkBaseClient.Run () [0x00019] in /home/knocte1404/banshee/src/Core/Banshee.ThickClient/Banshee.Gui/GtkBaseClient.cs:225
  at Banshee.Gui.GtkBaseClient.Startup () [0x00010] in /home/knocte1404/banshee/src/Core/Banshee.ThickClient/Banshee.Gui/GtkBaseClient.cs:79
  at Hyena.Gui.CleanRoomStartup.Startup (Hyena.Gui.CleanRoomStartup/StartupInvocationHandler) [0x00050] in /home/knocte1404/banshee/src/Hyena/Hyena.Gui/Hyena.Gui/CleanRoomStartup.cs:54
  at Banshee.Gui.GtkBaseClient.Startup<T> () [0x00049] in /home/knocte1404/banshee/src/Core/Banshee.ThickClient/Banshee.Gui/GtkBaseClient.cs:74
  at Banshee.Gui.GtkBaseClient.Startup<T> (string[]) [0x00021] in /home/knocte1404/banshee/src/Core/Banshee.ThickClient/Banshee.Gui/GtkBaseClient.cs:64
  at Nereid.Client.Main (string[]) [0x00002] in /home/knocte1404/banshee/src/Clients/Nereid/Nereid/Client.cs:54
  at (wrapper runtime-invoke) <Module>.runtime_invoke_void_object (object,intptr,intptr,intptr) <IL 0x00050, 0xffffffff>


I will test again in a sec...

(In reply to comment #21)
> I will test again in a sec...

Next times I tested I couldn't reproduce the deadlock above again...

Now, let me state much clearerly the possible three scenarios:

1) Connect Nexus7 while banshee is not running. Result: nautilus opens a new window showing the device content (mtp://[usb:001,012]/).

2) Connect Nexus7 while banshee (not patched) is running. Result: Banshee doesn't show the device, and a nautilus dialog opens up saying (title: This location could not be displayed) '“mtp:host=%5Busb%3A001%2C013%5D” could not be found. Perhaps it has recently been deleted.'

[Tried the above (2) a second time, and I get different dialog: (title: Oops! Something went wrong.) 'Unhandled error message: No such interface 'org.gtk.vfs.Mount' on object at path /org/gtk/vfs/mount/1'

3) Connect Nexus7 while banshee (patched) is running. Result: nautilus opens 1 new dialog (title: Unable to mount Nexus 7) saying "Unable to open MTP device '[usb:001,015]'", and after some seconds, Banshee shows the device.

So, I think that I will commit your new patch once you update it to git master and address the concerns I raised.

However, after committing that patch, and marking this bug as FIXED, we will need to open 2 new bugs:
a) the fact that nautilus opens the dialog (title: Unable to mount Nexus 7) saying "Unable to open MTP device '[usb:001,015]'" is still unacceptable. I guess we should not mount the device by default (if it's an MTP device), because if we do, we steal it, so we should probably ask the user what to do (a modal dialog similar to the one that banshee has now when it is about to delete more than 10 tracks in the upcoming sync).
b) right now banshee shows my nexus7, but doesn't show the contents. It's stuck in a informative black message that says "Loading Nexus7 - 2358 of 2484". :(

Created attachment 277895 [details] [review]
Revision 2

Lots of fun with git am :)

My foo isn't strong enough to produce two patches which both apply without having to do a 3-way though, so first up is the patch to correct the problem described here. I'll follow the code style in another bug, in addition to the two others we need to file.

Created attachment 277897 [details] [review]
Revision 2.1

Missed a single character variable in the last one... Sorry!

I've splitted your last patch in two patches, tested them (with both Nexus7 and NokiaN900) and committed&pushed them.

Thanks for your hard work on this!

(That being said, Banshee now sees the device, but doesn't see the songs in it, for me, so let's work on that next; we should open a new bug.)

Hey Nicholas.

Today I found a bug, which I filed as bug 751746, and which seems related to your commit (https://git.gnome.org/browse/banshee/commit/?id=5a5cb82394519779c4ba641b7e993f4015714417) because you introduce in this patch the call to "volume.Mount()", however after it happens, volume.MountPoint is still null for me with this device :(

I've committed a fix for it so it doesn't throw more exceptions in the log (because this is probably an Mtp device anyway), but, is it the correct fix? Should I log a warning? Should I throw a different exception?

You're welcome to follow-up here or on the other bug, if you have time these days. Thanks!