After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 731916 - Banshee Should Not Steal MTP Mounts
Banshee Should Not Steal MTP Mounts
Status: RESOLVED WONTFIX
Product: banshee
Classification: Other
Component: Device - MTP
git master
Other Linux
: Normal normal
: ---
Assigned To: Banshee Maintainers
Banshee Maintainers
gnome[unmaintained]
Depends on:
Blocks: 723695
 
 
Reported: 2014-06-19 15:26 UTC by Nicholas Little
Modified: 2020-03-17 09:55 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Defer MTP Initialisation and allow Mapping of Unavailable Devices (13.47 KB, patch)
2014-06-28 01:12 UTC, Nicholas Little
none Details | Review
InactiveSource contents view for DAP with MTP and Mass Storage Support (18.00 KB, patch)
2014-07-03 17:21 UTC, Nicholas Little
none Details | Review
InactiveSource screenshot (64.11 KB, image/png)
2014-07-03 17:23 UTC, Nicholas Little
  Details
Dap Properties Set Behaviour (2.05 KB, patch)
2014-07-04 14:29 UTC, Nicholas Little
none Details | Review
Dictionary Alternative (2.90 KB, patch)
2014-07-04 16:45 UTC, Nicholas Little
none Details | Review
Prevents mount stealing (2.79 KB, patch)
2014-07-05 11:12 UTC, Nicholas Little
none Details | Review
Null Source (11.12 KB, patch)
2014-07-05 17:29 UTC, Nicholas Little
none Details | Review
Remove Unused variable, set property virtual (1.17 KB, patch)
2014-07-06 11:41 UTC, Nicholas Little
committed Details | Review
Connect Dap Action (aka Remedy Button) (11.89 KB, patch)
2014-07-06 12:00 UTC, Nicholas Little
none Details | Review
Cleanup and slight rework (19.59 KB, patch)
2014-07-18 03:21 UTC, Nicholas Little
none Details | Review
bad upload of tracks, screenshot (282.09 KB, image/png)
2014-07-22 13:34 UTC, Andrés G. Aragoneses (IRC: knocte)
  Details
Cleanup and slight rework V2 (24.08 KB, patch)
2014-07-23 15:32 UTC, Nicholas Little
none Details | Review
Cleanup and slight rework V3 (20.97 KB, patch)
2014-07-29 15:01 UTC, Nicholas Little
needs-work Details | Review
New "Device Unavailable" text (60.25 KB, image/png)
2014-07-29 15:05 UTC, Nicholas Little
  Details
Cleanup and slight rework V4 (22.49 KB, patch)
2014-07-30 14:09 UTC, Nicholas Little
needs-work Details | Review
Cleanup and slight rework V5 (30.70 KB, patch)
2014-07-30 19:26 UTC, Nicholas Little
none Details | Review
Cleanup and slight rework V6 (30.59 KB, patch)
2014-07-31 14:31 UTC, Nicholas Little
committed Details | Review
Mtp CheckErrorStack on Battery Status Retrieval error (1.09 KB, patch)
2014-07-31 14:32 UTC, Nicholas Little
none Details | Review
Publicise RequestUnmap (1.10 KB, patch)
2014-08-08 15:06 UTC, Nicholas Little
none Details | Review

Description Nicholas Little 2014-06-19 15:26:19 UTC
When Banshee sees a new mount point for an MTP device, it causes an unmount in its initialisation phase. While necessary to use the device, the behaviour leads to some unusual error messages from nautilus e.g.

"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."

Also,

4) With banshee running, and the device connected + recognised by banshee. Mount the device from nautilus. Behaviour is the same as 3.

Prior to BGO#729438, solved with commit 5a5cb82394519779c4ba641b7e993f4015714417, devices were subject to two events, addition and removal. Now that MTP devices are recognised reliably we can look at handling the new 'changed' event correctly for the above cases.
Comment 1 Andrés G. Aragoneses (IRC: knocte) 2014-06-20 14:18:41 UTC
Actually, since your commit 5a5cb82394519779c4ba641b7e993f4015714417 no error message from nautilus shows up for me, and I still can browse the contents in nautilus (and I guess this is the reason why bug 731920 happens).

So my guess is that, whenever you fix bug 731920, then this bug will reappear :)
Comment 2 Nicholas Little 2014-06-28 01:12:08 UTC
Created attachment 279457 [details] [review]
Defer MTP Initialisation and allow Mapping of Unavailable Devices

It's a diff, so that should immediately tell you this is not push ready yet.

That said, with this applied you should be able to plug in an MTP device and if nautilus manages to automount before Banshee, we'll ignore it, otherwise the user may get one of the messages mentioned above when nautilus gets around to doing what it does...

Furthermore, if Banshee does connect through libmtp you should be able to use the disconnect button and be able to mount the device in nautilus, you may need to change path in nautilus to get it to actually do the mount though. 

At which point you should see a very ragged "Device x Unavailable..." screen with a little warning sign replace the usual sync preferences display, if you unmount in nautilus at that point Banshee should pick up the device and reload it's contents.

You should be able to repeat this process as many times as you like, but; if the user tries to mount via nautilus without disconnecting us first, I haven't tested that path vigorously-ideally we'd be able to hook into something that would tell us if another gvfs client was trying to mount and act accordingly by e.g. suspending transfers

I say this isn't push ready because I need to take a closer look at the mass storage side to implement something similar, in addition to libgpod and karma devices. Finally, the "Device Unavailable..." screen obviously needs polish.
Comment 3 Andrés G. Aragoneses (IRC: knocte) 2014-06-28 01:16:12 UTC
So, is the fix for this bug a pre-requisite for fixing bug 731920? I'm a bit lost now.
Comment 4 Nicholas Little 2014-06-28 01:34:24 UTC
Yes, if the assumption below is correct; in bug 731920 you mention the Nexus was actually mounted and you could browse through nautilus, banshee just didn't show any tracks.

I'm not sure what could cause that - my assumption is we got a connection, then the mount happened and stole it from us, but we'd already mapped it and were left with a dead device handle after doing the initial database purge during the load.

I wasn't all too clear, with this applied whenever the device is not accessible to banshee but still present, we'll have a source item for it if we would have usually mapped it, but in the "unavailable" state, when it becomes usable again Banshee should reconnect and load the information from it just as if it was a new connection-only we re-use the existing DapSource.
Comment 5 Nicholas Little 2014-07-03 17:21:07 UTC
Created attachment 279857 [details] [review]
InactiveSource contents view for DAP with MTP and Mass Storage Support

Would you mind testing this prototype?

The only action I couldn't figure out how to disable off the top of my head was "Import to Library"

Since pictures paint a thousand words, I'll defer comment, except to say that behaviour is the same as described above, in lieu of the following screenshot :)

Would you mind reviewing this? It's a bit big I know, but a lot of it is GUI, and it would have only been bigger if I had tried to extract the common components between the new component and its Active brother into their parent.
Comment 6 Nicholas Little 2014-07-03 17:23:30 UTC
Created attachment 279858 [details]
InactiveSource screenshot

Should look exactly like this, but with the "Disconnect" button also disabled.
Comment 7 Nicholas Little 2014-07-04 14:29:16 UTC
Created attachment 279915 [details] [review]
Dap Properties Set Behaviour

While working on the above patch, with multiple device connects/reconnects I started to notice the device properties display getting longer after each cycle.

This patch ensures that the above behaviour doesn't occur by switching to a Set of properties instead of List which I feel is the correct representation.
Comment 8 Andrés G. Aragoneses (IRC: knocte) 2014-07-04 14:35:15 UTC
> From 15ae7712f13942805f32b34a847cfba1a0cde61b Mon Sep 17 00:00:00 2001
> From: Nicholas Little <arealityfarbetween@googlemail.com>
> Date: Fri, 4 Jul 2014 15:14:39 +0100
> Subject: [PATCH] Dap: Properties are a Set, not a Sequence
> 
> Using a sequence works fine now, since we only perform device
> initialisation once. But if we would like to do this multiple times, we
> must ensure that we don't add duplicates. This patch adds the methods to
> DapProperty to support that, in addition to ensuring the Name property
> does not change after construction.
> ---
>  src/Dap/Banshee.Dap/Banshee.Dap/DapSource.cs | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/src/Dap/Banshee.Dap/Banshee.Dap/DapSource.cs b/src/Dap/Banshee.Dap/Banshee.Dap/DapSource.cs
> index 2f1cabc..2708a0a 100644
> --- a/src/Dap/Banshee.Dap/Banshee.Dap/DapSource.cs
> +++ b/src/Dap/Banshee.Dap/Banshee.Dap/DapSource.cs
> @@ -385,15 +385,28 @@ namespace Banshee.Dap
>          }
>  
>          public struct DapProperty {
> -            public string Name;
> +            public readonly string Name;

Why not marking Value as readonly too? I think it's valuable.

>              public string Value;
>              public DapProperty (string k, string v) { Name = k; Value = v; }
> +            public override int GetHashCode ()
> +            {
> +                return Name.GetHashCode ();

I don't think that not taking in account the Value property here is correct.

But anyway, thinking more about it, I think that the fact that this thing is a struct (instead of a class) makes it already override equality operators properly without the need of overriding Equals() and GetHashCode().

> +            }
> +            public override bool Equals (object obj)
> +            {
> +                return obj is DapProperty &&
> +                    Name == ((DapProperty)obj).Name;
> +            }
>          }
>  
> -        private List<DapProperty> dap_properties = new List<DapProperty> ();
> +        private ISet<DapProperty> dap_properties = new HashSet<DapProperty> ();
>          protected void AddDapProperty (string key, string val)
>          {
> -            dap_properties.Add (new DapProperty (key, val));
> +            var prop = new DapProperty (key, val);
> +            if (dap_properties.Contains (prop)) {

If it's a HashSet, you don't need to check with Contains() first, because if the property is already there, then the method Add() will be a no-op.

> +                dap_properties.Remove (prop);
> +            }
> +            dap_properties.Add (prop);
>          }
>  
>          protected void AddYesNoDapProperty (string key, bool val)
> -- 
> 1.8.5.5
>
Comment 9 Nicholas Little 2014-07-04 14:48:57 UTC
(In reply to comment #8)
> > From 15ae7712f13942805f32b34a847cfba1a0cde61b Mon Sep 17 00:00:00 2001
> > From: Nicholas Little <arealityfarbetween@googlemail.com>
> > Date: Fri, 4 Jul 2014 15:14:39 +0100
> > Subject: [PATCH] Dap: Properties are a Set, not a Sequence
> > 
> > Using a sequence works fine now, since we only perform device
> > initialisation once. But if we would like to do this multiple times, we
> > must ensure that we don't add duplicates. This patch adds the methods to
> > DapProperty to support that, in addition to ensuring the Name property
> > does not change after construction.
> > ---
> >  src/Dap/Banshee.Dap/Banshee.Dap/DapSource.cs | 19 ++++++++++++++++---
> >  1 file changed, 16 insertions(+), 3 deletions(-)
> > 
> > diff --git a/src/Dap/Banshee.Dap/Banshee.Dap/DapSource.cs b/src/Dap/Banshee.Dap/Banshee.Dap/DapSource.cs
> > index 2f1cabc..2708a0a 100644
> > --- a/src/Dap/Banshee.Dap/Banshee.Dap/DapSource.cs
> > +++ b/src/Dap/Banshee.Dap/Banshee.Dap/DapSource.cs
> > @@ -385,15 +385,28 @@ namespace Banshee.Dap
> >          }
> >  
> >          public struct DapProperty {
> > -            public string Name;
> > +            public readonly string Name;
> 
> Why not marking Value as readonly too? I think it's valuable.

Because we want the value to change from time to time. e.g. When I first plug in my S2 its name is SAMSUNG_Android but after connection that can be updated to whatever MTP says its name is (GT-I9100 in my case but the user may have set something custom)

A dictionary is a more natural structure than a set in fact, but since we want to provide an IEnumerable<DapProperty> I elected a set to avoid the single liner earlier on in the class and also to avoid storing the key in two places.

> >              public string Value;
> >              public DapProperty (string k, string v) { Name = k; Value = v; }
> > +            public override int GetHashCode ()
> > +            {
> > +                return Name.GetHashCode ();
> 
> I don't think that not taking in account the Value property here is correct.

If you take into account the value property then DapProperty's which exist but are read with a different value will be considered non-equal and added to the set. Which is not what we want.

> But anyway, thinking more about it, I think that the fact that this thing is a
> struct (instead of a class) makes it already override equality operators
> properly without the need of overriding Equals() and GetHashCode().

No, it doesn't. DapProperty is actually a KeyValuePair, which have equality and hash defined only on the key value.

> > +            }
> > +            public override bool Equals (object obj)
> > +            {
> > +                return obj is DapProperty &&
> > +                    Name == ((DapProperty)obj).Name;
> > +            }
> >          }
> >  
> > -        private List<DapProperty> dap_properties = new List<DapProperty> ();
> > +        private ISet<DapProperty> dap_properties = new HashSet<DapProperty> ();
> >          protected void AddDapProperty (string key, string val)
> >          {
> > -            dap_properties.Add (new DapProperty (key, val));
> > +            var prop = new DapProperty (key, val);
> > +            if (dap_properties.Contains (prop)) {
> 
> If it's a HashSet, you don't need to check with Contains() first, because if
> the property is already there, then the method Add() will be a no-op.

That's why we need the contains, if the add is a no-op then we need to remove the old value to place the new one.

if it were a map we could just do:

dap_properties [key] = new DapProperty(key, val);

> > +                dap_properties.Remove (prop);
> > +            }
> > +            dap_properties.Add (prop);
> >          }
> >  
> >          protected void AddYesNoDapProperty (string key, bool val)
> > -- 
> > 1.8.5.5
> >
Comment 10 Andrés G. Aragoneses (IRC: knocte) 2014-07-04 14:53:13 UTC
(In reply to comment #9)
> Because we want the value to change from time to time. e.g. When I first plug
> in my S2 its name is SAMSUNG_Android but after connection that can be updated
> to whatever MTP says its name is (GT-I9100 in my case but the user may have set
> something custom)

After connection? I'm guessing that was a typo? If I'm guessing right, you mean that the MtpSource is not removing its DapProperties at reconnection, which maybe is what we want to fix (instead of implementing a hacky way of replacing DapProperties), no?
Comment 11 Nicholas Little 2014-07-04 15:38:39 UTC
(In reply to comment #10)
> (In reply to comment #9)
> > Because we want the value to change from time to time. e.g. When I first plug
> > in my S2 its name is SAMSUNG_Android but after connection that can be updated
> > to whatever MTP says its name is (GT-I9100 in my case but the user may have set
> > something custom)
> 
> After connection? I'm guessing that was a typo? If I'm guessing right, you mean
> that the MtpSource is not removing its DapProperties at reconnection, which
> maybe is what we want to fix (instead of implementing a hacky way of replacing
> DapProperties), no?

Not a typo. When I connect, the first name that's available is the name displayed by nautilus: SAMSUNG_Android.

If I connect with automount enabled then sometimes (about 50/50) nautilus beats banshee to the connection and mounts the device. When that happens all banshee has available is what I assume are the gvfs names.

After unmounting the device in nautilus and triggering the device changed signal banshee fires up the MTP connection, at which point I get the friendly name, battery level, IMEI, etc.

We could swap it out for a dictionary, but then clients would have to change to use Key instead of Name and KeyValuePair instead of DapProperty.
Comment 12 Nicholas Little 2014-07-04 15:43:10 UTC
One the releasing properties side, we might want to remove some but definitely not all.

For example, it's a good thing that when I plug in I see the nautilus name which is then replaced with the friendly one after the MTP connection occurs-at the moment that persists so if the device is disconnected then I get the "Device Unavailable..." contents but still the friendly name. 

However, things like battery percentage should probably get removed upon the eject call.
Comment 13 Nicholas Little 2014-07-04 16:45:31 UTC
Created attachment 279923 [details] [review]
Dictionary Alternative

Here's the Dictionary version, I don't think its ideal since we lose the semantic hint that the DapProperty type gave us.

I was wrong about the KeyValuePair type's equality and hashcode though. A short test program showed me that.

Perhaps in the other solution we could switch DapProperty to a class with the custom operations-so that future clients won't expect ValueType behaviour when invoking Equals or GetHashCode.
Comment 14 Andrés G. Aragoneses (IRC: knocte) 2014-07-04 17:00:24 UTC
(In reply to comment #12)
> However, things like battery percentage should probably get removed upon the
> eject call.

Well, after an eject call, that DapSource object should just go die, we should not reuse it ever after, so asking if the properties should stay or go at that point is wrong.
Comment 15 Nicholas Little 2014-07-04 19:00:47 UTC
(In reply to comment #14)
> (In reply to comment #12)
> > However, things like battery percentage should probably get removed upon the
> > eject call.
> 
> Well, after an eject call, that DapSource object should just go die, we should
> not reuse it ever after, so asking if the properties should stay or go at that
> point is wrong.

I'd like to try and convince you otherwise. For example, with an MTP device we can dispose of the connection, which allows nautilus to use it. However, if the user just wanted to connect back to banshee there's no way to do that unless they disconnect the cable or mount and unmount in nautilus.

Using the null object pattern for recognised but unmappable devices is an alternative idea, but it's pretty difficult to do with the current DAP architecture, since the source types are created by Banshee, instead of factory produced.

The simplest fix for this bug then is to try and connect once, and if that fails or the device is mounted throw an InvalidDeviceException.

The problem with that approach is that the user gets absolutely no feedback unless they're running banshee from a terminal.
Comment 16 Nicholas Little 2014-07-05 11:12:55 UTC
Created attachment 279945 [details] [review]
Prevents mount stealing

This version contains the smallest set of changes required to prevent mount stealing.
Comment 17 Nicholas Little 2014-07-05 17:29:28 UTC
Created attachment 279954 [details] [review]
Null Source

Your comments got me thinking, as it was indeed difficult to reconfigure a 'live' source so that it was all but disabled.

With the above patch applied titled "Prevents mount stealing", this one builds a little by adding the InactiveContent and a NullDapSource to house it in. If the hardware state changes we just unmap it and try the mapping procedure again.

What do you think? If it's promising I'll continue with this line of attack. It's probably more appropriate to open another bug for if so though, no?
Comment 18 Nicholas Little 2014-07-06 11:41:51 UTC
Created attachment 279983 [details] [review]
Remove Unused variable, set property virtual

I'd noted the Unmap property of RemovableSource previously, the private sync variable isn't used and it looks more like that property should be virtual so here's a little patch to apply that change. It's used in the next one so it's not just me rambling ;)

Also, I've marked the first two files obsolete, I'm really liking this new approach!
Comment 19 Nicholas Little 2014-07-06 12:00:54 UTC
Created attachment 279984 [details] [review]
Connect Dap Action (aka Remedy Button)

And here's something cool we can do with the null source.

I call it, The Remedy Button :)

For MTP: volume.Unmount()
For MassStorage: volume.Mount()

In both cases, when the device state changes and it's usable banshee gets a signal, maps the new source and unmaps the null one.

A short rationale for the MassStorage version of this; users with automount disabled might plug in a usb stick and want to do an fsck or work with the partition table, so we probably shouldn't mount automatically even then.
Comment 20 Andrés G. Aragoneses (IRC: knocte) 2014-07-11 07:58:40 UTC
I'm slightly concerned about the terminology here:

a) Remedy? That seems to hint that there is some kind of "problem". But there is not, it's just that the device is mounted by nautilus, and banshee lets you steal (on demand, thanks to your patch, AFAIU) the mount, right? So how about something like "Claim" instead of "Remedy"?

b) Not sure I like the name NullDapSource, in the source code. It doesn't give enough background to the future developers about what it is, shouldn't we name it something like PotentialDapSource or NotReadyDapSource?
Comment 21 Nicholas Little 2014-07-11 10:21:06 UTC
(In reply to comment #20)
> I'm slightly concerned about the terminology here:
> 
> a) Remedy? That seems to hint that there is some kind of "problem". But there
> is not, it's just that the device is mounted by nautilus, and banshee lets you
> steal (on demand, thanks to your patch, AFAIU) the mount, right? So how about
> something like "Claim" instead of "Remedy"?

That's exactly right.

"Remedy" was just to introduce the idea in this thread, the stock Connect action seems appropriate, but Claim is in line with what I had in mind for the new architecture so I'm equally happy either way.

> b) Not sure I like the name NullDapSource, in the source code. It doesn't give
> enough background to the future developers about what it is, shouldn't we name
> it something like PotentialDapSource or NotReadyDapSource?

Sure thing, I just took "Null" from the name of the design pattern :) But, it has a little bit of functionality with the latest patch so Potential sounds good, but NotReady looks better in CamelCase, oooh decisions!

Perhaps just PotentialSource? Probably ok to drop the Dap if we make it internal to the Dap assembly, I can't think of a reason why it would need to be visible elsewhere.
Comment 22 Andrés G. Aragoneses (IRC: knocte) 2014-07-17 20:28:43 UTC
(In reply to comment #21)
> (In reply to comment #20)
> > I'm slightly concerned about the terminology here:
> > 
> > a) Remedy? That seems to hint that there is some kind of "problem". But there
> > is not, it's just that the device is mounted by nautilus, and banshee lets you
> > steal (on demand, thanks to your patch, AFAIU) the mount, right? So how about
> > something like "Claim" instead of "Remedy"?
> 
> That's exactly right.
> 
> "Remedy" was just to introduce the idea in this thread, the stock Connect
> action seems appropriate, but Claim is in line with what I had in mind for the
> new architecture so I'm equally happy either way.
> 
> > b) Not sure I like the name NullDapSource, in the source code. It doesn't give
> > enough background to the future developers about what it is, shouldn't we name
> > it something like PotentialDapSource or NotReadyDapSource?
> 
> Sure thing, I just took "Null" from the name of the design pattern :) But, it
> has a little bit of functionality with the latest patch so Potential sounds
> good, but NotReady looks better in CamelCase, oooh decisions!
> 
> Perhaps just PotentialSource? Probably ok to drop the Dap if we make it
> internal to the Dap assembly, I can't think of a reason why it would need to be
> visible elsewhere.

