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

[Sven Krohlas]

Hey,

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" ) );

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


diff --git a/src/playlistmanager/file/PlaylistFileProvider.cpp
b/src/playlistmanager/file/PlaylistFileProvider.cpp
index f364984..eb6d3e2 100644
--- a/src/playlistmanager/file/PlaylistFileProvider.cpp
+++ b/src/playlistmanager/file/PlaylistFileProvider.cpp
@@ -129,6 +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('-') );

     QString ext = Amarok::extension( filename );
diff --git a/src/playlistmanager/file/PlaylistFileProvider.h
b/src/playlistmanager/file/PlaylistFileProvider.h
index 7ff31bd..c8dbe58 100644
--- a/src/playlistmanager/file/PlaylistFileProvider.h
+++ b/src/playlistmanager/file/PlaylistFileProvider.h
@@ -54,7 +54,8 @@ class PlaylistFileProvider : public Playlists::UserPlaylistProvider
         * Returns a Playlists::PlaylistPtr to the new playlist, NULL if something failed.
         * @param tracks Tracks being added to that new playlist.
         * @param name File name of the new playlist. If no extension is being given we
-        *             default to xspf.
+        *             default to xspf. '/' and the directory separator of the system are
+        *             being replaced by '-'.
         */
         virtual Playlists::PlaylistPtr save( const Meta::TrackList &tracks,
                                              const QString &name = QString() );
diff --git a/tests/playlistmanager/file/TestPlaylistFileProvider.cpp
b/tests/playlistmanager/file/TestPlaylistFileProvider.cpp
index 5096ca2..50c53b8 100644
--- a/tests/playlistmanager/file/TestPlaylistFileProvider.cpp
+++ b/tests/playlistmanager/file/TestPlaylistFileProvider.cpp
@@ -74,6 +74,30 @@ void TestPlaylistFileProvider::testSave()
     QCOMPARE( testPlaylist->tracks().size(), 1 );
     QFile::remove( Amarok::saveLocation( "playlists" ) + "Amarok Test Playlist.xspf" );

+    // directory separators and '/' in file name are being repalced by '-'
+    testPlaylist = m_testPlaylistFileProvider->save( tempTrackList, "amarok/playlist" );
+    QVERIFY( testPlaylist );
+
+    QVERIFY( QFile::exists( Amarok::saveLocation( "playlists" ) + "amarok-playlist.xspf" ) );
+    QCOMPARE( testPlaylist->name(), QString( "amarok-playlist.xspf" ) );
+    QCOMPARE( testPlaylist->tracks().size(), 1 );
+    QFile::remove( Amarok::saveLocation( "playlists" ) + "amarok-playlist.xspf" );
+
+    testPlaylist = m_testPlaylistFileProvider->save( tempTrackList, "amarok\\playlist" );
+    QVERIFY( testPlaylist );
+
+#ifdef Q_WS_WIN
+    QVERIFY( QFile::exists( Amarok::saveLocation( "playlists" ) + "amarok-playlist.xspf" ) );
+    QCOMPARE( testPlaylist->name(), QString( "amarok-playlist.xspf" ) );
+    QCOMPARE( testPlaylist->tracks().size(), 1 );
+    QFile::remove( Amarok::saveLocation( "playlists" ) + "amarok-playlist.xspf" );
+#else
+    QVERIFY( QFile::exists( Amarok::saveLocation( "playlists" ) + "amarok\\playlist.xspf" ) );
+    QCOMPARE( testPlaylist->name(), QString( "amarok\\playlist.xspf" ) );
+    QCOMPARE( testPlaylist->tracks().size(), 1 );
+    QFile::remove( Amarok::saveLocation( "playlists" ) + "amarok\\playlist.xspf" );
+#endif
+
     // xspf
     testPlaylist = m_testPlaylistFileProvider->save( tempTrackList, "Amarok Test Playlist.xspf" );
     QVERIFY( testPlaylist );





-- 
Darkerradio Free Music Charts Januar 2012:
http://www.darkerradio.com/news/free-music-charts-januar-2012/
Klarmachen zum Ändern! -> http://www.piratenpartei.de/