[amarok] src/playlistmanager/file: Don't hardcode / as directory separator

Matěj Laitl matej at laitl.cz
Wed Jun 20 14:00:23 UTC 2012


On 20. 6. 2012 Sven Krohlas wrote:
> Git commit 04dd8cfb4c2767f9c9c4b2f0b087c16b2a2421de by Sven Krohlas.
> 
> Don't hardcode / as directory separator
> 
> Might have caused problems on Windows. I don't know, I don't have a Windows
> machine to test. While we are at it:
>
> diff --git a/src/playlistmanager/file/PlaylistFileProvider.cpp
> b/src/playlistmanager/file/PlaylistFileProvider.cpp index 9510268..f364984
> 100644
> --- a/src/playlistmanager/file/PlaylistFileProvider.cpp
> +++ b/src/playlistmanager/file/PlaylistFileProvider.cpp
> @@ -129,7 +129,7 @@ PlaylistFileProvider::save( const Meta::TrackList
> &tracks, const QString &name ) {
>      DEBUG_BLOCK
>      QString filename = name.isEmpty() ?
> QDateTime::currentDateTime().toString( "ddd MMMM d yy hh-mm") : name; -   
> filename.replace( QLatin1Char('/'), QLatin1Char('-') );
> +    filename.replace( QDir::separator(), QLatin1Char('-') );

Sven, your fix is actually incorrect. From QDir::separator() documentation:
> Returns the native directory separator: "/" under Unix (including Mac OS X) 
> and "\" under Windows. You do not need to use this function to build file
> paths. If you always use "/", Qt will translate your paths to conform to the
> underlying operating system.

Imagine a Windows user tries to save a playlist called "play/list". As only \ 
would be filtered, the path passes and then Qt translates the / to native 
Windows separator. To be really safe, I would filter-out both "/" and 
QDir::separator(), no-op on Unix and safe on Windoze. As a bonus, it would be 
very nice if this was tested in TestPlaylistFileProvider. ;)

Kudos to you, Sven, for cleaning up our tests, this is much needed.

	Matěj


More information about the Amarok-devel mailing list