Great!

Any particular reason you have split the patch in 2 (one which adds the NullDapSource and the other that adds the remedy button)? I actually tried to apply the first (success) and the second didn't apply.

If you're going to fix this and merge them in just 1, also take the opportunity to rename to PotentialSource and to "Claim" instead of Remedy please (and use also Catalog.GetString ("Claim") instead of Catalog.GetString ("Connect")).

Also remove this whitespace changes please:
         public override bool CanImport {
-            get {
-                return false;
-            }
+            get { return false; }
         }
 
         public override bool IsReadOnly {
-            get {
-                return true;
-            }
+            get { return true; }
         }
 
         public override long BytesUsed {
-            get {
-                return 0L;
-            }
+            get { return 0L; }
         }
 
         public override long BytesCapacity {
-            get {
-                return 0L;
-            }
+            get { return 0L; }
         }
Comment 23 Nicholas Little 2014-07-17 20:54:48 UTC
(In reply to comment #22)
> (In reply to comment #21)
> > (In reply to comment #20)
> > > I'm slightly concerned about the terminology here:
> > > 
> > > a) Remedy? That seems to hint that there is some kind of "problem". But there
> > > is not, it's just that the device is mounted by nautilus, and banshee lets you
> > > steal (on demand, thanks to your patch, AFAIU) the mount, right? So how about
> > > something like "Claim" instead of "Remedy"?
> > 
> > That's exactly right.
> > 
> > "Remedy" was just to introduce the idea in this thread, the stock Connect
> > action seems appropriate, but Claim is in line with what I had in mind for the
> > new architecture so I'm equally happy either way.
> > 
> > > b) Not sure I like the name NullDapSource, in the source code. It doesn't give
> > > enough background to the future developers about what it is, shouldn't we name
> > > it something like PotentialDapSource or NotReadyDapSource?
> > 
> > Sure thing, I just took "Null" from the name of the design pattern :) But, it
> > has a little bit of functionality with the latest patch so Potential sounds
> > good, but NotReady looks better in CamelCase, oooh decisions!
> > 
> > Perhaps just PotentialSource? Probably ok to drop the Dap if we make it
> > internal to the Dap assembly, I can't think of a reason why it would need to be
> > visible elsewhere.
> 
> Great!
> 
> Any particular reason you have split the patch in 2 (one which adds the
> NullDapSource and the other that adds the remedy button)? I actually tried to
> apply the first (success) and the second didn't apply.

I didn't, I committed the first and then worked from it to get the last two. IIRC, another reason is that the last one has a behaviour change for mass storage devices which we probably shouldn't commit here.

> If you're going to fix this and merge them in just 1, also take the opportunity
> to rename to PotentialSource and to "Claim" instead of Remedy please (and use
> also Catalog.GetString ("Claim") instead of Catalog.GetString ("Connect")).

Roger wilko; I'll come back to this one in the morning, isolate the MTP changes and produce a clean version for you :)

> Also remove this whitespace changes please:
>          public override bool CanImport {
> -            get {
> -                return false;
> -            }
> +            get { return false; }
>          }
<snip />

I do get a bee in my bonnet sometimes. Sorry about that.
Comment 24 Nicholas Little 2014-07-18 03:21:48 UTC
Created attachment 281052 [details] [review]
Cleanup and slight rework

In addition to the changes you asked for I realised that the last version wasn't handling the case where a Claim results in a successful DeviceInitialize call.

I know that might sound confusing but I've been trying to write a satisfactory explanation for what I mean the past half hour and I'm ready to give up.

One last bash, as a sequence of events:

1. DapService sees a device, tries mapping, fails and maps a PotentialSource
2. User clicks Claim
3. PotentialSource creates a new DapSource but has to call DeviceInitialize on it to get enough state into it for the Claim call to work (i.e. the mtp_volume in MtpSource)

The last step is tricky.

If DeviceInitialize succeeds: I added a method, SwapSource, to DapService allowing the new, active source to replace the potential one.
If DeviceInitialize fails: That's when Claim is called on the new source but then it's simply discarded though we generally receive a hardware change event after (or possibly during) the operation that lets us start using the device.

Catching you on IRC is probably best ;)

Last, I removed the MassStorageSource modifications.
Comment 25 Andrés G. Aragoneses (IRC: knocte) 2014-07-22 12:51:10 UTC
Great thanks for the patch!

I think there are 2 things to fix before we commit this:

1) The "Claim" action is only available on the context menu, but it should also be next to the "Device unavailable..." label, when you click on the source, in the form of a button.

2) The patch unfortunately doesn't work for me (tested with Nexus7). This is the log when launching banshee (the device appeared as a source, as unavailable, then I clicked Claim, and then it disappeared):

