Made tag reading safer and easier.

Show error message if we couldn't write tags to a file.

src/libtomahawk/filemetadata/MetadataEditor.cpp

@@ -42,6 +42,7 @@
 #include <QDialogButtonBox>
 #include <QFileInfo>
 #include <QFile>
+#include <QMessageBox>
 
 
 MetadataEditor::MetadataEditor( const Tomahawk::query_ptr& query, const Tomahawk::playlistinterface_ptr& plInterface, QWidget* parent )
@@ -85,9 +86,12 @@ MetadataEditor::init( const Tomahawk::playlistinterface_ptr& plInterface )
 void
 MetadataEditor::writeMetadata( bool closeDlg )
 {
+    bool failed = false;
+    QFileInfo fi;
+
     if ( m_result )
     {
-        QFileInfo fi( QUrl( m_result->url() ).toLocalFile() );
+        fi = QFileInfo( QUrl( m_result->url() ).toLocalFile() );
 
         bool changed = false;
         QByteArray fileName = QFile::encodeName( fi.canonicalFilePath() );
@@ -99,77 +103,88 @@ MetadataEditor::writeMetadata( bool closeDlg )
 
         TagLib::FileRef f( encodedName );
         QSharedPointer<Tomahawk::Tag> tag( Tomahawk::Tag::fromFile( f ) );
-
+        if ( !tag )
-        if ( title() != m_result->track()->track() )
         {
-            tDebug() << Q_FUNC_INFO << "Track changed" << title() << m_result->track();
+            failed = true;
-
-            tag->setTitle( title() );
-            m_result->track()->setTrack( title() );
-
-            changed = true;
         }
-
+        else
-        Tomahawk::artist_ptr newArtist = Tomahawk::Artist::get( artist(), true );
-        if ( newArtist != m_result->track()->artistPtr() )
         {
-            tDebug() << Q_FUNC_INFO << "Artist changed" << artist() << m_result->track()->artist();
+            if ( title() != m_result->track()->track() )
-
-            tag->setArtist( artist() );
-            m_result->track()->setArtist( artist() );
-
-            changed = true;
-        }
-
-        Tomahawk::album_ptr newAlbum = Tomahawk::Album::get( newArtist, album(), true );
-        if ( newAlbum != m_result->track()->albumPtr() )
-        {
-            tDebug() << Q_FUNC_INFO << "Album changed" << album() << newAlbum->id() << m_result->track()->album() << m_result->track()->albumPtr()->id() << newAlbum.data() << m_result->track()->albumPtr().data();
-            if ( newAlbum->id() != m_result->track()->albumPtr()->id() )
             {
-                tag->setAlbum( album() );
+                tDebug() << Q_FUNC_INFO << "Track changed" << title() << m_result->track();
-                m_result->track()->setAlbum( album() );
 
+                tag->setTitle( title() );
                 changed = true;
             }
-        }
 
-        // FIXME: Ugly workaround for the min value of 0
+            Tomahawk::artist_ptr newArtist = Tomahawk::Artist::get( artist(), true );
-        if ( albumPos() != 0 && albumPos() != (int)m_result->track()->albumpos() )
+            if ( newArtist != m_result->track()->artistPtr() )
-        {
-            tag->setTrack( albumPos() );
-            m_result->track()->setAlbumPos( albumPos() );
-
-            tDebug() << Q_FUNC_INFO << "Albumpos changed";
-            changed = true;
-        }
-
-        // FIXME: Ugly workaround for the min value of 1900
-        if ( year() != 1900 && year() != m_result->track()->year() )
-        {
-            tag->setYear( year() );
             {
-                QVariantMap attr = m_result->track()->attributes();
+                tDebug() << Q_FUNC_INFO << "Artist changed" << artist() << m_result->track()->artist();
-                attr[ "releaseyear" ] = year();
+
-                m_result->track()->setAttributes( attr );
+                tag->setArtist( artist() );
+                changed = true;
             }
 
-            tDebug() << Q_FUNC_INFO << "Year changed";
+            Tomahawk::album_ptr newAlbum = Tomahawk::Album::get( newArtist, album(), true );
-            changed = true;
+            if ( newAlbum != m_result->track()->albumPtr() )
-        }
+            {
+                tDebug() << Q_FUNC_INFO << "Album changed" << album() << newAlbum->id() << m_result->track()->album() << m_result->track()->albumPtr()->id() << newAlbum.data() << m_result->track()->albumPtr().data();
+                if ( newAlbum->id() != m_result->track()->albumPtr()->id() )
+                {
+                    tag->setAlbum( album() );
+                    changed = true;
+                }
+            }
 
-        if ( changed )
+            // FIXME: Ugly workaround for the min value of 0
-        {
+            if ( albumPos() != 0 && albumPos() != (int)m_result->track()->albumpos() )
-            f.save();
+            {
+                tag->setTrack( albumPos() );
 
-            m_editFiles.append( fileName );
+                tDebug() << Q_FUNC_INFO << "Albumpos changed";
-            m_result->doneEditing();
+                changed = true;
+            }
 
-            tDebug() << Q_FUNC_INFO << m_result->toString() << m_result->track()->toString();
+            // FIXME: Ugly workaround for the min value of 1900
+            if ( year() != 1900 && year() != m_result->track()->year() )
+            {
+                tag->setYear( year() );
+
+                tDebug() << Q_FUNC_INFO << "Year changed";
+                changed = true;
+            }
+
+            if ( changed )
+            {
+                if ( f.save() )
+                {
+                    m_result->track()->setTrack( title() );
+                    m_result->track()->setArtist( artist() );
+                    m_result->track()->setAlbum( album() );
+                    m_result->track()->setAlbumPos( albumPos() );
+                    {
+                        QVariantMap attr = m_result->track()->attributes();
+                        attr[ "releaseyear" ] = year();
+                        m_result->track()->setAttributes( attr );
+                    }
+
+                    m_editFiles.append( fileName );
+                    m_result->doneEditing();
+
+                    tDebug() << Q_FUNC_INFO << m_result->toString() << m_result->track()->toString();
+                }
+                else
+                    failed = true;
+            }
         }
     }
 
-    if ( closeDlg )
+    if ( failed )
+    {
+        QMessageBox::critical( 0, tr( "Error" ), tr( "Could not write tags to file:\n%1" ).arg( fi.canonicalFilePath() ) );
+    }
+    else if ( closeDlg )
     {
         if ( m_editFiles.count() )
             ScanManager::instance()->runFileScan( m_editFiles, false );

src/libtomahawk/filemetadata/MusicScanner.cpp

@@ -408,6 +408,9 @@ MusicScanner::readTags( const QFileInfo& fi )
     int bitrate = 0;
     int duration = 0;
     QSharedPointer<Tag> tag( Tag::fromFile( f ) );
+    if ( !tag )
+        return QVariantMap();
+
     if ( f.audioProperties() )
     {
         TagLib::AudioProperties *properties = f.audioProperties();
@@ -415,18 +418,14 @@ MusicScanner::readTags( const QFileInfo& fi )
         bitrate = properties->bitrate();
     }
 
-    QString artist, album, track;
+    const QString artist = tag->artist().trimmed();
-    if ( tag )
+    const QString album  = tag->album().trimmed();
-    {
+    const QString track  = tag->title().trimmed();
-        artist = tag->artist().trimmed();
+    if ( artist.isEmpty() || track.isEmpty() )
-        album  = tag->album().trimmed();
-        track  = tag->title().trimmed();
-    }
-    if ( !tag || artist.isEmpty() || track.isEmpty() )
         return QVariantMap();
 
-    QString mimetype = TomahawkUtils::extensionToMimetype( suffix );
+    const QString mimetype = TomahawkUtils::extensionToMimetype( suffix );
-    QString url( "file://%1" );
+    const QString url( "file://%1" );
 
     QVariantMap m;
     m["url"]          = url.arg( fi.canonicalFilePath() );