Permalink
Browse files

fixed: URL encoded filenames didn't properly create destination filen…

…ames for copy/move/etc. in the filemanager
  • Loading branch information...
1 parent ba9ca19 commit 15e04f81f78aa535d2c0d59845d643761b3f8783 arnova committed Jun 21, 2011
Showing with 6 additions and 2 deletions.
  1. +6 −2 xbmc/utils/FileOperationJob.cpp
@@ -152,7 +152,11 @@ bool CFileOperationJob::DoProcess(FileAction action, CFileItemList & items, cons
CStdString strnewDestFile;
if(!strDestFile.IsEmpty()) // only do this if we have a destination
+ {
URIUtils::AddFileToFolder(strDestFile, strFileName, strnewDestFile);
+ // URL Decode for cases where source uses URL encoding and target does not
+ CURL::Decode(strnewDestFile);
+ }
@elupus
elupus Jun 21, 2011 Member

this is wrong. please revert.

If i have a file with the name

my%20filename.avi

stored on my windows fileshare disk. That should not be translated to "my filename.avi" on copy.

@elupus
elupus Jun 21, 2011 Member

Hmm.. or if this is just for display then it's okey, but I can't tell from diff.

@arnova
arnova Jun 21, 2011 Member

It's ALSO for display. Although I doubt the practical use of your case, especially considering what this fixes (copying from URL encoded VFS's), reverting maybe a little drastic. I think a far better fix is simply adding a check performing an IsInternetStream() on the source filename.

@jmarshallnz
jmarshallnz via email Jun 21, 2011 Member
@topfs2
topfs2 Jun 22, 2011 Member

I don't understand why the need for decode, the path we are doing something to is given by vfs (from directory and such) thus it should be understandable by the vfs.

To me it seems like a wrong fix for the problem, so revert seems to be in order.

@arnova
arnova Jun 22, 2011 Member

The reason we need to decode is when one has an eg. http source on the left in the filemanger and a local drive on the right files like "01 - track.mp3" will stored on the http source as "01%20-%20track.mp3" and literally(!) copied to the local drive. The fact that the target maybe an URL encoded VFS too, is no problem since this is handled by our VFS. Furthermore any of those VFS's are streaming and/or don't support file writing of any kind.

Of course this can be reverted but this obviously doesn't fix the issue nor does yelling "just revert". I'd prefer a more constructive solution.

@jmarshallnz
jmarshallnz via email Jun 22, 2011 Member
@elupus
elupus Jun 22, 2011 Member

that looks better. I just wonder how ftp should be handled here. Don't remember how it is constructed

@arnova
arnova Jun 23, 2011 Member

I think FTP can do both. I'll have a look at it. How about creating a function called eg. CURL::UsesURLEncoding(CStdstring &strFileName) to simply check the protocol against a list of protocols that use URL encoding instead of using IsInternetStream()? We can also use that for some of the other instances of URLDecode()?

if (pItem->m_bIsFolder)
{
@@ -225,14 +229,14 @@ bool CFileOperationJob::CFileOperation::ExecuteOperation(CFileOperationJob *base
case ActionCopy:
case ActionReplace:
{
- CLog::Log(LOGDEBUG,"FileManager: copy %s->%s\n", m_strFileA.c_str(), m_strFileB.c_str());
+ CLog::Log(LOGDEBUG,"FileManager: copy %s -> %s\n", m_strFileA.c_str(), m_strFileB.c_str());
bResult = CFile::Cache(m_strFileA, m_strFileB, this, &data);
}
break;
case ActionMove:
{
- CLog::Log(LOGDEBUG,"FileManager: move %s->%s\n", m_strFileA.c_str(), m_strFileB.c_str());
+ CLog::Log(LOGDEBUG,"FileManager: move %s -> %s\n", m_strFileA.c_str(), m_strFileB.c_str());
if (CanBeRenamed(m_strFileA, m_strFileB))
bResult = CFile::Rename(m_strFileA, m_strFileB);

5 comments on commit 15e04f8

@elupus
Member
elupus commented on 15e04f8 Jun 23, 2011

Very true

@elupus
Member
elupus commented on 15e04f8 Jun 23, 2011

Having GetFileName() return an urldecoded name for internet streams seem valid..

@arnova
Member
arnova commented on 15e04f8 Jun 23, 2011

@cptspiff: Yeah this would be the best future solution although this can't be easily/quickly be implemented.
@elupus: Do you mean we should move the URLDecode() call + URL check to URIUtils::GetFileName() instead to make it more generic?

@elupus
Member
elupus commented on 15e04f8 Jun 23, 2011

Doesn't URIUtils make use of CURL to get the filename part?

@arnova
Member
arnova commented on 15e04f8 Jun 24, 2011

I think the reason why we don't have ::Parse() URLDecode is that it could cause problems when manipulating the CURL filename member and then re-using the CURL object. It would be better then to put this in URIUtils::GetFileName(), I guess.

Since I didn't think such a minor fix would introduce this much fuss, I'll revert the whole thing. But since we've been talking about moving to CURL's for all our filenames for ages now and we haven't done this yet I doubt it will happen in the near future including a fix for this issue.

Please sign in to comment.