[MTP] Unable to rename a file / folder

RESOLVED FIXED in 2.1 S1 (1aug)

Status

Firefox OS
MTP/UMS
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: ericchou, Assigned: ericchou)

Tracking

unspecified
2.1 S1 (1aug)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(feature-b2g:2.1)

Details

(Whiteboard: [ft:devices])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
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.
(Assignee)

Updated

4 years ago
Assignee: nobody → echou
Blocks: 922927
Depends on: 1029533
(Assignee)

Updated

4 years ago
Summary: Unable to rename a file / folder → [MTP] Unable to rename a file / folder
(Assignee)

Comment 1

4 years ago
ok, I've already had a patch. Will request for review tomorrow. :)
(Assignee)

Comment 2

4 years ago
Created attachment 8456769 [details] [diff] [review]
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.
Attachment #8456769 - Flags: review?(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'"
Attachment #8456769 - Flags: review?(dhylands) → review+
(Assignee)

Comment 4

4 years ago
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.
(Assignee)

Comment 6

4 years ago
Created attachment 8458579 [details] [diff] [review]
patch 1: final: Rename file/folder via MTP, r=dhylands

* nits picked.
(Assignee)

Updated

4 years ago
Attachment #8456769 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
feature-b2g: --- → 2.1
https://hg.mozilla.org/mozilla-central/rev/80464b4faafe
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S1 (1aug)

Updated

4 years ago
Whiteboard: [ft:devices]
(Assignee)

Updated

4 years ago
Component: General → MTP/UMS
You need to log in before you can comment on or make changes to this bug.