1036863 - [MTP] Unable to rename a file / folder

Status: RESOLVED FIXED

(Firefox OS Graveyard :: MTP/UMS, defect)

Description

[Eric Chou [:ericchou] [:echou]]

Host: Ubuntu 13.04

When I tried to rename a file or a folder, an error message

   "libmtp error:  could not get property description."

showed.

Comment 1

[Eric Chou [:ericchou] [:echou]]

ok, I've already had a patch. Will request for review tomorrow. :)

Comment 2

[Eric Chou [:ericchou] [:echou]]

Attachment: patch 1: v1: Rename file/folder via MTP

This patch fixed 'unable to rename file/folder via MTP' by responding to MTP operation 'setObjectPropertyValue' correctly.

Comment 3

[Dave Hylands [:dhylands]]

Comment on attachment 8456769 [details] [diff] [review]
patch 1: v1: Rename file/folder via MTP

Review of attachment 8456769 [details] [diff] [review]:
-----------------------------------------------------------------

All of my comments are pretty minor, so I'm marking that as r+ and leave it to your disgression how much of this you address

::: dom/system/gonk/MozMtpDatabase.cpp
@@ +109,5 @@
> +  nsCString path(aFullPath);
> +
> +  int32_t offset = aFullPath.RFindChar('/');
> +  if (offset != kNotFound) {
> +    path = StringHead(aFullPath, offset + 1);

Just to clarify - this seems to be returning the trailing slash as part of the path. IF that's intentional then we should probably mention it in a comment.

@@ +110,5 @@
> +
> +  int32_t offset = aFullPath.RFindChar('/');
> +  if (offset != kNotFound) {
> +    path = StringHead(aFullPath, offset + 1);
> +  }

If you call GetPathWithoutFileName('foo') shouldn't it return an empty string?

@@ +112,5 @@
> +  if (offset != kNotFound) {
> +    path = StringHead(aFullPath, offset + 1);
> +  }
> +
> +  MTP_LOG("path: %s", path.get());

nit: Since MTP_LOG will print the name of the function, perhaps change this to read "returning '%s'"

I like to put strings in quotes to makes it a bit more obvious that an empty string is being printed.

@@ +397,5 @@
> +    {MTP_PROPERTY_GENRE,             MTP_TYPE_STR     },
> +    {MTP_PROPERTY_COMPOSER,          MTP_TYPE_STR     },
> +    {MTP_PROPERTY_DURATION,          MTP_TYPE_UINT32  },
> +    {MTP_PROPERTY_DESCRIPTION,       MTP_TYPE_STR     },
> +  };

I think that this table should be global, and should only contain the properties that we actually support.

Then getSupportedObjectProperties, getObjectPopertyList and getObjectPropertyDesc can use the same table.

I feel like we need to turn this around and create our own property class, and then create an instance of this for each property.

The instance would know its own property id and type, and how to get/set the information from the DbEntry. Does that make sense? I'd be happy to see that in a followup bug as well.

Then when we add support for a new property id, we add a new entry and all of the rest of the code automagically works.

@@ +427,5 @@
> +    return  MTP_RESPONSE_OBJECT_PROP_NOT_SUPPORTED;
> +  }
> +
> +  if (GetTypeOfObjectProp(aProperty) != MTP_TYPE_STR) {
> +    return MTP_RESPONSE_GENERAL_ERROR;

I haven't done this consistently through the rest of the code, but I think that we should have a LOG at every return, especially the errors.

Otherwise, when we're trying to debug a log we won't be able to tell why a function failed.

Eventually, we'll have some runtime control on logging, and we can tweak whether we use MTP_LOG or MTP_ERR.

@@ +439,5 @@
> +
> +  MtpStringBuffer buf;
> +  aPacket.getString(buf);
> +
> +  nsDependentCString newFileName((const char *)buf);

I would have thought that this would work without the cast. Otherwise, I'd prefer to break this into 2 lines and do:

const char *bufp = buf;
nsDependentCString newFileName(bufp);

@@ +442,5 @@
> +
> +  nsDependentCString newFileName((const char *)buf);
> +  nsCString newFileFullPath(GetPathWithoutFileName(entry->mPath) + newFileName);
> +
> +  MTP_LOG("New location (full path): %s", newFileFullPath.get());

super nit: add single quotes around %s and lets move this after the PR_Rename and change it to be:

"Renamed '%s' to '%s'"

@@ +445,5 @@
> +
> +  MTP_LOG("New location (full path): %s", newFileFullPath.get());
> +
> +  if (PR_Rename(entry->mPath.get(), newFileFullPath.get()) != PR_SUCCESS) {
> +    MTP_ERR("Failed to rename");

I would like to see the from and to names on the failure message.

Something like "Failed to rename '%s' to '%s'"

Comment 4

[Eric Chou [:ericchou] [:echou]]

Hi Dave,

Thank for prompt review! I've addressed all problems you mentioned except the one below.

> 
> @@ +397,5 @@
> > +    {MTP_PROPERTY_GENRE,             MTP_TYPE_STR     },
> > +    {MTP_PROPERTY_COMPOSER,          MTP_TYPE_STR     },
> > +    {MTP_PROPERTY_DURATION,          MTP_TYPE_UINT32  },
> > +    {MTP_PROPERTY_DESCRIPTION,       MTP_TYPE_STR     },
> > +  };
> 
> I think that this table should be global, and should only contain the
> properties that we actually support.
> 
> Then getSupportedObjectProperties, getObjectPopertyList and
> getObjectPropertyDesc can use the same table.
> 
> I feel like we need to turn this around and create our own property class,
> and then create an instance of this for each property.
> 
> The instance would know its own property id and type, and how to get/set the
> information from the DbEntry. Does that make sense? I'd be happy to see that
> in a followup bug as well.
> 
> Then when we add support for a new property id, we add a new entry and all
> of the rest of the code automagically works.
> 

Yes, that makes sense to me. In v2 patch, I have removed unsupported properties from the table but did not create a structure for it. I'll do it in the followup.

Comment 5

[Eric Chou [:ericchou] [:echou]]

try: https://tbpl.mozilla.org/?tree=Try&rev=cde5b74b5d13

Comment 6

[Eric Chou [:ericchou] [:echou]]

Attachment: patch 1: final: Rename file/folder via MTP, r=dhylands

* nits picked.

Comment 7

[Eric Chou [:ericchou] [:echou]]

b2g-inbound: https://hg.mozilla.org/integration/b2g-inbound/rev/80464b4faafe

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/80464b4faafe