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

Matěj Laitl matej at laitl.cz
Fri Jun 22 14:47:59 UTC 2012


On 22. 6. 2012 Sven Krohlas wrote:
> Matěj Laitl wrote:
> > 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.
> 
> I tried the following, but
> 
> +#else
> +    QVERIFY( QFile::exists( Amarok::saveLocation( "playlists" ) +
> "amarok\\playlist.xspf" ) );

I've backtraced it and the reason is calling  Amarok::vfatPath( filename ) in 
PlaylistFileProvider::save(), which replaces \ with _ on Unix. I don't think 
it is needed, because the directory is hard-coded to Amarok::saveLocation( 
"playlists" ) and I cannot imagine it can be vfat on Unix box.

> fails. Maybe we should just filter both '/' and '\'?

Maybe yes, perhaps we're putting too much effort to supporting such obscure 
thing as \ in Unix filenames. Either remove vfatPath() call or hard-code it to 
/ and \, I'll leave it to you.

	Matěj


More information about the Amarok-devel mailing list