[1 Debug 14:46:43.910] (libbanshee:player) Using system (gst-plugins-good) equalizer element
[1 Debug 14:46:43.941] Player state change: NotReady -> Ready
[1 Debug 14:46:43.947] Loaded equalizer presets: 0.000346
[1 Debug 14:46:43.951] Selected equalizer: Rock
[1 Debug 14:46:43.959] Player state change: Ready -> Idle
[1 Debug 14:46:43.963] (libbanshee:player) Disabled ReplayGain
[1 Info  14:46:43.965] GStreamer version 1.2.4.0, gapless: True, replaygain: False
[1 Debug 14:46:43.967] Delayed Initializating Banshee.Dap.DapService
[1 Debug 14:46:43.977] Dap support extension loaded: Banshee.Dap.Mtp
[1 Debug 14:46:43.978] Dap support extension loaded: Banshee.Dap.AppleDevice
[1 Debug 14:46:43.979] Dap support extension loaded: Banshee.Dap.MassStorage
[1 Warn  14:46:43.980] Failed to load media-player-info file for 1
[1 Debug 14:46:43.984] Delayed Initializating Banshee.Podcasting.PodcastService
[1 Debug 14:46:44.046] Delayed Initializating Banshee.Daap.DaapService
[8 Debug 14:46:44.049] Refreshing any podcasts that haven't been updated in over an hour
Device 0 (VID=18d1 and PID=4e41) is a Google Inc (for Asus) Nexus 7 (MTP).
[7 Error 14:46:44.060] Dap.DapService: invalid state, mapping potential source for Nexus 7
[1 Debug 14:46:44.104] Gio.Manager: received MountAdded signal
[1 Warn  14:46:44.105] Failed to load media-player-info file for 1
[7 Debug 14:46:44.357] Found DAP support (Banshee.Dap.PotentialSource) for device Nexus 7 and Uuid /devices/pci0000:00/0000:00:14.0/usb3/3-4
[7 Info  14:46:44.358] AppleDeviceSource is ignoring unmounted volume 11 GB Volume
[7 Info  14:46:44.364] AppleDeviceSource is ignoring unmounted volume 16 GB Volume
[7 Info  14:46:44.369] AppleDeviceSource is ignoring unmounted volume Windows8_OS
[1 Debug 14:46:45.049] Finished - Startup Job
[1 Debug 14:46:45.054] Starting - Downloading Cover Art
[12 Debug 14:46:45.059] Finished - Downloading Cover Art
[15 Debug 14:46:45.812] DAAP Proxy listening for connections on port 8089
Device 0 (VID=18d1 and PID=4e41) is a Google Inc (for Asus) Nexus 7 (MTP).
[1 Error 14:47:09.834] Caught an exception - Banshee.Dap.InvalidStateException: An application exception has occurred. (in `Banshee.Dap.Mtp')
  at Banshee.Dap.Mtp.MtpSource.DeviceInitialize (IDevice device) [0x000b8] in /home/knocte1404/bansheeSHINY/src/Dap/Banshee.Dap.Mtp/Banshee.Dap.Mtp/MtpSource.cs:111 
  at Banshee.Dap.PotentialSource.TryCreateSource (Banshee.Dap.DapSource& source) [0x0001f] in /home/knocte1404/bansheeSHINY/src/Dap/Banshee.Dap/Banshee.Dap/PotentialSource.cs:105 
[1 Info  14:47:14.837] Timed out trying to unmount {0} - Nexus 7
[1 Debug 14:47:14.859] Gio.Manager: received MountRemoved signal
[1 Error 14:47:14.859] Gio.Manager: ignoring VolumeChanged signal with no volume
[1 Debug 14:47:14.861] Gio.Manager: received MountRemoved signal
[1 Warn  14:47:14.861] Failed to load media-player-info file for 1
[1 Debug 14:47:14.861] Unmapping DAP source (/devices/pci0000:00/0000:00:14.0/usb3/3-4)
[1 Debug 14:47:14.886] Creating Pango.Layout, configuring Cairo.Context
[17 Info  14:47:14.902] AppleDeviceSource is ignoring unmounted volume Nexus 7
Device 0 (VID=18d1 and PID=4e41) is a Google Inc (for Asus) Nexus 7 (MTP).
Unable to open ~/.mtpz-data for reading, MTPZ disabled.Android device detected, assigning default bug flags
[1 Debug 14:47:14.947] Creating Pango.Layout, configuring Cairo.Context
[1 Debug 14:47:14.958] Creating Pango.Layout, configuring Cairo.Context
[1 Debug 14:47:15.126] Last.fm State Changed to NoAccount
[1 Debug 14:47:18.881] Creating Pango.Layout, configuring Cairo.Context
[1 Debug 14:47:18.917] Creating Pango.Layout, configuring Cairo.Context
[1 Debug 14:47:18.929] Creating Pango.Layout, configuring Cairo.Context
[1 Debug 14:47:20.627] Creating Pango.Layout, configuring Cairo.Context
[17 Error 14:47:39.096] Caught an exception - 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/knocte1404/bansheeSHINY/src/Libraries/Mtp/Mtp/MtpDevice.cs:312 
  at Mtp.MtpDevice.get_BatteryLevel () [0x0000b] in /home/knocte1404/bansheeSHINY/src/Libraries/Mtp/Mtp/MtpDevice.cs:68 
  at Banshee.Dap.Mtp.MtpSource.DeviceInitialize (IDevice device) [0x00254] in /home/knocte1404/bansheeSHINY/src/Dap/Banshee.Dap.Mtp/Banshee.Dap.Mtp/MtpSource.cs:152 
[17 Debug 14:47:39.097] Found DAP support (Banshee.Dap.Mtp.MtpSource) for device Nexus 7 and Uuid /devices/pci0000:00/0000:00:14.0/usb3/3-4
Comment 26 Andrés G. Aragoneses (IRC: knocte) 2014-07-22 12:57:03 UTC
I tested one more time, and this time it appeared (maybe I wasn't patient enough the last time).

However, I clicked on the source and it displayed: loading track 49389 of 309488 (I just made up the numbers, I don't remember them). And it felt it had stopped there. Then, as I was filing this comment, then banshee requested my attention, and apparently it has finished loading, and it told me "the sync operation would remove 128 tracks from your device".

This is interesting, it switched automatically to auto-sync mode even though I don't recall to have used this mode with this device in the past. Are you sure you're not hardcoding this somehow?
Comment 27 Andrés G. Aragoneses (IRC: knocte) 2014-07-22 13:11:11 UTC
Testing also a bit more this patch I've found two other problems:

a) In some cases, banshee is unable to show the songs that the device has. When this happens, banshee is somehow assuming that the device has 0 songs (instead of knowing that the state of its track count is "don't know yet", maybe because it's still loading or other reason) and as a consequence of this (as I had auto-sync configured with this device this time), it has started to sync songs to it, even before calculating the sync properly (i.e.: X number of songs to remove, Y number to add, etc). After the songs were synced, I clicked on the source, and banshee couldn't show any track yet either.

2) I've seen these 2 errors in my log, which maybe are suspects of these buggy behaviours:

[1 Debug 15:06:29.615] Gio.Manager: received MountAdded signal
[1 Error 15:06:29.615] Gio.Manager: ignoring VolumeChanged signal with no volume
[1 Debug 15:06:29.615] Gio.Manager: received MountAdded signal
[1 Warn  15:06:29.616] Failed to load media-player-info file for 1
[38 Error 15:06:29.807] Caught an exception - 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/knocte1404/bansheeSHINY/src/Libraries/Mtp/Mtp/MtpDevice.cs:312 
  at Mtp.MtpDevice.get_BatteryLevel () [0x0000b] in /home/knocte1404/bansheeSHINY/src/Libraries/Mtp/Mtp/MtpDevice.cs:68 
  at Banshee.Dap.Mtp.MtpSource.DeviceInitialize (IDevice device) [0x00254] in /home/knocte1404/bansheeSHINY/src/Dap/Banshee.Dap.Mtp/Banshee.Dap.Mtp/MtpSource.cs:152
Comment 28 Andrés G. Aragoneses (IRC: knocte) 2014-07-22 13:34:04 UTC
Created attachment 281383 [details]
bad upload of tracks, screenshot

I also tried to sync a playlist of 6 items with auto-sync (which meant it had to first delete 128 tracks), and the result is this screenshot (128 files deleted successfully, 6 tracks uploaded *not* successfully).

This is one exception I got from the console:






























(Nereid:13287): GLib-CRITICAL **: Source ID 78666 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78575 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78709 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78731 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78622 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78617 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78593 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78743 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78633 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78513 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78749 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78745 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78597 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78595 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78657 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78629 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78580 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78689 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78587 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78529 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78582 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78747 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78532 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78615 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78741 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78671 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78585 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78750 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78589 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78584 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78672 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78577 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78564 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78691 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78656 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78605 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78621 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78520 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78537 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78531 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78635 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78637 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78619 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78560 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78598 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78720 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78661 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78519 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78710 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78572 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78536 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78630 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78543 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78677 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78562 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78600 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78563 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78574 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78592 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78632 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78558 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78602 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78676 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78594 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78576 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78607 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78623 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78640 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78549 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78744 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78579 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78542 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78746 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78735 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78596 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78548 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78681 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78740 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78581 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78588 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78690 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78583 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78748 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78662 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78616 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78742 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78604 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78586 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78512 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78590 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78547 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78680 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78565 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78692 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78524 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78606 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78724 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78525 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78618 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78559 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78667 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78719 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78650 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78511 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78608 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78514 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78636 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78638 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78620 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78561 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78651 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78599 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78573 was not found when attempting to remove it

(Nereid:13287): GLib-CRITICAL **: Source ID 78728 was not found when attempting to remove it
[53 Error 15:29:29.134] Caught an exception - Mtp.LibMtpException: PTP Layer error 02fe: get_storage_freespace(): could not get storage info. (in `')

Mtp.LibMtpException: Error 02fe: PTP: Protocol error, data expected (in `')

Mtp.LibMtpException: check_if_file_fits(): error checking free storage. (in `')

Mtp.LibMtpException: get_writeable_storageid(): all device storage is full or corrupt. (in `')

Mtp.LibMtpException: PTP Layer error 2008: send_file_object_info(): Could not send object info. (in `')

Mtp.LibMtpException: Error 2008: PTP: Invalid Storage ID (in `')

Mtp.LibMtpException: LIBMTP_Send_Track_From_File_Descriptor(): subcall to LIBMTP_Send_File_From_File_Descriptor failed. (in `Mtp')
  at Mtp.LibMtpException.CheckErrorStack (Mtp.MtpDeviceHandle handle) [0x00071] in /home/knocte1404/bansheeSHINY/src/Libraries/Mtp/Mtp/Error.cs:67 
  at Mtp.Track.SendTrack (Mtp.MtpDeviceHandle handle, System.String path, Mtp.TrackStruct& metadata, Mtp.ProgressFunction callback, IntPtr data) [0x00024] in /home/knocte1404/bansheeSHINY/src/Libraries/Mtp/Mtp/Track.cs:240 
  at Mtp.MtpDevice.UploadTrack (System.String path, Mtp.Track track, Mtp.Folder folder, Mtp.ProgressFunction callback) [0x00065] in /home/knocte1404/bansheeSHINY/src/Libraries/Mtp/Mtp/MtpDevice.cs:253 
  at Banshee.Dap.Mtp.MtpSource.AddTrackToDevice (Banshee.Collection.Database.DatabaseTrackInfo track, Hyena.SafeUri fromUri) [0x0005a] in /home/knocte1404/bansheeSHINY/src/Dap/Banshee.Dap.Mtp/Banshee.Dap.Mtp/MtpSource.cs:348 
  at Banshee.Dap.DapSource.AttemptToAddTrackToDevice (Banshee.Collection.Database.DatabaseTrackInfo track, Hyena.SafeUri fromUri) [0x00070] in /home/knocte1404/bansheeSHINY/src/Dap/Banshee.Dap/Banshee.Dap/DapSource.cs:366 
  at Banshee.Dap.DapSource.AddTrackAndIncrementCount (Banshee.Collection.Database.DatabaseTrackInfo track) [0x00016] in /home/knocte1404/bansheeSHINY/src/Dap/Banshee.Dap/Banshee.Dap/DapSource.cs:411 
  at Banshee.Sources.PrimarySource.AddTrackList (System.Object cached_list) [0x00099] in /home/knocte1404/bansheeSHINY/src/Core/Banshee.Services/Banshee.Sources/PrimarySource.cs:647
Comment 29 Nicholas Little 2014-07-22 13:52:57 UTC
(In reply to comment #25)
> Great thanks for the patch!
> 
> I think there are 2 things to fix before we commit this:
> 
> 1) The "Claim" action is only available on the context menu, but it should also
> be next to the "Device unavailable..." label, when you click on the source, in
> the form of a button.

The claim button should appear at the top of the Source content view next to the disconnect button, not just in the context menu.

> 2) The patch unfortunately doesn't work for me (tested with Nexus7). This is
> the log when launching banshee (the device appeared as a source, as
> unavailable, then I clicked Claim, and then it disappeared):
> 

<snip />

> [17 Error 14:47:39.096] Caught an exception - 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/knocte1404/bansheeSHINY/src/Libraries/Mtp/Mtp/MtpDevice.cs:312 
>   at Mtp.MtpDevice.get_BatteryLevel () [0x0000b] in
> /home/knocte1404/bansheeSHINY/src/Libraries/Mtp/Mtp/MtpDevice.cs:68 
>   at Banshee.Dap.Mtp.MtpSource.DeviceInitialize (IDevice device) [0x00254] in
> /home/knocte1404/bansheeSHINY/src/Dap/Banshee.Dap.Mtp/Banshee.Dap.Mtp/MtpSource.cs:152 
> [17 Debug 14:47:39.097] Found DAP support (Banshee.Dap.Mtp.MtpSource) for
> device Nexus 7 and Uuid /devices/pci0000:00/0000:00:14.0/usb3/3-4

Looks like things are working as intended until the battery status issue, but then afterwards banshee still reports that it found DAP support. Little weird, I'm not sure why the source didn't reappear.

(In reply to comment #26)
> I tested one more time, and this time it appeared (maybe I wasn't patient
> enough the last time).
> 
> However, I clicked on the source and it displayed: loading track 49389 of
> 309488 (I just made up the numbers, I don't remember them). And it felt it had
> stopped there. Then, as I was filing this comment, then banshee requested my
> attention, and apparently it has finished loading, and it told me "the sync
> operation would remove 128 tracks from your device".

It showed all the correct track information for what was on the device?

> 
> This is interesting, it switched automatically to auto-sync mode even though I
> don't recall to have used this mode with this device in the past. Are you sure
> you're not hardcoding this somehow?

Definitely not. GConf keys are separate, which I hadn't checked for certain until now :)

 /apps/banshee-1/sources/_mtp_source_-00192db970d37e:
  /apps/banshee-1/sources/_mtp_source_-00192db970d37e/sync:
   /apps/banshee-1/sources/_mtp_source_-00192db970d37e/sync/_video_library_source_-_video_library:
    sync_entire_library = false
    enabled = false
   /apps/banshee-1/sources/_mtp_source_-00192db970d37e/sync/_music_library_source_-_library:
    sync_source = PlaylistSource-5
    sync_entire_library = false
    enabled = false
   /apps/banshee-1/sources/_mtp_source_-00192db970d37e/sync/_podcast_source_-_podcast_library:
    sync_entire_library = false
    enabled = false
 /apps/banshee-1/sources/_potential_source-00192db970d37e:
  expanded = true

(In reply to comment #27)
> Testing also a bit more this patch I've found two other problems:
> 
> a) In some cases, banshee is unable to show the songs that the device has. When
> this happens, banshee is somehow assuming that the device has 0 songs (instead
> of knowing that the state of its track count is "don't know yet", maybe because
> it's still loading or other reason) and as a consequence of this (as I had
> auto-sync configured with this device this time), it has started to sync songs
> to it, even before calculating the sync properly (i.e.: X number of songs to
> remove, Y number to add, etc). After the songs were synced, I clicked on the
> source, and banshee couldn't show any track yet either.
>
> 2) I've seen these 2 errors in my log, which maybe are suspects of these buggy
> behaviours:
> 
> [1 Debug 15:06:29.615] Gio.Manager: received MountAdded signal
> [1 Error 15:06:29.615] Gio.Manager: ignoring VolumeChanged signal with no
> volume
> [1 Debug 15:06:29.615] Gio.Manager: received MountAdded signal
> [1 Warn  15:06:29.616] Failed to load media-player-info file for 1
> [38 Error 15:06:29.807] Caught an exception - 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/knocte1404/bansheeSHINY/src/Libraries/Mtp/Mtp/MtpDevice.cs:312 
>   at Mtp.MtpDevice.get_BatteryLevel () [0x0000b] in
> /home/knocte1404/bansheeSHINY/src/Libraries/Mtp/Mtp/MtpDevice.cs:68 
>   at Banshee.Dap.Mtp.MtpSource.DeviceInitialize (IDevice device) [0x00254] in
> /home/knocte1404/bansheeSHINY/src/Dap/Banshee.Dap.Mtp/Banshee.Dap.Mtp/MtpSource.cs:152

You're testing with Trusty Tahr? In that case our libmtp is pretty much the same 1.1.6. Do you get the battery error, even occasionally, without the patch applied?

Still, if we got a connection to the device which should be the case at that point, that error is meant to allow us to go on and load tracks.

I'm beginning to suspect differences in libmtp support for our devices.
Comment 30 Andrés G. Aragoneses (IRC: knocte) 2014-07-22 14:00:09 UTC
(In reply to comment #29)
> (In reply to comment #25)
> > Great thanks for the patch!
> > 
> > I think there are 2 things to fix before we commit this:
> > 
> > 1) The "Claim" action is only available on the context menu, but it should also
> > be next to the "Device unavailable..." label, when you click on the source, in
> > the form of a button.
> 
> The claim button should appear at the top of the Source content view next to
> the disconnect button, not just in the context menu.

Hmmm, not for me. Can you post a screenshot of what you see?


> 
> > 2) The patch unfortunately doesn't work for me (tested with Nexus7). This is
> > the log when launching banshee (the device appeared as a source, as
> > unavailable, then I clicked Claim, and then it disappeared):
> > 
> 
> <snip />
> 
> > [17 Error 14:47:39.096] Caught an exception - 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/knocte1404/bansheeSHINY/src/Libraries/Mtp/Mtp/MtpDevice.cs:312 
> >   at Mtp.MtpDevice.get_BatteryLevel () [0x0000b] in
> > /home/knocte1404/bansheeSHINY/src/Libraries/Mtp/Mtp/MtpDevice.cs:68 
> >   at Banshee.Dap.Mtp.MtpSource.DeviceInitialize (IDevice device) [0x00254] in
> > /home/knocte1404/bansheeSHINY/src/Dap/Banshee.Dap.Mtp/Banshee.Dap.Mtp/MtpSource.cs:152 
> > [17 Debug 14:47:39.097] Found DAP support (Banshee.Dap.Mtp.MtpSource) for
> > device Nexus 7 and Uuid /devices/pci0000:00/0000:00:14.0/usb3/3-4
> 
> Looks like things are working as intended until the battery status issue, but
> then afterwards banshee still reports that it found DAP support. Little weird,
> I'm not sure why the source didn't reappear.

I think I didn't wait enough that time (would be nice to show a progress bar saying that the load is happening, somehow).


> 
> (In reply to comment #26)
> > I tested one more time, and this time it appeared (maybe I wasn't patient
> > enough the last time).
> > 
> > However, I clicked on the source and it displayed: loading track 49389 of
> > 309488 (I just made up the numbers, I don't remember them). And it felt it had
> > stopped there. Then, as I was filing this comment, then banshee requested my
> > attention, and apparently it has finished loading, and it told me "the sync
> > operation would remove 128 tracks from your device".
> 
> It showed all the correct track information for what was on the device?

Yep, it was showing some tracks.


> 
> > 
> > This is interesting, it switched automatically to auto-sync mode even though I
> > don't recall to have used this mode with this device in the past. Are you sure
> > you're not hardcoding this somehow?
> 
> Definitely not. GConf keys are separate, which I hadn't checked for certain
> until now :)

Fair enough, I guess it's that I simply don't remember configuring it as auto-sync, but I must have done it.


> (In reply to comment #27)
> > Testing also a bit more this patch I've found two other problems:
> > 
> > a) In some cases, banshee is unable to show the songs that the device has. When
> > this happens, banshee is somehow assuming that the device has 0 songs (instead
> > of knowing that the state of its track count is "don't know yet", maybe because
> > it's still loading or other reason) and as a consequence of this (as I had
> > auto-sync configured with this device this time), it has started to sync songs
> > to it, even before calculating the sync properly (i.e.: X number of songs to
> > remove, Y number to add, etc). After the songs were synced, I clicked on the
> > source, and banshee couldn't show any track yet either.
> >
> > 2) I've seen these 2 errors in my log, which maybe are suspects of these buggy
> > behaviours:
> > 
> > [1 Debug 15:06:29.615] Gio.Manager: received MountAdded signal
> > [1 Error 15:06:29.615] Gio.Manager: ignoring VolumeChanged signal with no
> > volume
> > [1 Debug 15:06:29.615] Gio.Manager: received MountAdded signal
> > [1 Warn  15:06:29.616] Failed to load media-player-info file for 1
> > [38 Error 15:06:29.807] Caught an exception - 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/knocte1404/bansheeSHINY/src/Libraries/Mtp/Mtp/MtpDevice.cs:312 
> >   at Mtp.MtpDevice.get_BatteryLevel () [0x0000b] in
> > /home/knocte1404/bansheeSHINY/src/Libraries/Mtp/Mtp/MtpDevice.cs:68 
> >   at Banshee.Dap.Mtp.MtpSource.DeviceInitialize (IDevice device) [0x00254] in
> > /home/knocte1404/bansheeSHINY/src/Dap/Banshee.Dap.Mtp/Banshee.Dap.Mtp/MtpSource.cs:152
> 
> You're testing with Trusty Tahr?

Yes.

> Do you get the battery error, even occasionally, without the patch
> applied?

Yes.


> Still, if we got a connection to the device which should be the case at that
> point, that error is meant to allow us to go on and load tracks.

But the ArgumentNullException error should not happen. I think that's the one that was happening with my NokiaN900 when I had to revert your first MTP patch (https://github.com/GNOME/banshee/commit/6b6302daccc1d4c6b97f78b0d084d4a6b48407f9).
Comment 31 Nicholas Little 2014-07-22 16:17:51 UTC
(In reply to comment #30)
> (In reply to comment #29)
> > (In reply to comment #25)
> > > Great thanks for the patch!
> > > 
> > > I think there are 2 things to fix before we commit this:
> > > 
> > > 1) The "Claim" action is only available on the context menu, but it should also
> > > be next to the "Device unavailable..." label, when you click on the source, in
> > > the form of a button.
> > 
> > The claim button should appear at the top of the Source content view next to
> > the disconnect button, not just in the context menu.
> 
> Hmmm, not for me. Can you post a screenshot of what you see?
> 

Identical to this, except for the word on the button: https://gbbmtp.files.wordpress.com/2014/07/screenshot-from-2014-07-06-143955.png?w=908

> > 
> > > 2) The patch unfortunately doesn't work for me (tested with Nexus7). This is
> > > the log when launching banshee (the device appeared as a source, as
> > > unavailable, then I clicked Claim, and then it disappeared):
> > > 
> > 
> > <snip />
> > 
> > > [17 Error 14:47:39.096] Caught an exception - 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/knocte1404/bansheeSHINY/src/Libraries/Mtp/Mtp/MtpDevice.cs:312 
> > >   at Mtp.MtpDevice.get_BatteryLevel () [0x0000b] in
> > > /home/knocte1404/bansheeSHINY/src/Libraries/Mtp/Mtp/MtpDevice.cs:68 
> > >   at Banshee.Dap.Mtp.MtpSource.DeviceInitialize (IDevice device) [0x00254] in
> > > /home/knocte1404/bansheeSHINY/src/Dap/Banshee.Dap.Mtp/Banshee.Dap.Mtp/MtpSource.cs:152 
> > > [17 Debug 14:47:39.097] Found DAP support (Banshee.Dap.Mtp.MtpSource) for
> > > device Nexus 7 and Uuid /devices/pci0000:00/0000:00:14.0/usb3/3-4
> > 
> > Looks like things are working as intended until the battery status issue, but
> > then afterwards banshee still reports that it found DAP support. Little weird,
> > I'm not sure why the source didn't reappear.
> 
> I think I didn't wait enough that time (would be nice to show a progress bar
> saying that the load is happening, somehow).
> 

It's still a difference between what I see with my S2, it must read the battery status without trouble.

> > 
> > (In reply to comment #26)
> > > I tested one more time, and this time it appeared (maybe I wasn't patient
> > > enough the last time).
> > > 
> > > However, I clicked on the source and it displayed: loading track 49389 of
> > > 309488 (I just made up the numbers, I don't remember them). And it felt it had
> > > stopped there. Then, as I was filing this comment, then banshee requested my
> > > attention, and apparently it has finished loading, and it told me "the sync
> > > operation would remove 128 tracks from your device".
> > 
> > It showed all the correct track information for what was on the device?
> 
> Yep, it was showing some tracks.
> 

Sorry, was the successful connection attempt one where you saw the battery error above?

<snip />
> 
> > (In reply to comment #27)
> > > Testing also a bit more this patch I've found two other problems:
> > > 
> > > a) In some cases, banshee is unable to show the songs that the device has. When
> > > this happens, banshee is somehow assuming that the device has 0 songs (instead
> > > of knowing that the state of its track count is "don't know yet", maybe because
> > > it's still loading or other reason) and as a consequence of this (as I had
> > > auto-sync configured with this device this time), it has started to sync songs
> > > to it, even before calculating the sync properly (i.e.: X number of songs to
> > > remove, Y number to add, etc). After the songs were synced, I clicked on the
> > > source, and banshee couldn't show any track yet either.
> > >
> > > 2) I've seen these 2 errors in my log, which maybe are suspects of these buggy
> > > behaviours:
> > > 
> > > [1 Debug 15:06:29.615] Gio.Manager: received MountAdded signal
> > > [1 Error 15:06:29.615] Gio.Manager: ignoring VolumeChanged signal with no
> > > volume
> > > [1 Debug 15:06:29.615] Gio.Manager: received MountAdded signal
> > > [1 Warn  15:06:29.616] Failed to load media-player-info file for 1
> > > [38 Error 15:06:29.807] Caught an exception - 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/knocte1404/bansheeSHINY/src/Libraries/Mtp/Mtp/MtpDevice.cs:312 
> > >   at Mtp.MtpDevice.get_BatteryLevel () [0x0000b] in
> > > /home/knocte1404/bansheeSHINY/src/Libraries/Mtp/Mtp/MtpDevice.cs:68 
> > >   at Banshee.Dap.Mtp.MtpSource.DeviceInitialize (IDevice device) [0x00254] in
> > > /home/knocte1404/bansheeSHINY/src/Dap/Banshee.Dap.Mtp/Banshee.Dap.Mtp/MtpSource.cs:152
> > 
> > You're testing with Trusty Tahr?
> 
> Yes.
> 
> > Do you get the battery error, even occasionally, without the patch
> > applied?
> 
> Yes.
> 
> 
> > Still, if we got a connection to the device which should be the case at that
> > point, that error is meant to allow us to go on and load tracks.
> 
> But the ArgumentNullException error should not happen. I think that's the one
> that was happening with my NokiaN900 when I had to revert your first MTP patch
> (https://github.com/GNOME/banshee/commit/6b6302daccc1d4c6b97f78b0d084d4a6b48407f9).

That was with a mass storage behaviour change though (since devices were potentially coming through unmounted we thought mounting was logical), a slightly ill conceived idea and IIRC we improved that patch and made no changes to Mass Storage in doing so. The error is about not getting a volume with the MountArgs, it's just ignoring the signal where before we were delivering the null.

At the point where the battery status error happens in your log above it should have the MTP connection but as we see from the transfer error I don't think it did, or perhaps we were trying to do two things at once over it? Though we're not multi threading there. <confused />

My S2 isn't presenting the same battery or load errors, but I'm fairly sure I see the null volume one as I did before so that shouldn't be a concern.
Comment 32 Nicholas Little 2014-07-23 15:32:09 UTC
Created attachment 281486 [details] [review]
Cleanup and slight rework V2

I've done some testing an Ubuntu Trust Tahr system and found that sometimes due to the race, nautilus and us try to connect to the device but nautilus is a little bit quicker so when we look it's not mounted, but we fail to connect. However, after that we should have raised an InvalidDeviceException and device type enumeration should have happened so I'm still not sure what went on during your failed connection attempt above. In this version, we raise an InvalidStateException also if the connection fails for an identified MTP device, not just when it's mounted - that at least got the PotentialSource to map in my case.

It looks like those GLib errors are from child sources that we either had at one time or are accumulating somehow but they appear (for MTP) without the patch too.

This version should prevent them from occurring for PotentialSource since I've added some properties and overrides to try and make it as transient as possible and at least disappear completely between times, but for that to work we need to get PrimarySource.Dispose to be invoked so:

1. DapSource.Dispose must call base.Dispose
2. MtpSource.Dispose can't unpeel inside out
3. Flush during Dispose had to go (it's called from Eject already)

These changes had the lovely benefit that I was able to remove the rather hacky SourceManager.RemoveSource call in MtpSource.Dispose - the source gets removed both on Ubuntu and Gentoo after the Dispose call unwinds.
Comment 33 Andrés G. Aragoneses (IRC: knocte) 2014-07-26 12:17:01 UTC
Why are you removing the Flush() call in Dispose?
Comment 34 Andrés G. Aragoneses (IRC: knocte) 2014-07-26 12:18:49 UTC
(In reply to comment #33)
> Why are you removing the Flush() call in Dispose?

I mean, the reason "Flush during Dispose had to go (it's called from Eject already)" is not enough. The DapSource instance could be disposed by different means than Eject, and anyway, Flush() being called twice doesn't do any harm (look at its implementation).
Comment 35 Andrés G. Aragoneses (IRC: knocte) 2014-07-26 12:24:05 UTC
Just tested the last patch and I saw two issues:

a) The time that passes since I press "Claim" until I see the source appear again is too long (maybe 20 secs?). I wonder if we can:
   (1). fix it so it's not so long?
   (2). if the above can't be fixed, maybe show a progress bar indicating "Claiming... (don't disconnect)" status?
   (3). if we can do (2), it would be useful too that we don't remove the PotentialDapSource from the UI until the new DapSource appears.

b) I saw this in the console after ending the sync (probably a different bug):

[21 Debug 14:20:26.144] Ending DAP sync
[21 Error 14:20:26.235] Caught an exception - System.NullReferenceException: Object reference not set to an instance of an object (in `Mtp')
  at Mtp.Playlist.Create () [0x00001] in /home/knocte1404/bansheeSHINY/src/Libraries/Mtp/Mtp/Playlist.cs:87 
  at Mtp.AbstractTrackList.Save () [0x000bc] in /home/knocte1404/bansheeSHINY/src/Libraries/Mtp/Mtp/AbstractTrackList.cs:90 
  at Banshee.Dap.Mtp.MtpSource.SyncPlaylists () [0x00134] in /home/knocte1404/bansheeSHINY/src/Dap/Banshee.Dap.Mtp/Banshee.Dap.Mtp/MtpSource.cs:267 
  at Banshee.Dap.DapSync.RateLimitedSync () [0x0011e] in /home/knocte1404/bansheeSHINY/src/Dap/Banshee.Dap/Banshee.Dap/DapSync.cs:373 
  at Banshee.Base.RateLimiter.InnerExecute () [0x0002b] in /home/knocte1404/bansheeSHINY/src/Core/Banshee.Services/Banshee.Base/RateLimiter.cs:64 
  at Banshee.Base.RateLimiter.Execute () [0x00042] in /home/knocte1404/bansheeSHINY/src/Core/Banshee.Services/Banshee.Base/RateLimiter.cs:55 
  at Banshee.Dap.DapSync.<Sync>m__2 () [0x00007] in /home/knocte1404/bansheeSHINY/src/Dap/Banshee.Dap/Banshee.Dap/DapSync.cs:329 
  at Hyena.ThreadAssist.SpawnFromMain (System.Threading.ThreadStart threadedMethod) [0x0001c] in /home/knocte1404/bansheeSHINY/src/Hyena/Hyena/Hyena/ThreadAssist.cs:112 
  at Banshee.Dap.DapSync.Sync () [0x0000d] in /home/knocte1404/bansheeSHINY/src/Dap/Banshee.Dap/Banshee.Dap/DapSync.cs:328 
  at Banshee.Dap.DapSource.PostLoad () [0x0002e] in /home/knocte1404/bansheeSHINY/src/Dap/Banshee.Dap/Banshee.Dap/DapSource.cs:326 
  at Banshee.Dap.DapSource.ThreadedLoadDeviceContents () [0x0002b] in /home/knocte1404/bansheeSHINY/src/Dap/Banshee.Dap/Banshee.Dap/DapSource.cs:308
Comment 36 Nicholas Little 2014-07-26 13:14:38 UTC
(In reply to comment #34)
> (In reply to comment #33)
> > Why are you removing the Flush() call in Dispose?
> 
> I mean, the reason "Flush during Dispose had to go (it's called from Eject
> already)" is not enough. The DapSource instance could be disposed by different
> means than Eject, and anyway, Flush() being called twice doesn't do any harm
> (look at its implementation).

No problem, the real reason is that the device should have already gone by the time the superclass has a look at it.

I thought that was obvious from number 2 :) Dispose is a tear down call, so it really has to go subclass -> base class. So, if our DapSources release their devices then call base.Dispose, the device is gone so there's no point in calling Flush, which does do harm if we need to sync playlists and there's nothing to flush to.

Think about it this way, if the user presses "Disconnect" then before the source is disposed, Eject is run on it, causing us to Flush and we know the device is there. If the user unplugs it, we get a signal and call Dispose, which ends up calling Flush even though that makes absolutely no sense.

Basically, we wouldn't have needed the boolean if we only called it once, from one place and under controlled circumstances.

(In reply to comment #35)
> Just tested the last patch and I saw two issues:
> 
> a) The time that passes since I press "Claim" until I see the source appear
> again is too long (maybe 20 secs?). I wonder if we can:
>    (1). fix it so it's not so long?

I've found this varies. My Gentoo system suffers like yours does, but when I tried this on Ubuntu the mount/unmount (wrt MTP) seemed really quick.

Are you still getting those GLib errors? Those seemed to slow the source display down too. I'll file a new bug about those today/tomorrow.

>    (2). if the above can't be fixed, maybe show a progress bar indicating
> "Claiming... (don't disconnect)" status?

This definitely makes sense, should be a doddle :)

>    (3). if we can do (2), it would be useful too that we don't remove the
> PotentialDapSource from the UI until the new DapSource appears.

In fact, thinking about it now I think we can probably use TryCreateSource there instead of just running the device back through the Unmap/Map cycle. That way, if we still can't connect then we won't Unmap Potential, Map Potential.

> b) I saw this in the console after ending the sync (probably a different bug):

<snip />

Most definitely a different bug. Seems it's one down, two up ;)
Comment 37 Andrés G. Aragoneses (IRC: knocte) 2014-07-26 14:41:32 UTC
(In reply to comment #36)
> (In reply to comment #34)
> > (In reply to comment #33)
> > > Why are you removing the Flush() call in Dispose?
> > 
> > I mean, the reason "Flush during Dispose had to go (it's called from Eject
> > already)" is not enough. The DapSource instance could be disposed by different
> > means than Eject, and anyway, Flush() being called twice doesn't do any harm
> > (look at its implementation).
> 
> No problem, the real reason is that the device should have already gone by the
> time the superclass has a look at it.
> 
> I thought that was obvious from number 2 :) Dispose is a tear down call, so it
> really has to go subclass -> base class. So, if our DapSources release their
> devices then call base.Dispose, the device is gone so there's no point in
> calling Flush, which does do harm if we need to sync playlists and there's
> nothing to flush to.
> 
> Think about it this way, if the user presses "Disconnect" then before the
> source is disposed, Eject is run on it, causing us to Flush and we know the
> device is there. If the user unplugs it, we get a signal and call Dispose,
> which ends up calling Flush even though that makes absolutely no sense.
> 
> Basically, we wouldn't have needed the boolean if we only called it once, from
> one place and under controlled circumstances.

If you do git-blame on that flush impl, you will actually find a link to a bug report which *I* filed, and in which a propose a bugfix which doesn't have a boolean flag (but was told to use a flag, even when I didn't like very much the idea ;) ). Anyway, this seems like should be addressed in a different bug, right?

> 
> (In reply to comment #35)
> > Just tested the last patch and I saw two issues:
> > 
> > a) The time that passes since I press "Claim" until I see the source appear
> > again is too long (maybe 20 secs?). I wonder if we can:
> >    (1). fix it so it's not so long?
> 
> I've found this varies. My Gentoo system suffers like yours does, but when I
> tried this on Ubuntu the mount/unmount (wrt MTP) seemed really quick.
> 
> Are you still getting those GLib errors? Those seemed to slow the source
> display down too. I'll file a new bug about those today/tomorrow.

The "(Nereid:6353): GLib-CRITICAL **: Source ID 11097 was not found when attempting to remove it" ones? Yes, because I haven't upgraded yet to gtk-sharp master.

> 
> >    (2). if the above can't be fixed, maybe show a progress bar indicating
> > "Claiming... (don't disconnect)" status?
> 
> This definitely makes sense, should be a doddle :)

Great. I guess you will need to set up a timeout (if in 1 minute the new source doesn't appear, there was a problem?)

> 
> >    (3). if we can do (2), it would be useful too that we don't remove the
> > PotentialDapSource from the UI until the new DapSource appears.
> 
> In fact, thinking about it now I think we can probably use TryCreateSource
> there instead of just running the device back through the Unmap/Map cycle. That
> way, if we still can't connect then we won't Unmap Potential, Map Potential.

This sounds exactly as I was thinking, great plan!


> > b) I saw this in the console after ending the sync (probably a different bug):
> 
> <snip />
> 
> Most definitely a different bug. Seems it's one down, two up ;)

Ok, will file it separately.

BTW can you modify the message "Device is not available" by something like "Device is connected but in use by other software. Press the 'Claim' button above if you want to mount it in Banshee"?
Comment 38 Andrés G. Aragoneses (IRC: knocte) 2014-07-26 15:07:09 UTC
Another thing, I saw you use NIEs in your patch:

+        protected override void AddTrackToDevice (DatabaseTrackInfo track, SafeUri fromUri)
+        {
+            throw new NotImplementedException ();
+        }

However, NIEs are elements that invite contributors to implement something that is not implemented. In this case, as PotentialDapSource cannot and shouldn't ever implement AddTrackToDevice I guess it's better to use NotSupportedException() and/or Hyena.Error("Shouldn't be reached") and/or InvalidOperationException.
Comment 39 Nicholas Little 2014-07-26 15:16:50 UTC
(In reply to comment #37)
> (In reply to comment #36)
> > (In reply to comment #34)
> > > (In reply to comment #33)
<snip />
> 
> If you do git-blame on that flush impl, you will actually find a link to a bug
> report which *I* filed, and in which a propose a bugfix which doesn't have a
> boolean flag (but was told to use a flag, even when I didn't like very much the
> idea ;) ). Anyway, this seems like should be addressed in a different bug,
> right?
> 

Sounds good to me.

> > 
> > (In reply to comment #35)
<snip />
> The "(Nereid:6353): GLib-CRITICAL **: Source ID 11097 was not found when
> attempting to remove it" ones? Yes, because I haven't upgraded yet to gtk-sharp
> master.
> 

Ahh, I run gtk-sharp from git on Gentoo, I hope the absence is nothing to worry about ;)

> > 
> > >    (2). if the above can't be fixed, maybe show a progress bar indicating
> > > "Claiming... (don't disconnect)" status?
> > 
> > This definitely makes sense, should be a doddle :)
> 
> Great. I guess you will need to set up a timeout (if in 1 minute the new source
> doesn't appear, there was a problem?)
> 

Might require a slight modification (sending an IDevice into the Claim method) to do it cleanly, but it's been bugging me that the implementation tries a regular initialize before attempting to force the matter, then after it calls Claim it just hopes that whatever action will cause it to be unmapped.

> > 
> > >    (3). if we can do (2), it would be useful too that we don't remove the
> > > PotentialDapSource from the UI until the new DapSource appears.
> > 
> > In fact, thinking about it now I think we can probably use TryCreateSource
> > there instead of just running the device back through the Unmap/Map cycle. That
> > way, if we still can't connect then we won't Unmap Potential, Map Potential.
> 
> This sounds exactly as I was thinking, great plan!
> 
> 
> > > b) I saw this in the console after ending the sync (probably a different bug):
> > 
> > <snip />
> > 
> > Most definitely a different bug. Seems it's one down, two up ;)
> 
> Ok, will file it separately.
> 
> BTW can you modify the message "Device is not available" by something like
> "Device is connected but in use by other software. Press the 'Claim' button
> above if you want to mount it in Banshee"?

That's much more eloquent :)(In reply to comment #38)
> Another thing, I saw you use NIEs in your patch:
> 
> +        protected override void AddTrackToDevice (DatabaseTrackInfo track,
> SafeUri fromUri)
> +        {
> +            throw new NotImplementedException ();
> +        }
> 
> However, NIEs are elements that invite contributors to implement something that
> is not implemented. In this case, as PotentialDapSource cannot and shouldn't
> ever implement AddTrackToDevice I guess it's better to use
> NotSupportedException() and/or Hyena.Error("Shouldn't be reached") and/or
> InvalidOperationException.

Good point, I've never really thought about the semantics. I'll fix it.
Comment 40 Nicholas Little 2014-07-29 15:01:14 UTC
Created attachment 281960 [details] [review]
Cleanup and slight rework V3

Ok, in the new version I absorbed the Claim method into DeviceInitialize via a boolean parameter, this seems a lot cleaner. 

Also, in the other versions I'd pulled out a null check from MtpSource which I've left in this time though I'm pretty sure it's unnecessary.

TryDeviceInitialize (was TryCreateSource) instead of Unmap/Map cycle: check

s/NotImplementedException/NotSupportedException/g: check

message update: check, screenshot to follow.

clean wrt disposable improvements: check, new bug to follow.
Comment 41 Nicholas Little 2014-07-29 15:05:12 UTC
Created attachment 281961 [details]
New "Device Unavailable" text

I gave the first a bit sentence of a massage and thought we'd drop the 'mount' word and just go with 'use' in the last one to avoid any possible confusion.

What do you think?
Comment 42 Andrés G. Aragoneses (IRC: knocte) 2014-07-30 11:36:10 UTC
(In reply to comment #41)
> Created an attachment (id=281961) [details]
> New "Device Unavailable" text
> 
> I gave the first a bit sentence of a massage and thought we'd drop the 'mount'
> word and just go with 'use' in the last one to avoid any possible confusion.
> 
> What do you think?

I like it!

But I guess you have just used this message in the case PotentialDapSource is being used, right? Just double checking...
Comment 43 Andrés G. Aragoneses (IRC: knocte) 2014-07-30 11:55:24 UTC
Comment on attachment 281960 [details] [review]
Cleanup and slight rework V3

I guess this patch is ready, except for:
a) style issues (I would have fixed this myself, but thinking that at some point in the future you will be reviewing patches of others, makes me want to point out everything now, so you keep practicing ;) )
b) haven't tested it yet (will do it this afternoon)

So here we come with the style issues, inline:

> From df2c0e7b3e021b1a7058faf19a4d55d6184fe628 Mon Sep 17 00:00:00 2001
> From: Nicholas Little <arealityfarbetween@googlemail.com>
> Date: Sat, 5 Jul 2014 12:01:53 +0100
> Subject: [PATCH] Dap/Mtp: Remove unmount, add claim (BGO#731916)

We usually write 'bgo' in lowercase.


> 
> We need to unmount MTP devices for banshee to make use of them, this
> causes nautilus to produce error messages in some cases, as described in
> [1].
> 
> This patch adds a parameter to DapSource.DeviceInitialize to signal
> whether we should attempt to force the connection to the device (at the
> user's request via the ClaimDapAction) or just make a best attempt.
> 
> To facilitate this, DapSource subclasses can now throw
> InvalidStateException which will allow them to make a claim on a
> device even though they couldn't initialize it. At that point, a new
> type, PotentialSource is mapped, enabling the ClaimDapAction.
> 
> 1. https://bugzilla.gnome.org/show_bug.cgi?id=731916

We usually write [1] too (instead of '1.'), so it's easily spotted by the eye.


> ---
>  .../Banshee.Dap.MassStorage/MassStorageSource.cs   |   4 +-
>  .../Banshee.Dap.Mtp/Banshee.Dap.Mtp/MtpSource.cs   |  24 ++--
>  src/Dap/Banshee.Dap/Banshee.Dap.Gui/DapActions.cs  |  15 +++
>  src/Dap/Banshee.Dap/Banshee.Dap.Gui/DapContent.cs  |  71 +++++++++-
>  src/Dap/Banshee.Dap/Banshee.Dap.csproj             |   1 +
>  src/Dap/Banshee.Dap/Banshee.Dap/DapService.cs      |  46 ++++++-
>  src/Dap/Banshee.Dap/Banshee.Dap/DapSource.cs       |   2 +-
>  .../Banshee.Dap/InvalidDeviceException.cs          |   4 +
>  src/Dap/Banshee.Dap/Banshee.Dap/PotentialSource.cs | 148 +++++++++++++++++++++
>  src/Dap/Banshee.Dap/Makefile.am                    |   1 +
>  src/Dap/Banshee.Dap/Resources/ActiveSourceUI.xml   |   1 +
>  src/Dap/Banshee.Dap/Resources/GlobalUI.xml         |   1 +
>  12 files changed, 295 insertions(+), 23 deletions(-)
>  create mode 100644 src/Dap/Banshee.Dap/Banshee.Dap/PotentialSource.cs
> 
> 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 f49a8cf..cdf6d38 100644
> --- a/src/Dap/Banshee.Dap.MassStorage/Banshee.Dap.MassStorage/MassStorageSource.cs
> +++ b/src/Dap/Banshee.Dap.MassStorage/Banshee.Dap.MassStorage/MassStorageSource.cs
> @@ -58,9 +58,9 @@ namespace Banshee.Dap.MassStorage
>          private IVolume volume;
>          private IUsbDevice usb_device;
>  
> -        public override void DeviceInitialize (IDevice device)
> +        public override void DeviceInitialize (IDevice device, bool force)
>          {
> -            base.DeviceInitialize (device);
> +            base.DeviceInitialize (device, force);
>  
>              volume = device as IVolume;
>  
> diff --git a/src/Dap/Banshee.Dap.Mtp/Banshee.Dap.Mtp/MtpSource.cs b/src/Dap/Banshee.Dap.Mtp/Banshee.Dap.Mtp/MtpSource.cs
> index 3ef48ff..f6bc8f8 100644
> --- a/src/Dap/Banshee.Dap.Mtp/Banshee.Dap.Mtp/MtpSource.cs
> +++ b/src/Dap/Banshee.Dap.Mtp/Banshee.Dap.Mtp/MtpSource.cs
> @@ -63,9 +63,9 @@ namespace Banshee.Dap.Mtp
>          private bool can_sync_albumart = NeverSyncAlbumArtSchema.Get () == false;
>          private int thumb_width = AlbumArtWidthSchema.Get ();
>  
> -        public override void DeviceInitialize (IDevice device)
> +        public override void DeviceInitialize (IDevice device, bool force)
>          {
> -            base.DeviceInitialize (device);
> +            base.DeviceInitialize (device, force);
>  
>              var portInfo = device.ResolveUsbPortInfo ();
>              if (portInfo == null || portInfo.DeviceNumber == 0) {
> @@ -97,19 +97,21 @@ namespace Banshee.Dap.Mtp
>                  //if (v.BusNumber == busnum && v.DeviceNumber == devnum) {
>                  if (v.DeviceNumber == devnum) {
>                      // If gvfs-gphoto has it mounted, unmount it
> -                    if (volume != null && volume.IsMounted) {
> +                    if (volume != null && volume.IsMounted && force) {
>                          volume.Unmount ();
>                      }
>  
> -                    for (int i = 5; i > 0 && mtp_device == null; i--) {
> -                        try {
> -                            mtp_device = MtpDevice.Connect (v);
> -                        } catch (Exception) {}
> +                    if (volume != null && volume.IsMounted) {
> +                        throw new InvalidStateException ();
> +                    }
> +
> +                    try {
> +                        mtp_device = MtpDevice.Connect (v);
> +                    } catch (Exception) {}

I don't like empty generic catches :)
Please use a more specific type of exception (not base System.Exception), and log the exception.


>  
> -                        if (mtp_device == null) {
> -                            Log.DebugFormat ("Failed to connect to mtp device. Trying {0} more times...", i - 1);
> -                            Thread.Sleep (2000);
> -                        }
> +                    if (mtp_device == null) {
> +                        Log.DebugFormat ("Failed to connect to mtp device {0}", device.Name);
> +                        throw new InvalidStateException ();
>                      }
>                  }
>              }
> diff --git a/src/Dap/Banshee.Dap/Banshee.Dap.Gui/DapActions.cs b/src/Dap/Banshee.Dap/Banshee.Dap.Gui/DapActions.cs
> index 949007c..296e78d 100644
> --- a/src/Dap/Banshee.Dap/Banshee.Dap.Gui/DapActions.cs
> +++ b/src/Dap/Banshee.Dap/Banshee.Dap.Gui/DapActions.cs
> @@ -49,6 +49,11 @@ namespace Banshee.Dap.Gui
>          public DapActions () : base ("dap")
>          {
>              AddImportant (
> +                new ActionEntry ("ClaimDapAction", null,
> +                    Catalog.GetString ("Claim"), null,
> +                    String.Empty, OnClaimDap)
> +            );
> +            AddImportant (
>                  new ActionEntry ("SyncDapAction", null,
>                      Catalog.GetString ("Sync"), null,
>                      String.Empty, OnSyncDap)
> @@ -57,6 +62,7 @@ namespace Banshee.Dap.Gui
>              AddUiFromFile ("GlobalUI.xml");
>  
>              this["SyncDapAction"].IconName = Stock.Refresh;
> +            this["ClaimDapAction"].IconName = Stock.Connect;
>              ServiceManager.SourceManager.ActiveSourceChanged += OnActiveSourceChanged;
>              Actions.SourceActions.Updated += delegate { UpdateActions (); };
>              OnActiveSourceChanged (null);
> @@ -71,6 +77,7 @@ namespace Banshee.Dap.Gui
>              }
>  
>              previous_dap = ActiveSource as DapSource;
> +            UpdateActions ();
>  
>              if (previous_dap != null) {
>                  previous_dap.Sync.Updated += OnSyncUpdated;
> @@ -87,6 +94,7 @@ namespace Banshee.Dap.Gui
>              DapSource dap = Dap;
>              if (dap != null) {
>                  UpdateAction ("SyncDapAction", dap.Sync.Enabled);
> +                UpdateAction ("ClaimDapAction", dap is PotentialSource);
>              }
>          }
>  
> @@ -98,5 +106,12 @@ namespace Banshee.Dap.Gui
>              }
>          }
>  
> +        private void OnClaimDap (object o, EventArgs args)
> +        {
> +            var dap = Dap as PotentialSource;
> +            if (dap != null) {
> +                dap.Claim ();
> +            }
> +        }
>      }
>  }
> diff --git a/src/Dap/Banshee.Dap/Banshee.Dap.Gui/DapContent.cs b/src/Dap/Banshee.Dap/Banshee.Dap.Gui/DapContent.cs
> index 81c5349..ae29944 100644
> --- a/src/Dap/Banshee.Dap/Banshee.Dap.Gui/DapContent.cs
> +++ b/src/Dap/Banshee.Dap/Banshee.Dap.Gui/DapContent.cs
> @@ -65,7 +65,6 @@ namespace Banshee.Dap.Gui
>              dap = source;
>  
>              BuildWidgets ();
> -            BuildActions ();
>              dap.Properties.PropertyChanged += OnPropertyChanged;
>          }
>  
> @@ -139,11 +138,51 @@ namespace Banshee.Dap.Gui
>              opts.Dispose ();
>          }
>  
> -        private void BuildActions ()
> +        private void SetTitleText (string name)
>          {
> -            if (actions == null) {
> -                actions = new DapActions ();
> -            }
> +            title.Markup = String.Format ("<span size=\"x-large\" weight=\"bold\">{0}</span>", name);
> +        }
> +
> +        private void OnPropertyChanged (object o, PropertyChangeEventArgs args)
> +        {
> +            if (args.PropertyName == "Name")

We use braces for 'if' statements always, even if the line after it is just one.


> +                SetTitleText (args.NewValue.ToString ());
> +        }
> +
> +        private static Banshee.Gui.BansheeActionGroup actions = new DapActions ();
> +
> +        public static void Init () { }
> +    }
> +
> +    public class InactiveContent : DapPropertiesDisplay
> +    {
> +        Label title;
> +
> +        public InactiveContent (DapSource dapSource) : base(dapSource)
> +        {
> +            dapSource.Properties.PropertyChanged += OnPropertyChanged;
> +            BuildWidgets ();
> +        }
> +
> +        private void BuildWidgets ()
> +        {
> +            var outer = new HBox();
> +            var device = new Image (LargeIcon) { Yalign = 0.0f };
> +            outer.PackStart (device, false, false, 0);
> +
> +            var inner = new VBox { Spacing = 5, BorderWidth = 5 };
> +            title = new Label { UseMarkup = true, Xalign = 0.0f };
> +            SetTitleText (Source.Name);
> +            inner.PackStart (title, false, false, 0);
> +
> +            var box = new HBox { Spacing = 5 };
> +            box.PackStart (new Image { IconName = "dialog-warning" }, false, false, 0);
> +            box.PackStart (new Label { Markup = ErrorString, UseMarkup = true }, false, false, 0);
> +            inner.PackStart (box, false, false, 0);
> +
> +            outer.PackEnd (inner, false, false, 0);
> +            Add (outer);
> +            ShowAll ();
>          }
>  
>          private void SetTitleText (string name)
> @@ -157,6 +196,26 @@ namespace Banshee.Dap.Gui
>                  SetTitleText (args.NewValue.ToString ());
>          }
>  
> -        private static Banshee.Gui.BansheeActionGroup actions;
> +        protected virtual string ErrorString {
> +            get { return DefaultErrorString; }
> +        }
> +
> +        static InactiveContent () {

Braces of methods go in 2nd line.


> +            var generic = Catalog.GetString ("Your device appears to be in use by another program");
> +            var claimit = String.Format (
> +                @"<span weight=""bold"">{0}</span>",
> +                Catalog.GetString ("Claim")
> +            );
> +            var pressit = String.Format (
> +                Catalog.GetString ("Press the {0} button above to use it in Banshee"),
> +                claimit
> +            );
> +            DefaultErrorString = string.Format(

Missing space before parenthesis.


> +                @"<span size=""large"">{0}." + "\n" + "{1}…</span>", generic, pressit
> +            );
> +            DapContent.Init ();
> +        }
> +
> +        public static readonly string DefaultErrorString;

You sure this needs to be public? Check if you can put it as internal at least.


>      }
>  }
> diff --git a/src/Dap/Banshee.Dap/Banshee.Dap.csproj b/src/Dap/Banshee.Dap/Banshee.Dap.csproj
> index 465ba8d..023de4e 100644
> --- a/src/Dap/Banshee.Dap/Banshee.Dap.csproj
> +++ b/src/Dap/Banshee.Dap/Banshee.Dap.csproj
> @@ -97,6 +97,7 @@
>      <Compile Include="Banshee.Dap.Gui\LibrarySyncOptions.cs" />
>      <Compile Include="Banshee.Dap\DapPriorityNode.cs" />
>      <Compile Include="Banshee.Dap\SyncPlaylist.cs" />
> +    <Compile Include="Banshee.Dap\PotentialSource.cs" />
>    </ItemGroup>
>    <ItemGroup>
>      <EmbeddedResource Include="Banshee.Dap.addin.xml">
> diff --git a/src/Dap/Banshee.Dap/Banshee.Dap/DapService.cs b/src/Dap/Banshee.Dap/Banshee.Dap/DapService.cs
> index 8537dca..4b1bb93 100644
> --- a/src/Dap/Banshee.Dap/Banshee.Dap/DapService.cs
> +++ b/src/Dap/Banshee.Dap/Banshee.Dap/DapService.cs
> @@ -153,10 +153,17 @@ namespace Banshee.Dap
>              foreach (TypeExtensionNode node in supported_dap_types) {
>                  try {
>                      DapSource source = (DapSource)node.CreateInstance ();
> -                    source.DeviceInitialize (device);
> +                    source.DeviceInitialize (device, false);
>                      source.LoadDeviceContents ();
>                      source.AddinId = node.Addin.Id;
>                      return source;
> +                } catch (InvalidStateException) {
> +                    Log.ErrorFormat (
> +                        "Dap.DapService: invalid state, mapping potential source for {0}",
> +                        device.Name
> +                    );
> +                    DapSource source = new PotentialSource (this, node, device);
> +                    return source;
>                  } catch (InvalidDeviceException) {
>                  } catch (InvalidCastException e) {
>                      Log.Warning ("Extension is not a DapSource as required", e);
> @@ -173,6 +180,29 @@ namespace Banshee.Dap
>              Scheduler.Schedule (new MapDeviceJob (this, device));
>          }
>  
> +        internal void SwapSource (DapSource oldSource, DapSource newSource)
> +        {
> +            if (oldSource.Device.Uuid != newSource.Device.Uuid) {
> +                Log.ErrorFormat (
> +                    "Dap.DapService: swap ignored from {0} to {1}.",
> +                    oldSource.Device.Uuid, newSource.Device.Uuid
> +                );
> +                return;
> +            }
> +            Log.DebugFormat (
> +                "Dap.DapService: Swapping {0} with UUID {1} for {2}",
> +                oldSource.GetType ().Name, oldSource.Device.Uuid,
> +                newSource.GetType ().Name
> +            );
> +            ThreadAssist.ProxyToMain (() => {
> +                lock (sync) {
> +                    ServiceManager.SourceManager.RemoveSource (oldSource);
> +                    sources [newSource.Device.Uuid] = newSource;
> +                    ServiceManager.SourceManager.AddSource (newSource);
> +                }
> +            });
> +        }
> +
>          private class MapDeviceJob : IJob
>          {
>              IDevice device;
> @@ -286,7 +316,8 @@ namespace Banshee.Dap
>          {
>              DapSource dap_source = args.Source as DapSource;
>              if (dap_source != null) {
> -                UnmapDevice (dap_source.Device.Uuid);
> +                sources.Remove (dap_source.Device.Uuid);
> +                dap_source.Dispose ();
>              }
>          }

Should this hunk above be part of this patch? Seems unrelated (maybe this is part of other patch that you have been working on, related to Dispose pattern).

>  
> @@ -297,7 +328,16 @@ namespace Banshee.Dap
>  
>          private void OnHardwareDeviceChanged (object o, DeviceChangedEventArgs args)
>          {
> -            MapDevice (args.Device);
> +            DapSource source;
> +            if (!sources.TryGetValue (args.Device.Uuid, out source)) {
> +                MapDevice (args.Device);
> +                return;
> +            }
> +
> +            PotentialSource potential = source as PotentialSource;
> +            if (potential != null && potential.TryDeviceInitialize(false, out source)) {

Space before parenthesis.

> +                SwapSource (potential, source);
> +            }
>          }
>  
>          private void OnHardwareDeviceRemoved (object o, DeviceRemovedArgs args)
> diff --git a/src/Dap/Banshee.Dap/Banshee.Dap/DapSource.cs b/src/Dap/Banshee.Dap/Banshee.Dap/DapSource.cs
> index 337f8a5..00709be 100644
> --- a/src/Dap/Banshee.Dap/Banshee.Dap/DapSource.cs
> +++ b/src/Dap/Banshee.Dap/Banshee.Dap/DapSource.cs
> @@ -88,7 +88,7 @@ namespace Banshee.Dap
>          {
>          }
>  
> -        public virtual void DeviceInitialize (IDevice device)
> +        public virtual void DeviceInitialize (IDevice device, bool force)
>          {
>              this.device = device;
>              TypeUniqueId = device.Serial;
> diff --git a/src/Dap/Banshee.Dap/Banshee.Dap/InvalidDeviceException.cs b/src/Dap/Banshee.Dap/Banshee.Dap/InvalidDeviceException.cs
> index f8dbb76..7a05cc4 100644
> --- a/src/Dap/Banshee.Dap/Banshee.Dap/InvalidDeviceException.cs
> +++ b/src/Dap/Banshee.Dap/Banshee.Dap/InvalidDeviceException.cs
> @@ -33,4 +33,8 @@ namespace Banshee.Dap
>      public class InvalidDeviceException : ApplicationException
>      {
>      }
> +
> +    public class InvalidStateException : InvalidDeviceException
> +    {
> +    }

I prefer the name InvalidDeviceStateException.


>  }
> diff --git a/src/Dap/Banshee.Dap/Banshee.Dap/PotentialSource.cs b/src/Dap/Banshee.Dap/Banshee.Dap/PotentialSource.cs
> new file mode 100644
> index 0000000..c1aeeed
> --- /dev/null
> +++ b/src/Dap/Banshee.Dap/Banshee.Dap/PotentialSource.cs
> @@ -0,0 +1,148 @@
> +//
> +// PotentialSource.cs
> +//
> +// Author:
> +//   Nicholas Little <arealityfarbetween@googlemail.com>
> +//
> +// 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
> +// in the Software without restriction, including without limitation the rights
> +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> +// copies of the Software, and to permit persons to whom the Software is
> +// furnished to do so, subject to the following conditions:
> +//
> +// The above copyright notice and this permission notice shall be included in
> +// all copies or substantial portions of the Software.
> +//
> +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
> +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> +// THE SOFTWARE.
> +using System;

Missing a blank line between the header and the first 'using'.

> +
> +using Banshee.Collection.Database;
> +using Banshee.Dap.Gui;
> +using Banshee.Hardware;
> +using Banshee.Sources;
> +using Banshee.Sources.Gui;
> +using Hyena;
> +using Mono.Addins;
> +
> +namespace Banshee.Dap
> +{
> +    internal class PotentialSource : DapSource
> +    {
> +        public readonly TypeExtensionNode Claimant;

I only see Claimant being used privately here, so 'private'?


> +        public readonly DapService Service;

Same here maybe?


> +
> +        internal PotentialSource (DapService service, TypeExtensionNode claim, IDevice device)
> +        {
> +            Claimant = claim;
> +            Service = service;
> +
> +            IsTemporary = true;
> +
> +            SupportsPlaylists = false;
> +            SupportsPodcasts = false;
> +            SupportsVideo = false;
> +
> +            DeviceInitialize (device, false);
> +            Initialize ();
> +        }
> +
> +        #region overridden members of IDisposable
> +
> +        public override void Dispose ()
> +        {
> +            IsTemporary = true;

You have set IsTemporary to true in the constructor, do you think it could have changed during its life, before Dispose() is called?

> +            base.Dispose ();
> +        }
> +
> +        #endregion
> +
> +        #region overridden members of Source
> +
> +        protected override void Initialize ()
> +        {
> +            base.Initialize ();
> +            ThreadAssist.ProxyToMain (() => {
> +                ClearChildSources ();
> +                Properties.Set<ISourceContents> ("Nereid.SourceContents", new InactiveContent (this));
> +            });
> +        }
> +
> +        #endregion
> +
> +        #region implemented abstract members of RemovableSource
> +
> +        public override void Import ()
> +        {
> +            throw new NotSupportedException ();
> +        }
> +
> +        public override bool CanUnmap {
> +            get { return false; }
> +        }
> +
> +        public override bool CanImport {
> +            get { return false; }
> +        }
> +
> +        public override bool IsReadOnly {
> +            get { return true; }
> +        }
> +
> +        public override long BytesUsed {
> +            get { return 0L; }
> +        }
> +
> +        public override long BytesCapacity {
> +            get { return 0L; }
> +        }
> +
> +        #endregion
> +
> +        #region implemented abstract members of DapSource
> +
> +        public override void AddChildSource (Source child) { }

Whenever you want to leave a method empty, please put the braces in 2nd and 3rd line, like this:

public override void AddChildSource (Source child)
{
}

> +
> +        public override void RemoveChildSource (Source child) { }

Shouldn't these 2 methods above throw NSE too?

> +
> +        protected override void AddTrackToDevice (DatabaseTrackInfo track, SafeUri fromUri)
> +        {
> +            throw new NotSupportedException ();
> +        }
> +
> +        internal bool TryDeviceInitialize(bool force, out DapSource source)
> +        {
> +            source = default(DapSource);
> +            DapSource src = null;
> +            try {
> +                src = (DapSource) Claimant.CreateInstance ();
> +                src.DeviceInitialize (Device, force);
> +                src.LoadDeviceContents ();
> +                src.AddinId = Claimant.Addin.Id;
> +                source = src;
> +            } catch (Exception e) {

What exception are you expecting here?

> +                Log.Error (e);
> +            }
> +
> +            return !object.ReferenceEquals (source, default(DapSource));

Missing space before parenthesis.


> +        }
> +        #endregion
> +
> +        internal void Claim ()
> +        {
> +            DapSource source;
> +            if (TryDeviceInitialize (true, out source)) {
> +                Service.SwapSource (this, source);
> +            }
> +        }
> +    }
> +}
> +
> diff --git a/src/Dap/Banshee.Dap/Makefile.am b/src/Dap/Banshee.Dap/Makefile.am
> index 11b9038..3e98e64 100644
> --- a/src/Dap/Banshee.Dap/Makefile.am
> +++ b/src/Dap/Banshee.Dap/Makefile.am
> @@ -21,6 +21,7 @@ SOURCES =  \
>  	Banshee.Dap/MediaGroupSource.cs \
>  	Banshee.Dap/MusicGroupSource.cs \
>  	Banshee.Dap/PodcastGroupSource.cs \
> +	Banshee.Dap/PotentialSource.cs \
>  	Banshee.Dap/RemovableSource.cs \
>  	Banshee.Dap/SyncPlaylist.cs \
>  	Banshee.Dap/VideoGroupSource.cs
> diff --git a/src/Dap/Banshee.Dap/Resources/ActiveSourceUI.xml b/src/Dap/Banshee.Dap/Resources/ActiveSourceUI.xml
> index eb5bfac..ce56a0c 100644
> --- a/src/Dap/Banshee.Dap/Resources/ActiveSourceUI.xml
> +++ b/src/Dap/Banshee.Dap/Resources/ActiveSourceUI.xml
> @@ -1,6 +1,7 @@
>  <ui>
>    <toolbar name="HeaderToolbar">
>      <placeholder name="SourceActions">
> +      <toolitem action="ClaimDapAction"/>
>        <toolitem action="UnmapSourceAction"/>
>        <toolitem action="SyncDapAction"/>
>      </placeholder>
> diff --git a/src/Dap/Banshee.Dap/Resources/GlobalUI.xml b/src/Dap/Banshee.Dap/Resources/GlobalUI.xml
> index 9f6869c..5287014 100644
> --- a/src/Dap/Banshee.Dap/Resources/GlobalUI.xml
> +++ b/src/Dap/Banshee.Dap/Resources/GlobalUI.xml
> @@ -1,6 +1,7 @@
>  <ui>
>    <popup name="RemovableSourceContextMenu" action="RemovableSourceContextMenuAction">
>      <placeholder name="AboveImportSource">
> +      <menuitem action="ClaimDapAction"/>
>        <menuitem action="SyncDapAction"/>
>      </placeholder>
>    </popup>
> -- 
> 1.8.5.5
>
Comment 44 Nicholas Little 2014-07-30 14:09:03 UTC
Created attachment 282068 [details] [review]
Cleanup and slight rework V4

(In reply to comment #43)
> (From update of attachment 281960 [details] [review])
> I guess this patch is ready, except for:
> a) style issues (I would have fixed this myself, but thinking that at some
> point in the future you will be reviewing patches of others, makes me want to
> point out everything now, so you keep practicing ;) )
> b) haven't tested it yet (will do it this afternoon)
> 
> So here we come with the style issues, inline:
<snip />
> 
> We usually write 'bgo' in lowercase.
> 

Fixed, URI acronyms in lowercase.

<snip />

> > +
> > +                    try {
> > +                        mtp_device = MtpDevice.Connect (v);
> > +                    } catch (Exception) {}
> 
> I don't like empty generic catches :)
> Please use a more specific type of exception (not base System.Exception), and
> log the exception.

Well, I just moved those lines. There's no documented exception in the Mtp library either. If the operation is unsuccessful then we should just get null back. If an exception does occur, then I think the only place it can come from is libmtp itself. I'm not sure if our managed catch block can handle those? 

Anyway, I removed the try/catch in the latest patch which actually sort of makes sense, MtpSource can't recover from a native library error at the moment...

<snip />

> > +
> > +        private void OnPropertyChanged (object o, PropertyChangeEventArgs args)
> > +        {
> > +            if (args.PropertyName == "Name")
> 
> We use braces for 'if' statements always, even if the line after it is just
> one.
> 

Again, not my lines but displaced because of my bad patch work. 

The latest version makes it obvious what I've changed with respect to DapContent and perhaps highlights that it and InactiveDapContent (was InactiveContent) should probably share more code.

<snip />

> > +
> > +        static InactiveContent () {
> 
> Braces of methods go in 2nd line.
> 

Fixed.

> > +            var generic = Catalog.GetString ("Your device appears to be in use by another program");
> > +            var claimit = String.Format (
> > +                @"<span weight=""bold"">{0}</span>",
> > +                Catalog.GetString ("Claim")
> > +            );
> > +            var pressit = String.Format (
> > +                Catalog.GetString ("Press the {0} button above to use it in Banshee"),
> > +                claimit
> > +            );
> > +            DefaultErrorString = string.Format(
> 
> Missing space before parenthesis.
> 

Fixed. Honestly, I blame monodevelop for this. Usually it Just Works beautifully which makes me lazy.

> > +                @"<span size=""large"">{0}." + "\n" + "{1}…</span>", generic, pressit
> > +            );
> > +            DapContent.Init ();
> > +        }
> > +
> > +        public static readonly string DefaultErrorString;
> 
> You sure this needs to be public? Check if you can put it as internal at least.
> 

Yeah, you're right, private it is. It's exposed to sublasses through their ErrorString virtual instance property.

<snip />

> > @@ -173,6 +180,29 @@ namespace Banshee.Dap
> >              Scheduler.Schedule (new MapDeviceJob (this, device));
> >          }
> >  
> > +        internal void SwapSource (DapSource oldSource, DapSource newSource)
> > +        {
> > +            if (oldSource.Device.Uuid != newSource.Device.Uuid) {
> > +                Log.ErrorFormat (
> > +                    "Dap.DapService: swap ignored from {0} to {1}.",
> > +                    oldSource.Device.Uuid, newSource.Device.Uuid
> > +                );
> > +                return;
> > +            }
> > +            Log.DebugFormat (
> > +                "Dap.DapService: Swapping {0} with UUID {1} for {2}",
> > +                oldSource.GetType ().Name, oldSource.Device.Uuid,
> > +                newSource.GetType ().Name
> > +            );
> > +            ThreadAssist.ProxyToMain (() => {
> > +                lock (sync) {
> > +                    ServiceManager.SourceManager.RemoveSource (oldSource);
> > +                    sources [newSource.Device.Uuid] = newSource;
> > +                    ServiceManager.SourceManager.AddSource (newSource);
> > +                }
> > +            });
> > +        }
> > +
> >          private class MapDeviceJob : IJob
> >          {
> >              IDevice device;
> > @@ -286,7 +316,8 @@ namespace Banshee.Dap
> >          {
> >              DapSource dap_source = args.Source as DapSource;
> >              if (dap_source != null) {
> > -                UnmapDevice (dap_source.Device.Uuid);
> > +                sources.Remove (dap_source.Device.Uuid);
> > +                dap_source.Dispose ();
> >              }
> >          }
> 
> Should this hunk above be part of this patch? Seems unrelated (maybe this is
> part of other patch that you have been working on, related to Dispose pattern).

Actually, this change was because the handler was removing the new, active source straight after mapping. I've tweaked the locking a little in the latest version so this change shouldn't be required.

<snip />

> > +            if (potential != null && potential.TryDeviceInitialize(false, out source)) {
> 
> Space before parenthesis.

MonoDevelop, I curse you!

<snip />

> > diff --git a/src/Dap/Banshee.Dap/Banshee.Dap/InvalidDeviceException.cs b/src/Dap/Banshee.Dap/Banshee.Dap/InvalidDeviceException.cs
> > index f8dbb76..7a05cc4 100644
> > --- a/src/Dap/Banshee.Dap/Banshee.Dap/InvalidDeviceException.cs
> > +++ b/src/Dap/Banshee.Dap/Banshee.Dap/InvalidDeviceException.cs
> > @@ -33,4 +33,8 @@ namespace Banshee.Dap
> >      public class InvalidDeviceException : ApplicationException
> >      {
> >      }
> > +
> > +    public class InvalidStateException : InvalidDeviceException
> > +    {
> > +    }
> 
> I prefer the name InvalidDeviceStateException.
> 

Cool.

<snip />

> > +
> > +using Banshee.Collection.Database;
> > +using Banshee.Dap.Gui;
> > +using Banshee.Hardware;
> > +using Banshee.Sources;
> > +using Banshee.Sources.Gui;
> > +using Hyena;
> > +using Mono.Addins;
> > +
> > +namespace Banshee.Dap
> > +{
> > +    internal class PotentialSource : DapSource
> > +    {
> > +        public readonly TypeExtensionNode Claimant;
> 
> I only see Claimant being used privately here, so 'private'?
> 
> 
> > +        public readonly DapService Service;
> 
> Same here maybe?
> 

Both private now.

With it being an internal class, I didn't think it made a huge amount of difference, i.e. public within the Dap assembly seemed ok to me. But I might be misinterpreting that.

> > +
> > +        internal PotentialSource (DapService service, TypeExtensionNode claim, IDevice device)
> > +        {
> > +            Claimant = claim;
> > +            Service = service;
> > +
> > +            IsTemporary = true;
> > +
> > +            SupportsPlaylists = false;
> > +            SupportsPodcasts = false;
> > +            SupportsVideo = false;
> > +
> > +            DeviceInitialize (device, false);
> > +            Initialize ();
> > +        }
> > +
> > +        #region overridden members of IDisposable
> > +
> > +        public override void Dispose ()
> > +        {
> > +            IsTemporary = true;
> 
> You have set IsTemporary to true in the constructor, do you think it could have
> changed during its life, before Dispose() is called?

No, it shouldn't be needed. I've removed the overload from the latest patch.

The prototype needed that when I decided they should be temporary to ensure that PrimarySource's Dispose method would clean them out, since they were likely created with IsTemporary set to false which is saved in the database and is initialised along with the DbId property, after I set IsTemporary = true in the constructor...

For this to work as intended, we also need DapSource to stop breaking the Dispose chain ;)

<snip />

> 
> Whenever you want to leave a method empty, please put the braces in 2nd and 3rd
> line, like this:
> 
> public override void AddChildSource (Source child)
> {
> }

Got it. So you don't change the signature line when adding a body?

> > +
> > +        public override void RemoveChildSource (Source child) { }
> 
> Shouldn't these 2 methods above throw NSE too?

They can't, PotentialSource does it's best to reduce the number of child sources (Supports* = false) but DapSource will always try to add the MusicGroupSource. Due to that, I simply disabled their implementations.

> > +
> > +        protected override void AddTrackToDevice (DatabaseTrackInfo track, SafeUri fromUri)
> > +        {
> > +            throw new NotSupportedException ();
> > +        }
> > +
> > +        internal bool TryDeviceInitialize(bool force, out DapSource source)
> > +        {
> > +            source = default(DapSource);
> > +            DapSource src = null;
> > +            try {
> > +                src = (DapSource) Claimant.CreateInstance ();
> > +                src.DeviceInitialize (Device, force);
> > +                src.LoadDeviceContents ();
> > +                src.AddinId = Claimant.Addin.Id;
> > +                source = src;
> > +            } catch (Exception e) {
> 
> What exception are you expecting here?

Exception! :) TryDeviceInitialize is essentially DapService.FindDeviceSource but there's only one extension node to try.

But, since PotentialSource knows it's purpose (mapped after an InvalidDeviceStateException) it knows that the Claimant is the correct type, so there should be no possibility of an InvalidDeviceException, only InvalidDeviceState and Exception itself.

> > +                Log.Error (e);
> > +            }
> > +
> > +            return !object.ReferenceEquals (source, default(DapSource));
> 
> Missing space before parenthesis.
> 

Fixed.

<snip />
Comment 45 Andrés G. Aragoneses (IRC: knocte) 2014-07-30 15:11:37 UTC
Comment on attachment 282068 [details] [review]
Cleanup and slight rework V4

This patch needs some last touches, especially because you broke the API and now some extensions don't build, i.e.:

./Banshee.Dap.AppleDevice/AppleDeviceSource.cs(73,30): error CS0115: `Banshee.Dap.AppleDevice.AppleDeviceSource.DeviceInitialize(Banshee.Hardware.IDevice)' is marked as an override but no suitable method found to override

Also because I just tested it, and I found a pretty big issue: when I pressed the "Claim" button, not only I saw an exception in the console, but Banshee froze (grey UI) for 34 (proof in the logs below) seconds (so didn't show the progress bar), and after that, the device didn't appear. The exception was:

[1 Error 16:57:00.876] Caught an exception - Banshee.Dap.InvalidDeviceStateException: An application exception has occurred. (in `Banshee.Dap.Mtp')
  at Banshee.Dap.Mtp.MtpSource.DeviceInitialize (IDevice device, Boolean force) [0x000da] in /home/knocte1404/bansheeSHINY/src/Dap/Banshee.Dap.Mtp/Banshee.Dap.Mtp/MtpSource.cs:105 
  at Banshee.Dap.PotentialSource.TryDeviceInitialize (Boolean force, Banshee.Dap.DapSource& source) [0x00020] in /home/knocte1404/bansheeSHINY/src/Dap/Banshee.Dap/Banshee.Dap/PotentialSource.cs:122 

(Take in account that any Log.Error() calls are like a tool that allows us devs find bugs. That is, any Log.Error() happening is a bug. If the above is not a bug, then use Log.Warning())

After banshee stopped being frozen (almost 34 seconds later), I also saw this one:

[1 Debug 16:57:00.924] Gio.Manager: received MountRemoved signal
[1 Error 16:57:00.925] Gio.Manager: ignoring VolumeChanged signal with no volume
[1 Debug 16:57:00.927] Gio.Manager: received MountRemoved signal
[1 Warn  16:57:00.927] 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).
Unable to open ~/.mtpz-data for reading, MTPZ disabled.Android device detected, assigning default bug flags
[1 Error 16:57:34.389] Caught an exception - 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/knocte1404/bansheeSHINY/src/Libraries/Mtp/Mtp/MtpDevice.cs:312 
  at Mtp.MtpDevice.get_BatteryLevel () [0x0000b] in /home/knocte1404/bansheeSHINY/src/Libraries/Mtp/Mtp/MtpDevice.cs:68 
  at Banshee.Dap.Mtp.MtpSource.DeviceInitialize (IDevice device, Boolean force) [0x0026c] in /home/knocte1404/bansheeSHINY/src/Dap/Banshee.Dap.Mtp/Banshee.Dap.Mtp/MtpSource.cs:146 

(Before this patch, this used to be a warning log, not error log)

And after the above, this was the log:

[1 Debug 16:57:34.390] Dap.DapService: Swapping PotentialSource with UUID /devices/pci0000:00/0000:00:1a.0/usb1/1-1/1-1.2 for MtpSource
[1 Debug 16:57:34.404] Creating Pango.Layout, configuring Cairo.Context
[20 Debug 16:57:34.421] Unmapping DAP source (/devices/pci0000:00/0000:00:1a.0/usb1/1-1/1-1.2)
[1 Debug 16:57:34.444] Creating Pango.Layout, configuring Cairo.Context
[1 Debug 16:57:34.463] Creating Pango.Layout, configuring Cairo.Context
[1 Debug 16:57:34.477] Creating Pango.Layout, configuring Cairo.Context
[1 Debug 16:57:34.641] Last.fm State Changed to NoAccount
[19 Error 16:57:36.453] Caught an exception - System.Threading.ThreadAbortException: Thread was being aborted (in `mscorlib')
  at System.UInt32.ToString (System.String format, IFormatProvider provider) [0x00000] in <filename unknown>:0 
  at System.String.FormatHelper (System.Text.StringBuilder result, IFormatProvider provider, System.String format, System.Object[] args) [0x00000] in <filename unknown>:0 
  at System.String.Format (IFormatProvider provider, System.String format, System.Object[] args) [0x00000] in <filename unknown>:0 
  at System.String.Format (System.String format, System.Object arg0, System.Object arg1) [0x00000] in <filename unknown>:0 
  at Banshee.Dap.Mtp.MtpTrackInfo.GetPathFromMtpTrack (Mtp.Track track) [0x00017] in /home/knocte1404/bansheeSHINY/src/Dap/Banshee.Dap.Mtp/Banshee.Dap.Mtp/MtpTrackInfo.cs:53 
  at Banshee.Dap.Mtp.MtpSource.LoadFromDevice () [0x000de] in /home/knocte1404/bansheeSHINY/src/Dap/Banshee.Dap.Mtp/Banshee.Dap.Mtp/MtpSource.cs:182 
[19 Error 16:57:36.453] Caught an exception - System.Threading.ThreadAbortException: Thread was being aborted (in `mscorlib')
  at Banshee.Dap.Mtp.MtpSource.LoadFromDevice () [0x002a2] in /home/knocte1404/bansheeSHINY/src/Dap/Banshee.Dap.Mtp/Banshee.Dap.Mtp/MtpSource.cs:214 
  at Banshee.Dap.DapSource.ThreadedLoadDeviceContents () [0x00025] in /home/knocte1404/bansheeSHINY/src/Dap/Banshee.Dap/Banshee.Dap/DapSource.cs:305



More inline:

(In reply to comment #44)
> Created an attachment (id=282068) [details] [review]
> Cleanup and slight rework V4
> 
> With it being an internal class, I didn't think it made a huge amount of
> difference

True, but the more we hide, the better. We shouldn't expose API that is not being used (unless we want to encourage its usage to external/extension developers).

> 
> For this to work as intended, we also need DapSource to stop breaking the
> Dispose chain ;)

Does this mean that this patch cannot be committed before bug 733927 is fixed?

> 
> > 
> > Whenever you want to leave a method empty, please put the braces in 2nd and 3rd
> > line, like this:
> > 
> > public override void AddChildSource (Source child)
> > {
> > }
> 
> Got it. So you don't change the signature line when adding a body?

Yes. And also because IIRC it's in the style guidelines (if not in banshee ones, in Mono ones).


> 
> > > +
> > > +        protected override void AddTrackToDevice (DatabaseTrackInfo track, SafeUri fromUri)
> > > +        {
> > > +            throw new NotSupportedException ();
> > > +        }
> > > +
> > > +        internal bool TryDeviceInitialize(bool force, out DapSource source)
> > > +        {
> > > +            source = default(DapSource);
> > > +            DapSource src = null;
> > > +            try {
> > > +                src = (DapSource) Claimant.CreateInstance ();
> > > +                src.DeviceInitialize (Device, force);
> > > +                src.LoadDeviceContents ();
> > > +                src.AddinId = Claimant.Addin.Id;
> > > +                source = src;
> > > +            } catch (Exception e) {
> > 
> > What exception are you expecting here?
> 
> Exception! :) TryDeviceInitialize is essentially DapService.FindDeviceSource
> but there's only one extension node to try.
> 
> But, since PotentialSource knows it's purpose (mapped after an
> InvalidDeviceStateException) it knows that the Claimant is the correct type, so
> there should be no possibility of an InvalidDeviceException, only
> InvalidDeviceState and Exception itself.

What do you mean with "Exception itself"? I don't find any "throw new Exception" in your patch, or anywhere in banshee (related to Dap/Mtp).
Comment 46 Andrés G. Aragoneses (IRC: knocte) 2014-07-30 16:46:16 UTC
BTW the 34 secs freeze should be because you must be interacting somewhere with libmtp in the main thread instead of spawning it, should be easy to fix.
Comment 47 Nicholas Little 2014-07-30 19:26:46 UTC
Created attachment 282102 [details] [review]
Cleanup and slight rework V5

(In reply to comment #45)
> (From update of attachment 282068 [details] [review])
> This patch needs some last touches, especially because you broke the API and
> now some extensions don't build, i.e.:
> 
> ./Banshee.Dap.AppleDevice/AppleDeviceSource.cs(73,30): error CS0115:
> `Banshee.Dap.AppleDevice.AppleDeviceSource.DeviceInitialize(Banshee.Hardware.IDevice)'
> is marked as an override but no suitable method found to override

Yep, that was a concern, I've added those changes to the Apple and Karma sources with this version, as I do think we're pretty close.

I just couldn't see a way to keep the old method around though.

> Also because I just tested it, and I found a pretty big issue: when I pressed
> the "Claim" button, not only I saw an exception in the console, but Banshee
> froze (grey UI) for 34 (proof in the logs below) seconds (so didn't show the
> progress bar), and after that, the device didn't appear. The exception was:
> 
> [1 Error 16:57:00.876] Caught an exception -
> Banshee.Dap.InvalidDeviceStateException: An application exception has occurred.
> (in `Banshee.Dap.Mtp')
>   at Banshee.Dap.Mtp.MtpSource.DeviceInitialize (IDevice device, Boolean force)
> [0x000da] in
> /home/knocte1404/bansheeSHINY/src/Dap/Banshee.Dap.Mtp/Banshee.Dap.Mtp/MtpSource.cs:105 
>   at Banshee.Dap.PotentialSource.TryDeviceInitialize (Boolean force,
> Banshee.Dap.DapSource& source) [0x00020] in
> /home/knocte1404/bansheeSHINY/src/Dap/Banshee.Dap/Banshee.Dap/PotentialSource.cs:122 

Did you see an unmount timeout error for your device just before that message?

I think what's happening is that the unmount is taking greater than the 5 seconds that we wait for so the Claim operation is still failing.

> (Take in account that any Log.Error() calls are like a tool that allows us devs
> find bugs. That is, any Log.Error() happening is a bug. If the above is not a
> bug, then use Log.Warning())

Indeed, converted.

> After banshee stopped being frozen (almost 34 seconds later), I also saw this
> one:
> 
> [1 Debug 16:57:00.924] Gio.Manager: received MountRemoved signal
> [1 Error 16:57:00.925] Gio.Manager: ignoring VolumeChanged signal with no
> volume
> [1 Debug 16:57:00.927] Gio.Manager: received MountRemoved signal
> [1 Warn  16:57:00.927] 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).
> Unable to open ~/.mtpz-data for reading, MTPZ disabled.Android device detected,
> assigning default bug flags
> [1 Error 16:57:34.389] Caught an exception - 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/knocte1404/bansheeSHINY/src/Libraries/Mtp/Mtp/MtpDevice.cs:312 
>   at Mtp.MtpDevice.get_BatteryLevel () [0x0000b] in
> /home/knocte1404/bansheeSHINY/src/Libraries/Mtp/Mtp/MtpDevice.cs:68 
>   at Banshee.Dap.Mtp.MtpSource.DeviceInitialize (IDevice device, Boolean force)
> [0x0026c] in
> /home/knocte1404/bansheeSHINY/src/Dap/Banshee.Dap.Mtp/Banshee.Dap.Mtp/MtpSource.cs:146 
> 
> (Before this patch, this used to be a warning log, not error log)
> 
> And after the above, this was the log:
> 
> [1 Debug 16:57:34.390] Dap.DapService: Swapping PotentialSource with UUID
> /devices/pci0000:00/0000:00:1a.0/usb1/1-1/1-1.2 for MtpSource
> [1 Debug 16:57:34.404] Creating Pango.Layout, configuring Cairo.Context
> [20 Debug 16:57:34.421] Unmapping DAP source
> (/devices/pci0000:00/0000:00:1a.0/usb1/1-1/1-1.2)
> [1 Debug 16:57:34.444] Creating Pango.Layout, configuring Cairo.Context
> [1 Debug 16:57:34.463] Creating Pango.Layout, configuring Cairo.Context
> [1 Debug 16:57:34.477] Creating Pango.Layout, configuring Cairo.Context
> [1 Debug 16:57:34.641] Last.fm State Changed to NoAccount
> [19 Error 16:57:36.453] Caught an exception -
> System.Threading.ThreadAbortException: Thread was being aborted (in `mscorlib')
>   at System.UInt32.ToString (System.String format, IFormatProvider provider)
> [0x00000] in <filename unknown>:0 
>   at System.String.FormatHelper (System.Text.StringBuilder result,
> IFormatProvider provider, System.String format, System.Object[] args) [0x00000]
> in <filename unknown>:0 
>   at System.String.Format (IFormatProvider provider, System.String format,
> System.Object[] args) [0x00000] in <filename unknown>:0 
>   at System.String.Format (System.String format, System.Object arg0,
> System.Object arg1) [0x00000] in <filename unknown>:0 
>   at Banshee.Dap.Mtp.MtpTrackInfo.GetPathFromMtpTrack (Mtp.Track track)
> [0x00017] in
> /home/knocte1404/bansheeSHINY/src/Dap/Banshee.Dap.Mtp/Banshee.Dap.Mtp/MtpTrackInfo.cs:53 
>   at Banshee.Dap.Mtp.MtpSource.LoadFromDevice () [0x000de] in
> /home/knocte1404/bansheeSHINY/src/Dap/Banshee.Dap.Mtp/Banshee.Dap.Mtp/MtpSource.cs:182 
> [19 Error 16:57:36.453] Caught an exception -
> System.Threading.ThreadAbortException: Thread was being aborted (in `mscorlib')
>   at Banshee.Dap.Mtp.MtpSource.LoadFromDevice () [0x002a2] in
> /home/knocte1404/bansheeSHINY/src/Dap/Banshee.Dap.Mtp/Banshee.Dap.Mtp/MtpSource.cs:214 
>   at Banshee.Dap.DapSource.ThreadedLoadDeviceContents () [0x00025] in
> /home/knocte1404/bansheeSHINY/src/Dap/Banshee.Dap/Banshee.Dap/DapSource.cs:305
> 

This is the OnSourceRemoved callback firing and killing our freshly active source because UnmapDevice doesn't know that we already removed it ourselves.

Here, the sequence is; SwapSource, UnmapDevice, Map, OnSourceRemoved, UnmapDevice, which seems a little silly.

> 
> More inline:
>
> > 
> > > > +
> > > > +        protected override void AddTrackToDevice (DatabaseTrackInfo track, SafeUri fromUri)
> > > > +        {
> > > > +            throw new NotSupportedException ();
> > > > +        }
> > > > +
> > > > +        internal bool TryDeviceInitialize(bool force, out DapSource source)
> > > > +        {
> > > > +            source = default(DapSource);
> > > > +            DapSource src = null;
> > > > +            try {
> > > > +                src = (DapSource) Claimant.CreateInstance ();
> > > > +                src.DeviceInitialize (Device, force);
> > > > +                src.LoadDeviceContents ();
> > > > +                src.AddinId = Claimant.Addin.Id;
> > > > +                source = src;
> > > > +            } catch (Exception e) {
> > > 
> > > What exception are you expecting here?
> > 
> > Exception! :) TryDeviceInitialize is essentially DapService.FindDeviceSource
> > but there's only one extension node to try.
> > 
> > But, since PotentialSource knows it's purpose (mapped after an
> > InvalidDeviceStateException) it knows that the Claimant is the correct type, so
> > there should be no possibility of an InvalidDeviceException, only
> > InvalidDeviceState and Exception itself.
> 
> What do you mean with "Exception itself"? I don't find any "throw new
> Exception" in your patch, or anywhere in banshee (related to Dap/Mtp).

The FindDeviceSource method in DapService handles three types of exception; InvalidDevice, InvalidDeviceState and Exception. I was following that.

(In reply to comment #46)
> BTW the 34 secs freeze should be because you must be interacting somewhere with
> libmtp in the main thread instead of spawning it, should be easy to fix.

You'd think so, but it turned out a lot more difficult. The problem is that we need to unmap a source, but straight afterwards map one in its place. With the OnSourceRemoved callback, we can't distinguish between a source that we removed ourselves and one that wants to be removed.

This version adds an event for DapSource, RequestUnmap, which DapService listens to. DapSource sends the signal in it's Eject call, at which point DapService calls UnmapDevice.

For the PotentialSource issues, what seems to be happening is that the claim is causing TryDeviceInitialize to be called twice, once from the ClaimDapAction and again from the OnHardwareDeviceChanged callback in DapService. That won't happen in this version.
Comment 48 Andrés G. Aragoneses (IRC: knocte) 2014-07-30 22:49:04 UTC
This still doesn't compile:

./Banshee.Dap.AppleDevice/AppleDeviceSource.cs(97,18): error CS1501: No overload for method `DeviceInitialize' takes `1' arguments

(I assume same happens with Karma)

So I tested this patch, in two ways.

First, I ran Banshee, and then I connected my Nexus 7. Result? The device source appeared, but the metadata of the tracks somehow couldn't be read. All the tracks where shown as "Unknown Title", "Unknown Artist", etc

The log of that is:


[1 Debug 22:38:27.217] Gio.Manager: received VolumeAdded signal
[1 Warn  22:38:27.222] Failed to load media-player-info file for 1
[18 Info  22:38:27.227] AppleDeviceSource is ignoring unmounted volume Nexus 7
Device 0 (VID=18d1 and PID=4e41) is a Google Inc (for Asus) Nexus 7 (MTP).
Unable to open ~/.mtpz-data for reading, MTPZ disabled.Android device detected, assigning default bug flags
[1 Debug 22:38:27.770] Gio.Manager: received MountAdded signal
[1 Error 22:38:27.771] Gio.Manager: ignoring VolumeChanged signal with no volume
[1 Debug 22:38:27.776] Gio.Manager: received MountAdded signal
[1 Warn  22:38:27.777] Failed to load media-player-info file for 1
[18 Error 22:38:27.955] Caught an exception - 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/knocte1404/bansheeSHINY/src/Libraries/Mtp/Mtp/MtpDevice.cs:312 
  at Mtp.MtpDevice.get_BatteryLevel () [0x0000b] in /home/knocte1404/bansheeSHINY/src/Libraries/Mtp/Mtp/MtpDevice.cs:68 
  at Banshee.Dap.Mtp.MtpSource.DeviceInitialize (IDevice device, Boolean force) [0x0026c] in /home/knocte1404/bansheeSHINY/src/Dap/Banshee.Dap.Mtp/Banshee.Dap.Mtp/MtpSource.cs:147 
[18 Debug 22:38:27.956] 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
[18 Debug 22:38:27.958] DapService: mapping MtpSource-/devices/pci0000:00/0000:00:1a.0/usb1/1-1/1-1.2
[1 Debug 22:38:33.161] Creating Pango.Layout, configuring Cairo.Context
[1 Debug 22:38:39.083] Creating Pango.Layout, configuring Cairo.Context
[1 Debug 22:38:39.169] Creating Pango.Layout, configuring Cairo.Context
[1 Debug 22:38:39.173] Creating Pango.Layout, configuring Cairo.Context
[1 Debug 22:38:40.311] Creating Pango.Layout, configuring Cairo.Context


Then I closed Banshee, and without disconnecting the device, I ran Banshee again. The result is that the device source appeared, but as unavailable (with the Claim option). The log of that was:

[1 Info  22:40:09.112] nereid Client Started
[1 Debug 22:40:09.113] Delayed Initializating Banshee.MediaEngine.PlayerEngineService
[1 Debug 22:40:09.400] (libbanshee:player) Audiosink has volume: YES
[1 Debug 22:40:09.417] (libbanshee:player) Using system (gst-plugins-good) equalizer element
[1 Debug 22:40:09.544] Player state change: NotReady -> Ready
[1 Debug 22:40:09.547] Loaded equalizer presets: 0.000153
[1 Debug 22:40:09.550] Selected equalizer: Rock
[1 Debug 22:40:09.557] Player state change: Ready -> Idle
[1 Debug 22:40:09.561] (libbanshee:player) Disabled ReplayGain
[1 Info  22:40:09.563] GStreamer version 1.2.4.0, gapless: True, replaygain: False
[1 Debug 22:40:09.564] Delayed Initializating Banshee.Dap.DapService
[1 Debug 22:40:09.575] Dap support extension loaded: Banshee.Dap.Mtp
[1 Debug 22:40:09.577] Dap support extension loaded: Banshee.Dap.AppleDevice
[1 Debug 22:40:09.578] Dap support extension loaded: Banshee.Dap.MassStorage
[1 Warn  22:40:09.580] Failed to load media-player-info file for 1
[1 Debug 22:40:09.584] Delayed Initializating Banshee.Podcasting.PodcastService
[1 Debug 22:40:09.667] Delayed Initializating Banshee.Daap.DaapService
[8 Debug 22:40:09.683] Refreshing any podcasts that haven't been updated in over an hour
[1 Debug 22:40:09.823] Creating Pango.Layout, configuring Cairo.Context
Device 0 (VID=18d1 and PID=4e41) is a Google Inc (for Asus) Nexus 7 (MTP).
[7 Error 22:40:09.827] Dap.DapService: invalid state, mapping potential source for Nexus 7
[7 Debug 22:40:09.978] Found DAP support (Banshee.Dap.PotentialSource) for device Nexus 7 and Uuid /devices/pci0000:00/0000:00:1a.0/usb1/1-1/1-1.2
[7 Debug 22:40:09.980] DapService: mapping PotentialSource-/devices/pci0000:00/0000:00:1a.0/usb1/1-1/1-1.2
[7 Info  22:40:09.982] AppleDeviceSource is ignoring unmounted volume 11 GB Volume
[7 Info  22:40:09.988] AppleDeviceSource is ignoring unmounted volume 16 GB Volume
[7 Info  22:40:09.993] AppleDeviceSource is ignoring unmounted volume Windows8_OS
[1 Debug 22:40:10.024] Gio.Manager: received MountAdded signal
[1 Warn  22:40:10.025] Failed to load media-player-info file for 1
[11 Debug 22:40:10.026] PotentialSource: Creating Instance
[11 Debug 22:40:10.027] PotentialSource: Initializing Device
Device 0 (VID=18d1 and PID=4e41) is a Google Inc (for Asus) Nexus 7 (MTP).
[11 Warn  22:40:10.030]  - Banshee.Dap.InvalidDeviceStateException: An application exception has occurred. (in `Banshee.Dap.Mtp')
  at Banshee.Dap.Mtp.MtpSource.DeviceInitialize (IDevice device, Boolean force) [0x000da] in /home/knocte1404/bansheeSHINY/src/Dap/Banshee.Dap.Mtp/Banshee.Dap.Mtp/MtpSource.cs:106 
  at Banshee.Dap.PotentialSource.TryDeviceInitialize (Boolean force, Banshee.Dap.DapSource& source) [0x00059] in /home/knocte1404/bansheeSHINY/src/Dap/Banshee.Dap/Banshee.Dap/PotentialSource.cs:133 
[1 Debug 22:40:10.684] Finished - Startup Job
[1 Debug 22:40:10.689] Starting - Downloading Artists' Images
[13 Debug 22:40:10.755] Finished - Downloading Artists' Images
[13 Debug 22:40:10.767] Starting - Downloading Cover Art
[15 Debug 22:40:10.768] Finished - Downloading Cover Art
[17 Debug 22:40:11.359] DAAP Proxy listening for connections on port 8089
[1 Debug 22:40:14.385] Creating Pango.Layout, configuring Cairo.Context
[1 Debug 22:40:15.144] Starting - Saving Metadata to File
[18 Debug 22:40:15.148] Finished - Saving Metadata to File

(I assume we need to convert the Error above to a Warning, right?)

After that, I wanted to press the button "Claim", but suddenly I noticed that the UI was kind of frozen.
I waited some seconds, and it became unfrozen, but I saw this in the console:

[1 Debug 22:41:40.387] Creating Pango.Layout, configuring Cairo.Context
Domain: 'Gtk' Level: Critical
Message: _gtk_widget_captured_event: assertion 'WIDGET_REALIZED_FOR_EVENT (widget, event)' failed
Trace follows:
   at GLib.Log.PrintTraceLogFunction(System.String domain, LogLevelFlags level, System.String message)
   at GLib.Log.NativeCallback(IntPtr log_domain_native, LogLevelFlags flags, IntPtr message_native, IntPtr user_data)
   at Gtk.Application.gtk_main()
   at Gtk.Application.Run()
   at Banshee.Gui.GtkBaseClient.Run()
   at Banshee.Gui.GtkBaseClient.Startup()
   at Hyena.Gui.CleanRoomStartup.Startup(Hyena.Gui.StartupInvocationHandler startup)
   at Banshee.Gui.GtkBaseClient.Startup()
   at Banshee.Gui.GtkBaseClient.Startup(System.String[] args)
   at Nereid.Client.Main(System.String[] args)
Domain: 'Gtk' Level: Critical
Message: _gtk_widget_captured_event: assertion 'WIDGET_REALIZED_FOR_EVENT (widget, event)' failed
Trace follows:
   at GLib.Log.PrintTraceLogFunction(System.String domain, LogLevelFlags level, System.String message)
   at GLib.Log.NativeCallback(IntPtr log_domain_native, LogLevelFlags flags, IntPtr message_native, IntPtr user_data)
   at Gtk.Application.gtk_main()
   at Gtk.Application.Run()
   at Banshee.Gui.GtkBaseClient.Run()
   at Banshee.Gui.GtkBaseClient.Startup()
   at Hyena.Gui.CleanRoomStartup.Startup(Hyena.Gui.StartupInvocationHandler startup)
   at Banshee.Gui.GtkBaseClient.Startup()
   at Banshee.Gui.GtkBaseClient.Startup(System.String[] args)
   at Nereid.Client.Main(System.String[] args)
Domain: 'Gtk' Level: Critical
Message: _gtk_widget_captured_event: assertion 'WIDGET_REALIZED_FOR_EVENT (widget, event)' failed
Trace follows:
   at GLib.Log.PrintTraceLogFunction(System.String domain, LogLevelFlags level, System.String message)
   at GLib.Log.NativeCallback(IntPtr log_domain_native, LogLevelFlags flags, IntPtr message_native, IntPtr user_data)
   at Gtk.Application.gtk_main()
   at Gtk.Application.Run()
   at Banshee.Gui.GtkBaseClient.Run()
   at Banshee.Gui.GtkBaseClient.Startup()
   at Hyena.Gui.CleanRoomStartup.Startup(Hyena.Gui.StartupInvocationHandler startup)
   at Banshee.Gui.GtkBaseClient.Startup()
   at Banshee.Gui.GtkBaseClient.Startup(System.String[] args)
   at Nereid.Client.Main(System.String[] args)
Domain: 'Gtk' Level: Critical
Message: _gtk_widget_captured_event: assertion 'WIDGET_REALIZED_FOR_EVENT (widget, event)' failed
Trace follows:
   at GLib.Log.PrintTraceLogFunction(System.String domain, LogLevelFlags level, System.String message)
   at GLib.Log.NativeCallback(IntPtr log_domain_native, LogLevelFlags flags, IntPtr message_native, IntPtr user_data)
   at Gtk.Application.gtk_main()
   at Gtk.Application.Run()
   at Banshee.Gui.GtkBaseClient.Run()
   at Banshee.Gui.GtkBaseClient.Startup()
   at Hyena.Gui.CleanRoomStartup.Startup(Hyena.Gui.StartupInvocationHandler startup)
   at Banshee.Gui.GtkBaseClient.Startup()
   at Banshee.Gui.GtkBaseClient.Startup(System.String[] args)
   at Nereid.Client.Main(System.String[] args)
Domain: 'Gtk' Level: Critical
Message: _gtk_widget_captured_event: assertion 'WIDGET_REALIZED_FOR_EVENT (widget, event)' failed
Trace follows:
   at GLib.Log.PrintTraceLogFunction(System.String domain, LogLevelFlags level, System.String message)
   at GLib.Log.NativeCallback(IntPtr log_domain_native, LogLevelFlags flags, IntPtr message_native, IntPtr user_data)
   at Gtk.Application.gtk_main()
   at Gtk.Application.Run()
   at Banshee.Gui.GtkBaseClient.Run()
   at Banshee.Gui.GtkBaseClient.Startup()
   at Hyena.Gui.CleanRoomStartup.Startup(Hyena.Gui.StartupInvocationHandler startup)
   at Banshee.Gui.GtkBaseClient.Startup()
   at Banshee.Gui.GtkBaseClient.Startup(System.String[] args)
   at Nereid.Client.Main(System.String[] args)
Domain: 'Gtk' Level: Critical
Message: _gtk_widget_captured_event: assertion 'WIDGET_REALIZED_FOR_EVENT (widget, event)' failed
Trace follows:
   at GLib.Log.PrintTraceLogFunction(System.String domain, LogLevelFlags level, System.String message)
   at GLib.Log.NativeCallback(IntPtr log_domain_native, LogLevelFlags flags, IntPtr message_native, IntPtr user_data)
   at Gtk.Application.gtk_main()
   at Gtk.Application.Run()
   at Banshee.Gui.GtkBaseClient.Run()
   at Banshee.Gui.GtkBaseClient.Startup()
   at Hyena.Gui.CleanRoomStartup.Startup(Hyena.Gui.StartupInvocationHandler startup)
   at Banshee.Gui.GtkBaseClient.Startup()
   at Banshee.Gui.GtkBaseClient.Startup(System.String[] args)
   at Nereid.Client.Main(System.String[] args)
Domain: 'Gtk' Level: Critical
Message: _gtk_widget_captured_event: assertion 'WIDGET_REALIZED_FOR_EVENT (widget, event)' failed
Trace follows:
   at GLib.Log.PrintTraceLogFunction(System.String domain, LogLevelFlags level, System.String message)
   at GLib.Log.NativeCallback(IntPtr log_domain_native, LogLevelFlags flags, IntPtr message_native, IntPtr user_data)
   at Gtk.Application.gtk_main()
   at Gtk.Application.Run()
   at Banshee.Gui.GtkBaseClient.Run()
   at Banshee.Gui.GtkBaseClient.Startup()
   at Hyena.Gui.CleanRoomStartup.Startup(Hyena.Gui.StartupInvocationHandler startup)
   at Banshee.Gui.GtkBaseClient.Startup()
   at Banshee.Gui.GtkBaseClient.Startup(System.String[] args)
   at Nereid.Client.Main(System.String[] args)
Domain: 'Gtk' Level: Critical
Message: _gtk_widget_captured_event: assertion 'WIDGET_REALIZED_FOR_EVENT (widget, event)' failed
Trace follows:
   at GLib.Log.PrintTraceLogFunction(System.String domain, LogLevelFlags level, System.String message)
   at GLib.Log.NativeCallback(IntPtr log_domain_native, LogLevelFlags flags, IntPtr message_native, IntPtr user_data)
   at Gtk.Application.gtk_main()
   at Gtk.Application.Run()
   at Banshee.Gui.GtkBaseClient.Run()
   at Banshee.Gui.GtkBaseClient.Startup()
   at Hyena.Gui.CleanRoomStartup.Startup(Hyena.Gui.StartupInvocationHandler startup)
   at Banshee.Gui.GtkBaseClient.Startup()
   at Banshee.Gui.GtkBaseClient.Startup(System.String[] args)
   at Nereid.Client.Main(System.String[] args)
Domain: 'Gtk' Level: Critical
Message: _gtk_widget_captured_event: assertion 'WIDGET_REALIZED_FOR_EVENT (widget, event)' failed
Trace follows:
   at GLib.Log.PrintTraceLogFunction(System.String domain, LogLevelFlags level, System.String message)
   at GLib.Log.NativeCallback(IntPtr log_domain_native, LogLevelFlags flags, IntPtr message_native, IntPtr user_data)
   at Gtk.Application.gtk_main()
   at Gtk.Application.Run()
   at Banshee.Gui.GtkBaseClient.Run()
   at Banshee.Gui.GtkBaseClient.Startup()
   at Hyena.Gui.CleanRoomStartup.Startup(Hyena.Gui.StartupInvocationHandler startup)
   at Banshee.Gui.GtkBaseClient.Startup()
   at Banshee.Gui.GtkBaseClient.Startup(System.String[] args)
   at Nereid.Client.Main(System.String[] args)
Domain: 'Gtk' Level: Critical
Message: _gtk_widget_captured_event: assertion 'WIDGET_REALIZED_FOR_EVENT (widget, event)' failed
Trace follows:
   at GLib.Log.PrintTraceLogFunction(System.String domain, LogLevelFlags level, System.String message)
   at GLib.Log.NativeCallback(IntPtr log_domain_native, LogLevelFlags flags, IntPtr message_native, IntPtr user_data)
   at Gtk.Application.gtk_main()
   at Gtk.Application.Run()
   at Banshee.Gui.GtkBaseClient.Run()
   at Banshee.Gui.GtkBaseClient.Startup()
   at Hyena.Gui.CleanRoomStartup.Startup(Hyena.Gui.StartupInvocationHandler startup)
   at Banshee.Gui.GtkBaseClient.Startup()
   at Banshee.Gui.GtkBaseClient.Startup(System.String[] args)
   at Nereid.Client.Main(System.String[] args)
Domain: 'Gtk' Level: Critical
Message: _gtk_widget_captured_event: assertion 'WIDGET_REALIZED_FOR_EVENT (widget, event)' failed
Trace follows:
   at GLib.Log.PrintTraceLogFunction(System.String domain, LogLevelFlags level, System.String message)
   at GLib.Log.NativeCallback(IntPtr log_domain_native, LogLevelFlags flags, IntPtr message_native, IntPtr user_data)
   at Gtk.Application.gtk_main()
   at Gtk.Application.Run()
   at Banshee.Gui.GtkBaseClient.Run()
   at Banshee.Gui.GtkBaseClient.Startup()
   at Hyena.Gui.CleanRoomStartup.Startup(Hyena.Gui.StartupInvocationHandler startup)
   at Banshee.Gui.GtkBaseClient.Startup()
   at Banshee.Gui.GtkBaseClient.Startup(System.String[] args)
   at Nereid.Client.Main(System.String[] args)

Then I could press the "Claim" button, and the result is that the device source disappeared, there was no progress bar to tell me that the claim was in progress, and after about 40 seconds, the new source appeared, and this is the log of all that:

pressed claim:

[19 Debug 22:42:21.638] PotentialSource: Creating Instance
[19 Debug 22:42:21.710] PotentialSource: Initializing Device
Device 0 (VID=18d1 and PID=4e41) is a Google Inc (for Asus) Nexus 7 (MTP).
[1 Debug 22:42:24.908] Gio.Manager: received MountRemoved signal
[1 Error 22:42:24.908] Gio.Manager: ignoring VolumeChanged signal with no volume
[1 Debug 22:42:25.048] Gio.Manager: received MountRemoved signal
Unable to open ~/.mtpz-data for reading, MTPZ disabled.Android device detected, assigning default bug flags
[1 Warn  22:42:25.128] Failed to load media-player-info file for 1
[19 Error 22:42:59.999] Caught an exception - 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/knocte1404/bansheeSHINY/src/Libraries/Mtp/Mtp/MtpDevice.cs:312 
  at Mtp.MtpDevice.get_BatteryLevel () [0x0000b] in /home/knocte1404/bansheeSHINY/src/Libraries/Mtp/Mtp/MtpDevice.cs:68 
  at Banshee.Dap.Mtp.MtpSource.DeviceInitialize (IDevice device, Boolean force) [0x0026c] in /home/knocte1404/bansheeSHINY/src/Dap/Banshee.Dap.Mtp/Banshee.Dap.Mtp/MtpSource.cs:147 
[19 Debug 22:43:00.010] PotentialSource: Loading Contents
[19 Debug 22:43:00.026] PotentialSource: Success, new Source Nexus 7
[19 Debug 22:43:00.051] Dap.DapService: Swapping PotentialSource with UUID /devices/pci0000:00/0000:00:1a.0/usb1/1-1/1-1.2 for MtpSource
[19 Debug 22:43:00.051] Unmapping DAP source (/devices/pci0000:00/0000:00:1a.0/usb1/1-1/1-1.2)
[19 Debug 22:43:00.631] DapService: mapping MtpSource-/devices/pci0000:00/0000:00:1a.0/usb1/1-1/1-1.2
[1 Debug 22:43:00.828] Creating Pango.Layout, configuring Cairo.Context
[1 Debug 22:43:00.869] Creating Pango.Layout, configuring Cairo.Context
[1 Debug 22:43:00.924] Creating Pango.Layout, configuring Cairo.Context
[1 Debug 22:43:00.983] Creating Pango.Layout, configuring Cairo.Context
[1 Debug 22:43:01.307] Last.fm State Changed to NoAccount


So I guess this is still needs-work, but feels closer to resolution.

BTW, some more things:

> -                    ThreadAssist.ProxyToMain (delegate {
> -
> -                        ServiceManager.SourceManager.AddSource (source);
> -                        source.NotifyUser ();
> -
> -                        // If there are any queued device commands, see if they are to be
> -                        // handled by this new DAP (e.g. --device-activate=file:///media/disk)
> -                        try {
> -                            if (service.unhandled_device_commands != null) {
> -                                foreach (DeviceCommand command in service.unhandled_device_commands) {
> -                                    if (source.CanHandleDeviceCommand (command)) {
> -                                        service.HandleDeviceCommand (source, command.Action);
> -                                        service.unhandled_device_commands.Remove (command);
> -                                        if (service.unhandled_device_commands.Count == 0) {
> -                                            service.unhandled_device_commands = null;
> -                                        }
> -                                        break;
> -                                    }
> -                                }
> -                            }
> -                        } catch (Exception e) {
> -                            Log.Error (e);
> -                        }
> -                    });
> +                    service.MapSource (source);

Given that the above is mainly moving code around, in order to have a clearer diff of what you're really changing, I've committed a pre-refactoring now: https://git.gnome.org/browse/banshee/commit/?id=e13c46f3e889090b955485f23b62f4e6a4e8c621


> +        private object _lock_ = new { };

What kind of notation is that?? please use simply "lock_object" ;) and initialize via 'new object ();' not weird things.

> +            Claimant = claim;

Shouldn't the parameter be named "claimant" too?

> The FindDeviceSource method in DapService handles three types of exception;
> InvalidDevice, InvalidDeviceState and Exception. I was following that.

Then please separate the catch in three, send the exception to Log.Warn() for the first two, and send to Log.Err() for Exception.
(Same for other parts of the code where you catch(Exception )
Comment 49 Nicholas Little 2014-07-31 14:31:20 UTC
Created attachment 282147 [details] [review]
Cleanup and slight rework V6

(In reply to comment #48)
> This still doesn't compile:
> 
> ./Banshee.Dap.AppleDevice/AppleDeviceSource.cs(97,18): error CS1501: No
> overload for method `DeviceInitialize' takes `1' arguments
> 
> (I assume same happens with Karma)
> 

Sorry, careless. They should build in this version.

> So I tested this patch, in two ways.
> 
> First, I ran Banshee, and then I connected my Nexus 7. Result? The device
> source appeared, but the metadata of the tracks somehow couldn't be read. All
> the tracks where shown as "Unknown Title", "Unknown Artist", etc
> 
> The log of that is:
> 
> 
> [1 Debug 22:38:27.217] Gio.Manager: received VolumeAdded signal
> [1 Warn  22:38:27.222] Failed to load media-player-info file for 1
> [18 Info  22:38:27.227] AppleDeviceSource is ignoring unmounted volume Nexus 7
> Device 0 (VID=18d1 and PID=4e41) is a Google Inc (for Asus) Nexus 7 (MTP).
> Unable to open ~/.mtpz-data for reading, MTPZ disabled.Android device detected,
> assigning default bug flags
> [1 Debug 22:38:27.770] Gio.Manager: received MountAdded signal
> [1 Error 22:38:27.771] Gio.Manager: ignoring VolumeChanged signal with no
> volume
> [1 Debug 22:38:27.776] Gio.Manager: received MountAdded signal
> [1 Warn  22:38:27.777] Failed to load media-player-info file for 1
> [18 Error 22:38:27.955] Caught an exception - 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/knocte1404/bansheeSHINY/src/Libraries/Mtp/Mtp/MtpDevice.cs:312 
>   at Mtp.MtpDevice.get_BatteryLevel () [0x0000b] in
> /home/knocte1404/bansheeSHINY/src/Libraries/Mtp/Mtp/MtpDevice.cs:68 
>   at Banshee.Dap.Mtp.MtpSource.DeviceInitialize (IDevice device, Boolean force)
> [0x0026c] in
> /home/knocte1404/bansheeSHINY/src/Dap/Banshee.Dap.Mtp/Banshee.Dap.Mtp/MtpSource.cs:147 
> [18 Debug 22:38:27.956] 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
> [18 Debug 22:38:27.958] DapService: mapping
> MtpSource-/devices/pci0000:00/0000:00:1a.0/usb1/1-1/1-1.2
> [1 Debug 22:38:33.161] Creating Pango.Layout, configuring Cairo.Context
> [1 Debug 22:38:39.083] Creating Pango.Layout, configuring Cairo.Context
> [1 Debug 22:38:39.169] Creating Pango.Layout, configuring Cairo.Context
> [1 Debug 22:38:39.173] Creating Pango.Layout, configuring Cairo.Context
> [1 Debug 22:38:40.311] Creating Pango.Layout, configuring Cairo.Context
> 

The battery status error is bothering me. I took a look at the Mtp library and produced a small patch, can you try with both this one and that one (next comment) applied please?

> Then I closed Banshee, and without disconnecting the device, I ran Banshee
> again. The result is that the device source appeared, but as unavailable (with
> the Claim option). The log of that was:

So it was mounted in nautilus at this point. This is sounding more and more like a race. The annoying thing is that we're not even trying to win it anymore!

> [1 Info  22:40:09.112] nereid Client Started
> [1 Debug 22:40:09.113] Delayed Initializating
> Banshee.MediaEngine.PlayerEngineService
> [1 Debug 22:40:09.400] (libbanshee:player) Audiosink has volume: YES
> [1 Debug 22:40:09.417] (libbanshee:player) Using system (gst-plugins-good)
> equalizer element
> [1 Debug 22:40:09.544] Player state change: NotReady -> Ready
> [1 Debug 22:40:09.547] Loaded equalizer presets: 0.000153
> [1 Debug 22:40:09.550] Selected equalizer: Rock
> [1 Debug 22:40:09.557] Player state change: Ready -> Idle
> [1 Debug 22:40:09.561] (libbanshee:player) Disabled ReplayGain
> [1 Info  22:40:09.563] GStreamer version 1.2.4.0, gapless: True, replaygain:
> False
> [1 Debug 22:40:09.564] Delayed Initializating Banshee.Dap.DapService
> [1 Debug 22:40:09.575] Dap support extension loaded: Banshee.Dap.Mtp
> [1 Debug 22:40:09.577] Dap support extension loaded: Banshee.Dap.AppleDevice
> [1 Debug 22:40:09.578] Dap support extension loaded: Banshee.Dap.MassStorage
> [1 Warn  22:40:09.580] Failed to load media-player-info file for 1
> [1 Debug 22:40:09.584] Delayed Initializating Banshee.Podcasting.PodcastService
> [1 Debug 22:40:09.667] Delayed Initializating Banshee.Daap.DaapService
> [8 Debug 22:40:09.683] Refreshing any podcasts that haven't been updated in
> over an hour
> [1 Debug 22:40:09.823] Creating Pango.Layout, configuring Cairo.Context
> Device 0 (VID=18d1 and PID=4e41) is a Google Inc (for Asus) Nexus 7 (MTP).
> [7 Error 22:40:09.827] Dap.DapService: invalid state, mapping potential source
> for Nexus 7
> [7 Debug 22:40:09.978] Found DAP support (Banshee.Dap.PotentialSource) for
> device Nexus 7 and Uuid /devices/pci0000:00/0000:00:1a.0/usb1/1-1/1-1.2
> [7 Debug 22:40:09.980] DapService: mapping
> PotentialSource-/devices/pci0000:00/0000:00:1a.0/usb1/1-1/1-1.2
> [7 Info  22:40:09.982] AppleDeviceSource is ignoring unmounted volume 11 GB
> Volume
> [7 Info  22:40:09.988] AppleDeviceSource is ignoring unmounted volume 16 GB
> Volume
> [7 Info  22:40:09.993] AppleDeviceSource is ignoring unmounted volume
> Windows8_OS
> [1 Debug 22:40:10.024] Gio.Manager: received MountAdded signal
> [1 Warn  22:40:10.025] Failed to load media-player-info file for 1
> [11 Debug 22:40:10.026] PotentialSource: Creating Instance
> [11 Debug 22:40:10.027] PotentialSource: Initializing Device
> Device 0 (VID=18d1 and PID=4e41) is a Google Inc (for Asus) Nexus 7 (MTP).
> [11 Warn  22:40:10.030]  - Banshee.Dap.InvalidDeviceStateException: An
> application exception has occurred. (in `Banshee.Dap.Mtp')
>   at Banshee.Dap.Mtp.MtpSource.DeviceInitialize (IDevice device, Boolean force)
> [0x000da] in
> /home/knocte1404/bansheeSHINY/src/Dap/Banshee.Dap.Mtp/Banshee.Dap.Mtp/MtpSource.cs:106 
>   at Banshee.Dap.PotentialSource.TryDeviceInitialize (Boolean force,
> Banshee.Dap.DapSource& source) [0x00059] in
> /home/knocte1404/bansheeSHINY/src/Dap/Banshee.Dap/Banshee.Dap/PotentialSource.cs:133 
> [1 Debug 22:40:10.684] Finished - Startup Job
> [1 Debug 22:40:10.689] Starting - Downloading Artists' Images
> [13 Debug 22:40:10.755] Finished - Downloading Artists' Images
> [13 Debug 22:40:10.767] Starting - Downloading Cover Art
> [15 Debug 22:40:10.768] Finished - Downloading Cover Art
> [17 Debug 22:40:11.359] DAAP Proxy listening for connections on port 8089
> [1 Debug 22:40:14.385] Creating Pango.Layout, configuring Cairo.Context
> [1 Debug 22:40:15.144] Starting - Saving Metadata to File
> [18 Debug 22:40:15.148] Finished - Saving Metadata to File
> 
> (I assume we need to convert the Error above to a Warning, right?)
> 

Yep, it's a Warning in this patch, but probably could just be an Info, since we do expect Mtp users to mount their devices from time to time.

> After that, I wanted to press the button "Claim", but suddenly I noticed that
> the UI was kind of frozen.
> I waited some seconds, and it became unfrozen, but I saw this in the console:
> 
> [1 Debug 22:41:40.387] Creating Pango.Layout, configuring Cairo.Context
> Domain: 'Gtk' Level: Critical
> Message: _gtk_widget_captured_event: assertion 'WIDGET_REALIZED_FOR_EVENT
> (widget, event)' failed
> Trace follows:
>    at GLib.Log.PrintTraceLogFunction(System.String domain, LogLevelFlags level,
> System.String message)
>    at GLib.Log.NativeCallback(IntPtr log_domain_native, LogLevelFlags flags,
> IntPtr message_native, IntPtr user_data)
>    at Gtk.Application.gtk_main()
>    at Gtk.Application.Run()
>    at Banshee.Gui.GtkBaseClient.Run()
>    at Banshee.Gui.GtkBaseClient.Startup()
>    at Hyena.Gui.CleanRoomStartup.Startup(Hyena.Gui.StartupInvocationHandler
> startup)
>    at Banshee.Gui.GtkBaseClient.Startup()
>    at Banshee.Gui.GtkBaseClient.Startup(System.String[] args)
>    at Nereid.Client.Main(System.String[] args)

Wow, that's ugly. Please test with all of the Online Media extensions disabled and see if this goes away.

I ask, because LastFM actually crashes gnome-shell for me (the "Oh no! Something went wrong..." logout screen).

<snip />

> 
> Then I could press the "Claim" button, and the result is that the device source
> disappeared, there was no progress bar to tell me that the claim was in
> progress, and after about 40 seconds, the new source appeared, and this is the
> log of all that:

40 seconds is a really long time.

I've added some status messages to this one, sorry for the lack in the last version. In addition when we do a Claim, instead of just mapping the new source we try to make it active too. The effect should be that the potential source with a status message "Trying to Claim Your Device..." is seamlessly replaced by the active one with its "Loading Name - Track x of y" message and the familiar DAP preferences screen.

> pressed claim:
> 
> [19 Debug 22:42:21.638] PotentialSource: Creating Instance
> [19 Debug 22:42:21.710] PotentialSource: Initializing Device
> Device 0 (VID=18d1 and PID=4e41) is a Google Inc (for Asus) Nexus 7 (MTP).
> [1 Debug 22:42:24.908] Gio.Manager: received MountRemoved signal
> [1 Error 22:42:24.908] Gio.Manager: ignoring VolumeChanged signal with no
> volume
> [1 Debug 22:42:25.048] Gio.Manager: received MountRemoved signal
> Unable to open ~/.mtpz-data for reading, MTPZ disabled.Android device detected,
> assigning default bug flags
> [1 Warn  22:42:25.128] Failed to load media-player-info file for 1
> [19 Error 22:42:59.999] Caught an exception - 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/knocte1404/bansheeSHINY/src/Libraries/Mtp/Mtp/MtpDevice.cs:312 
>   at Mtp.MtpDevice.get_BatteryLevel () [0x0000b] in
> /home/knocte1404/bansheeSHINY/src/Libraries/Mtp/Mtp/MtpDevice.cs:68 
>   at Banshee.Dap.Mtp.MtpSource.DeviceInitialize (IDevice device, Boolean force)
> [0x0026c] in
> /home/knocte1404/bansheeSHINY/src/Dap/Banshee.Dap.Mtp/Banshee.Dap.Mtp/MtpSource.cs:147 
> [19 Debug 22:43:00.010] PotentialSource: Loading Contents
> [19 Debug 22:43:00.026] PotentialSource: Success, new Source Nexus 7
> [19 Debug 22:43:00.051] Dap.DapService: Swapping PotentialSource with UUID
> /devices/pci0000:00/0000:00:1a.0/usb1/1-1/1-1.2 for MtpSource
> [19 Debug 22:43:00.051] Unmapping DAP source
> (/devices/pci0000:00/0000:00:1a.0/usb1/1-1/1-1.2)
> [19 Debug 22:43:00.631] DapService: mapping
> MtpSource-/devices/pci0000:00/0000:00:1a.0/usb1/1-1/1-1.2
> [1 Debug 22:43:00.828] Creating Pango.Layout, configuring Cairo.Context
> [1 Debug 22:43:00.869] Creating Pango.Layout, configuring Cairo.Context
> [1 Debug 22:43:00.924] Creating Pango.Layout, configuring Cairo.Context
> [1 Debug 22:43:00.983] Creating Pango.Layout, configuring Cairo.Context
> [1 Debug 22:43:01.307] Last.fm State Changed to NoAccount
> 
> 
> So I guess this is still needs-work, but feels closer to resolution.
> 
> BTW, some more things:
> 
> > -                    ThreadAssist.ProxyToMain (delegate {
> > -
> > -                        ServiceManager.SourceManager.AddSource (source);
> > -                        source.NotifyUser ();
> > -
> > -                        // If there are any queued device commands, see if they are to be
> > -                        // handled by this new DAP (e.g. --device-activate=file:///media/disk)
> > -                        try {
> > -                            if (service.unhandled_device_commands != null) {
> > -                                foreach (DeviceCommand command in service.unhandled_device_commands) {
> > -                                    if (source.CanHandleDeviceCommand (command)) {
> > -                                        service.HandleDeviceCommand (source, command.Action);
> > -                                        service.unhandled_device_commands.Remove (command);
> > -                                        if (service.unhandled_device_commands.Count == 0) {
> > -                                            service.unhandled_device_commands = null;
> > -                                        }
> > -                                        break;
> > -                                    }
> > -                                }
> > -                            }
> > -                        } catch (Exception e) {
> > -                            Log.Error (e);
> > -                        }
> > -                    });
> > +                    service.MapSource (source);
> 
> Given that the above is mainly moving code around, in order to have a clearer
> diff of what you're really changing, I've committed a pre-refactoring now:
> https://git.gnome.org/browse/banshee/commit/?id=e13c46f3e889090b955485f23b62f4e6a4e8c621
> 

Great, but I will have those branches, and that break, out of that foreach loop eventually ;)

> 
> > +        private object _lock_ = new { };
> 
> What kind of notation is that?? please use simply "lock_object" ;) and
> initialize via 'new object ();' not weird things.
> 

'le3t H4x0r Notation? It's gone.

> > +            Claimant = claim;
> 
> Shouldn't the parameter be named "claimant" too?

Yep, done.

> > The FindDeviceSource method in DapService handles three types of exception;
> > InvalidDevice, InvalidDeviceState and Exception. I was following that.
> 
> Then please separate the catch in three, send the exception to Log.Warn() for
> the first two, and send to Log.Err() for Exception.
> (Same for other parts of the code where you catch(Exception )

Also done, though I feel that InvalidDeviceException is actually an error at this point, since it means we mapped a potential source that now reports it can't handle the device it said it could earlier. If that happens, maybe we should try a full Unmap/Map cycle?

Maybe it's useful for you to see a reference log, I started banshee with the device plugged in. This is what I get, pretty much every time. Ignore the "source returned null from its iter" messages.

Notably Claim never takes longer than about 3 seconds. However, what I just noticed making this log (since I had LastFM enabled last time I tested my device) is that afer a new desktop session, the mount/unmount cycle is practically instant on my box. I'll try and confirm that on the ubuntu system, but IIRC I was surprised how quick that machine did the cycle too.

*** Running with gui-thread-check ***
[Info  14:55:02.782] Running Banshee 2.9.2: [git-checkout (linux-gnu, x86_64) @ 2014-07-23 11:40:41 BST]
[1 Debug 14:55:02.794] Initializing GTK
*** GUI THREAD INITIALIZED: 2065024896
[1 Debug 14:55:03.608] Post-Initializing GTK
[1 Debug 14:55:03.618] Configuration client extension loaded (Banshee.GnomeBackend.GConfConfigurationClient)
[1 Debug 14:55:03.620] Using default gconf-base-key
[1 Debug 14:55:03.706] Bus.Session.RequestName ('org.bansheeproject.Banshee') replied with PrimaryOwner
[1 Debug 14:55:03.751] Core service started (DBusServiceManager, 0.00104)
[1 Debug 14:55:03.753] Registering remote object /org/bansheeproject/Banshee/DBusCommandService (Banshee.ServiceStack.DBusCommandService) on org.bansheeproject.Banshee
[1 Debug 14:55:03.758] Core service started (DBusCommandService, 0.006817)
[1 Debug 14:55:03.776] Opened SQLite (version 3.8.2) connection to /home/nicholas/.config/banshee-1/banshee.db
[1 Debug 14:55:03.776] Core service started (DbConnection, 0.018165)
[1 Debug 14:55:03.780] Database version 45 is up to date
[1 Debug 14:55:03.800] Core service started (PreferenceService, 0.009)
[1 Debug 14:55:03.805] Core service started (Network, 0.004972)
[1 Debug 14:55:03.805] Registering remote object /org/bansheeproject/Banshee/SourceManager (Banshee.Sources.SourceManager) on org.bansheeproject.Banshee
[1 Debug 14:55:03.805] Core service started (SourceManager, 0.000607)
[1 Debug 14:55:03.809] Core service started (MediaProfileManager, 0.000183)
[1 Debug 14:55:03.811] Registering remote object /org/bansheeproject/Banshee/PlayerEngine (Banshee.MediaEngine.PlayerEngineService) on org.bansheeproject.Banshee
[1 Debug 14:55:03.812] Core service started (PlayerEngine, 0.002964)
[1 Debug 14:55:03.822] Registering remote object /org/bansheeproject/Banshee/PlaybackController (Banshee.PlaybackController.PlaybackControllerService) on org.bansheeproject.Banshee
[1 Debug 14:55:03.823] Core service started (PlaybackController, 0.001956)
[1 Debug 14:55:03.827] Starting - Startup Job
[1 Debug 14:55:03.827] Core service started (JobScheduler, 0.004804)
[1 Debug 14:55:03.835] IO provider extension loaded (Banshee.IO.Gio.Provider)
[1 Debug 14:55:03.859] Loaded HardwareManager backend: Banshee.Hardware.Gio
[1 Debug 14:55:03.861] Core service started (HardwareManager, 0.033299)
[1 Debug 14:55:03.862] Bus.Session.RequestName ('org.bansheeproject.CollectionIndexer') replied with PrimaryOwner
[1 Debug 14:55:03.862] Registering remote object /org/bansheeproject/Banshee/CollectionIndexerService (Banshee.Collection.Indexer.CollectionIndexerService) on org.bansheeproject.CollectionIndexer
[1 Debug 14:55:03.864] Core service started (CollectionIndexerService, 0.002639)
[1 Debug 14:55:03.865] Core service started (SaveTrackMetadataService, 0.001744)
[1 Debug 14:55:03.877] Adding icon theme search path: /home/nicholas/Source/Csharp/banshee/bin/share/banshee/icons
[1 Debug 14:55:03.877] Core service started (GtkElementsService, 0.011898)
[1 Debug 14:55:03.878] Core service started (InterfaceActionService, 0.0009)
[1 Debug 14:55:03.969] Extension actions loaded: MetadataFixActions
[1 Debug 14:55:03.969] Registering remote object /org/bansheeproject/Banshee/GlobalUIActions (Banshee.Gui.GlobalActions) on org.bansheeproject.Banshee
[1 Debug 14:55:03.970] Album artwork path set to /home/nicholas/.cache/media-art
[1 Debug 14:55:03.981] Core service started (ArtworkManager, 0.011409)
[1 Debug 14:55:03.981] Core service started (BookmarksService, 0.000118)
[1 Debug 14:55:04.272] Constructed Nereid interface: 0.261055
[1 Debug 14:55:04.325] Registering remote object /org/bansheeproject/Banshee/ClientWindow (Nereid.PlayerInterface) on org.bansheeproject.Banshee
[1 Debug 14:55:04.325] Core service started (NereidPlayerInterface, 0.336945)
[1 Info  14:55:04.328] Updating web proxy from GConf
[1 Debug 14:55:04.332] Direct connection, no proxy in use
[1 Debug 14:55:04.341] Extension service started (GnomeService, 0.015899)
[1 Debug 14:55:04.350] Notification daemon supports persistence, no status icon needed
[1 Debug 14:55:04.350] Extension service started (NotificationAreaService, 0.008463)
[1 Debug 14:55:04.354] Extension service started (MediaPanelService, 0.003327)
[1 Debug 14:55:04.354] Extension service started (DapService, 0.000639)
[1 Debug 14:55:04.356] Using GNOME 2.22 API for Multimedia Keys
[1 Debug 14:55:04.357] Extension service started (MultimediaKeysService, 0.0021)
[1 Debug 14:55:04.374] Extension service started (AudioCdService, 0.017366)
[1 Debug 14:55:04.375] Extension service started (DvdService, 0.001381)
[1 Debug 14:55:04.377] Extension service started (CoverArtService, 0.001468)
[1 Debug 14:55:04.392] Extension service started (GStreamerCoreService, 0.015059)
[1 Debug 14:55:04.395] Extension service started (PodcastService, 0.002205)
[1 Info  14:55:04.395] All services are started 0.688817
[1 Debug 14:55:04.818] Extension source loaded: Now Playing
[1 Debug 14:55:04.874] Registering remote object /org/bansheeproject/Banshee/SourceManager/PlayQueue (Banshee.PlayQueue.PlayQueueSource) on org.bansheeproject.Banshee
[1 Debug 14:55:04.874] Extension source loaded: Play Queue
[1 Debug 14:55:04.885] Extension source loaded: Radio
[1 Debug 14:55:04.914] Extension source loaded: File System Queue
[1 Debug 14:55:04.935] Extension source loaded: Audiobooks
[1 Debug 14:55:04.938] Starting GTK main loop
[1 Info  14:55:04.990] nereid Client Started
[1 Debug 14:55:04.991] Delayed Initializating Banshee.MediaEngine.PlayerEngineService
[1 Debug 14:55:05.002] (libbanshee:player) Audiosink has volume: YES
[1 Debug 14:55:05.003] (libbanshee:player) Using system (gst-plugins-good) equalizer element
[1 Debug 14:55:05.022] Player state change: NotReady -> Ready
[1 Debug 14:55:05.025] Loaded equalizer presets: 0.000349
[1 Debug 14:55:05.029] Selected equalizer: Rock
[1 Debug 14:55:05.033] Player state change: Ready -> Idle
[1 Debug 14:55:05.036] (libbanshee:player) Enabled ReplayGain
[1 Debug 14:55:05.039] (libbanshee:player) scaled volume: 1.00 (ReplayGain) * 0.00 (User) = 0.00
[1 Info  14:55:05.040] GStreamer version 1.2.3.0, gapless: True, replaygain: True
[1 Debug 14:55:05.041] Delayed Initializating Banshee.Dap.DapService
[1 Debug 14:55:05.046] Dap support extension loaded: Banshee.Dap.MassStorage
[1 Debug 14:55:05.047] Dap support extension loaded: Banshee.Dap.Mtp
[1 Debug 14:55:05.051] Delayed Initializating Banshee.Podcasting.PodcastService
[1 Error 14:55:05.085] SourceView of some source could not render its source column because its type value returned null from the iter
[1 Error 14:55:05.085] SourceView of source Podcasts could not render its source column because its type value returned null from the iter
[1 Error 14:55:05.085] SourceView of source Podcasts could not render its source column because its type value returned null from the iter
[8 Debug 14:55:05.094] Refreshing any podcasts that haven't been updated in over an hour
[1 Debug 14:55:05.152] Creating Pango.Layout, configuring Cairo.Context
[1 Debug 14:55:06.107] Finished - Startup Job
[1 Debug 14:55:06.109] Starting - Downloading Cover Art
[11 Debug 14:55:06.111] Finished - Downloading Cover Art
[1 Debug 14:55:06.817] Gio.Manager: received VolumeAdded signal
[1 Warn  14:55:06.819] Failed to load media-player-info file for 1
Device 0 (VID=04e8 and PID=6860) is a Samsung Galaxy models (MTP).
PTP_ERROR_IO: failed to open session, trying again after resetting USB interface
LIBMTP libusb: Attempt to reset device
[1 Debug 14:55:13.869] Starting - Saving Metadata to File
[13 Debug 14:55:13.875] Finished - Saving Metadata to File
[1 Debug 14:55:15.351] Creating Pango.Layout, configuring Cairo.Context
[12 Debug 14:55:16.544] Found DAP support (Banshee.Dap.Mtp.MtpSource) for device GT-I9100 and Uuid /devices/pci0000:00/0000:00:1a.0/usb1/1-1/1-1.1
[1 Error 14:55:16.592] SourceView of some source could not render its source column because its type value returned null from the iter
[1 Error 14:55:16.592] SourceView of source Devices could not render its source column because its type value returned null from the iter
[1 Error 14:55:16.592] SourceView of source Devices could not render its source column because its type value returned null from the iter
[1 Error 14:55:16.594] SourceView of some source could not render its source column because its type value returned null from the iter
[1 Error 14:55:16.594] SourceView of source GT-I9100 could not render its source column because its type value returned null from the iter
[1 Error 14:55:16.594] SourceView of source GT-I9100 could not render its source column because its type value returned null from the iter
[1 Debug 14:55:41.639] Creating Pango.Layout, configuring Cairo.Context

Disconnect button here, active source removed.

[15 Debug 14:55:45.072] DapService: unmap request from /devices/pci0000:00/0000:00:1a.0/usb1/1-1/1-1.1
[15 Debug 14:55:45.072] Unmapping DAP source (/devices/pci0000:00/0000:00:1a.0/usb1/1-1/1-1.1)
[1 Debug 14:55:45.133] Creating Pango.Layout, configuring Cairo.Context
[1 Debug 14:55:45.396] Creating Pango.Layout, configuring Cairo.Context
[1 Debug 14:55:45.397] Creating Pango.Layout, configuring Cairo.Context
[1 Debug 14:55:49.570] Creating Pango.Layout, configuring Cairo.Context
[1 Debug 14:55:49.572] Creating Pango.Layout, configuring Cairo.Context

Within a second of this message I mounted the device in nautilus.

[1 Debug 14:55:52.461] Gio.Manager: received MountAdded signal
[1 Error 14:55:52.461] Gio.Manager: ignoring VolumeChanged signal with no volume
[1 Debug 14:55:52.464] Gio.Manager: received MountAdded signal
[1 Warn  14:55:52.464] Failed to load media-player-info file for 1
Device 0 (VID=04e8 and PID=6860) is a Samsung Galaxy models (MTP).
[16 Warn  14:55:52.473] Dap.DapService: invalid state, mapping potential source for SAMSUNG_Android
[16 Debug 14:55:52.777] Found DAP support (Banshee.Dap.PotentialSource) for device SAMSUNG_Android and Uuid /devices/pci0000:00/0000:00:1a.0/usb1/1-1/1-1.1
[1 Error 14:55:52.803] SourceView of some source could not render its source column because its type value returned null from the iter
[1 Error 14:55:52.804] SourceView of source Devices could not render its source column because its type value returned null from the iter
[1 Error 14:55:52.804] SourceView of source Devices could not render its source column because its type value returned null from the iter
[1 Error 14:55:52.805] SourceView of some source could not render its source column because its type value returned null from the iter
[1 Error 14:55:52.805] SourceView of source SAMSUNG_Android could not render its source column because its type value returned null from the iter
[1 Error 14:55:52.805] SourceView of source SAMSUNG_Android could not render its source column because its type value returned null from the iter
[1 Debug 14:55:54.825] Creating Pango.Layout, configuring Cairo.Context
[1 Debug 14:55:54.826] Creating Pango.Layout, configuring Cairo.Context

Claim pressed.

[1 Debug 14:56:02.363] PotentialSource: TryClaim SAMSUNG_Android as Banshee.Dap.Mtp.MtpSource
[17 Debug 14:56:02.365] PotentialSource: Creating Instance
[17 Debug 14:56:02.366] PotentialSource: Initializing Device
Device 0 (VID=04e8 and PID=6860) is a Samsung Galaxy models (MTP).
[17 Debug 14:56:02.366] MtpSource: attempting to unmount SAMSUNG_Android
[1 Debug 14:56:02.380] Gio.Manager: received MountRemoved signal
[1 Error 14:56:02.380] Gio.Manager: ignoring VolumeChanged signal with no volume
[1 Debug 14:56:02.381] Gio.Manager: received MountRemoved signal
[1 Warn  14:56:02.389] Failed to load media-player-info file for 1
[1 Debug 14:56:02.390] PotentialSource: TryInitialize SAMSUNG_Android as Banshee.Dap.Mtp.MtpSource
[17 Debug 14:56:04.389] PotentialSource: Loading Contents
[17 Debug 14:56:04.389] PotentialSource: Success, new Source GT-I9100

14:56:02 -> 14:56:04 claim time.

[17 Debug 14:56:04.390] Dap.DapService: Swapping PotentialSource with UUID /devices/pci0000:00/0000:00:1a.0/usb1/1-1/1-1.1 for MtpSource
[17 Debug 14:56:04.390] Unmapping DAP source (/devices/pci0000:00/0000:00:1a.0/usb1/1-1/1-1.1)
[1 Error 14:56:04.430] SourceView of some source could not render its source column because its type value returned null from the iter
[1 Error 14:56:04.430] SourceView of source Devices could not render its source column because its type value returned null from the iter
[1 Error 14:56:04.430] SourceView of source Devices could not render its source column because its type value returned null from the iter
[1 Error 14:56:04.431] SourceView of some source could not render its source column because its type value returned null from the iter
[1 Error 14:56:04.431] SourceView of source GT-I9100 could not render its source column because its type value returned null from the iter
[1 Error 14:56:04.432] SourceView of source GT-I9100 could not render its source column because its type value returned null from the iter
[1 Debug 14:56:11.148] Service disposed (PodcastService)
[1 Debug 14:56:11.148] Service disposed (GStreamerCoreService)
[1 Debug 14:56:11.148] Service disposed (CoverArtService)
[1 Debug 14:56:11.149] Service disposed (DvdService)
[1 Debug 14:56:11.150] Service disposed (AudioCdService)
[1 Debug 14:56:11.152] Service disposed (MultimediaKeysService)
[1 Debug 14:56:11.153] Service disposed (DapService)
[1 Debug 14:56:11.154] Service disposed (MediaPanelService)
[1 Debug 14:56:11.154] Service disposed (NotificationAreaService)
[1 Debug 14:56:11.155] Service disposed (GnomeService)
[1 Debug 14:56:11.160] Service disposed (NereidPlayerInterface)
[1 Debug 14:56:11.160] Service disposed (BookmarksService)
[1 Debug 14:56:11.160] Service disposed (CollectionIndexerService)
[1 Debug 14:56:11.161] Service disposed (HardwareManager)
[1 Debug 14:56:11.162] (libbanshee:player) bp_stop: setting state to GST_STATE_NULL
[1 Debug 14:56:11.164] (libbanshee:player) bp_destroy: disposed player
[1 Debug 14:56:11.164] Service disposed (PlayerEngine)
[1 Debug 14:56:11.182] Service disposed (SourceManager)
[1 Debug 14:56:11.182] Service disposed (Network)
[1 Debug 14:56:11.183] Service disposed (DbConnection)
Comment 50 Nicholas Little 2014-07-31 14:32:15 UTC
Created attachment 282148 [details] [review]
Mtp CheckErrorStack on Battery Status Retrieval error
Comment 51 Andrés G. Aragoneses (IRC: knocte) 2014-08-01 22:10:15 UTC
Comment on attachment 282147 [details] [review]
Cleanup and slight rework V6

This has been the best patch so far. And even if I thought it wasn't perfect (more on that later), I committed it by mistake.

Instead of reverting it though, I think I'm going to leave it as is, and any new improvements we'll commit them in subsequent patches.
Comment 52 Andrés G. Aragoneses (IRC: knocte) 2014-08-01 22:12:58 UTC
Comment on attachment 282148 [details] [review]
Mtp CheckErrorStack on Battery Status Retrieval error

This patch hasn't seemed to have fixed anything. As I committed it by mistake as well, should I revert it?
Comment 53 Nicholas Little 2014-08-08 15:06:01 UTC
Created attachment 282925 [details] [review]
Publicise RequestUnmap

I noticed that the Ejected signal in BluetoothSource performs the same function as RequestUnmap, any chance of the latter becoming public?

It's not really a related change, but since this one's still open for tweaking I thought it appropriate to post the patch here.

Cheers!
Comment 54 André Klapper 2020-03-17 09:55:25 UTC
Banshee is not under active development anymore and had its last code changes more than three years ago. Its codebase has been archived.

Closing this report as WONTFIX as part of Bugzilla Housekeeping to reflect
reality. Please feel free to reopen this ticket (or rather transfer the project
to GNOME Gitlab, as GNOME Bugzilla is being shut down) if anyone takes the
responsibility for active development again.
See https://gitlab.gnome.org/Infrastructure/Infrastructure/issues/264 for more info.