Bug 139757 - [PATCH] EBN#10 - Check for assignments to QString::null...
Summary: [PATCH] EBN#10 - Check for assignments to QString::null...
Status: RESOLVED FIXED
Alias: None
Product: amarok
Classification: Applications
Component: general (show other bugs)
Version: unspecified
Platform: Ubuntu Linux
: NOR wishlist
Target Milestone: ---
Assignee: Amarok Developers
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-01-08 07:50 UTC by Andrew Ash
Modified: 2007-01-08 22:06 UTC (History)
0 users

See Also:
Latest Commit:
Version Fixed In:


Attachments
Mentioned Patch (14.41 KB, patch)
2007-01-08 07:51 UTC, Andrew Ash
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Ash 2007-01-08 07:50:32 UTC
Version:           svn (using KDE KDE 3.5.5)
Installed from:    Ubuntu Packages

I'm a new contributor to Amarok and have a patch I'd like to submit.

I was looking on EBN ( http://www.englishbreakfastnetwork.org/krazy/reports/extragear/multimedia/amarok/index.html ) and picked one of the sections I thought I could handle, #10. -- "" Do not assign QString::null to a QString. Instead use the .clear() method. For example, "str = QString::null" becomes "str.clear()". When returning an empty string from a method use "return QString()". ""

So I went through and figured out a little of working with svn, patching process, etc, and now have a patch I'd like to be committed.  I'm not sure the proper procedure, so I will have sent a link to the mailing list too.  Let me know what's appropriate in the future.

Not all of the changes worked correctly on compiling, so I've only kept the ones that didn't prevent compilation.  I'm currently using this modified Amarok and don't see any regressions or anything.  I'm attaching the actual patch shortly.

Thanks for looking at this.
Comment 1 Andrew Ash 2007-01-08 07:51:45 UTC
Created attachment 19181 [details]
Mentioned Patch

against svn 621036
Comment 2 Alexandre Oliveira 2007-01-08 21:50:24 UTC
SVN commit 621458 by aoliveira:

EBN fixes related to QString::null usage.
Patch by Andrew Ash <ash211@gmail.com>. Thanks!
BUG: 139757


 M  +1 -1      actionclasses.cpp  
 M  +2 -2      amarokcore/amarokdcophandler.cpp  
 M  +1 -1      app.cpp  
 M  +3 -3      atomicstring.cpp  
 M  +3 -3      collectionbrowser.cpp  
 M  +1 -1      cuefile.cpp  
 M  +2 -2      database_refactor/collectiondb.cpp  
 M  +1 -1      engine/gst10/streamprovider.cpp  
 M  +1 -1      htmlview.cpp  
 M  +4 -4      ktrm.cpp  
 M  +1 -1      lastfm.cpp  
 M  +1 -1      mediadevice/generic/genericmediadevice.cpp  
 M  +1 -1      mediadevice/ifp/ifpmediadevice.cpp  
 M  +6 -6      metabundle.cpp  
 M  +2 -2      moodbar.cpp  
 M  +1 -1      playlistbrowser.cpp  
 M  +3 -3      scriptmanager.cpp  
 M  +1 -1      smartplaylisteditor.cpp  
 M  +1 -1      statistics.cpp  
 M  +8 -8      tagguesser.cpp  
 M  +1 -1      tracktooltip.cpp  


--- trunk/extragear/multimedia/amarok/src/actionclasses.cpp #621457:621458
@@ -408,7 +408,7 @@
 {
     if( m_icons.count() )
         return *m_icons.at( currentItem() );
-    return QString::null;
+    return QString();
 }
 
 QString SelectAction::currentText() const {
--- trunk/extragear/multimedia/amarok/src/amarokcore/amarokdcophandler.cpp #621457:621458
@@ -683,7 +683,7 @@
     {
         if( Playlist::instance()->currentItem() )
             return Playlist::instance()->currentItem()->uniqueId();
-        return QString::null;
+        return QString();
     }
 
 /////////////////////////////////////////////////////////////////////////////////////
@@ -936,7 +936,7 @@
         if (configItem)
             return configItem->property().toString();
         else
-            return QString::null;
+            return QString();
     }
 
     QStringList DcopScriptHandler::readListConfig(const QString& key)
--- trunk/extragear/multimedia/amarok/src/app.cpp #621457:621458
@@ -261,7 +261,7 @@
             if (!reply.isValid() || properties.count() < 6)
             {
                 debug() << "Invalid reply from mediamanager" << endl;
-                return QString::null;
+                return QString();
             }
             else
             {
--- trunk/extragear/multimedia/amarok/src/atomicstring.cpp #621457:621458
@@ -135,7 +135,7 @@
 
 QString AtomicString::string() const
 {
-    if ( !m_string ) return QString::null;
+    if ( !m_string ) return QString();
     // References to the stored string are only allowed to circulate in the main thread
     if ( isMainThread() ) return *m_string;
     else return deepCopy();
@@ -145,7 +145,7 @@
 {
     if (m_string)
 	return QString( m_string->unicode(), m_string->length() );
-    return QString::null;
+    return QString();
 }
 
 bool AtomicString::isEmpty() const
@@ -167,7 +167,7 @@
 	uint rc = m_string->refcount;
 	s_storeMutex.unlock();
 	return rc;
-    } 
+    }
     return 0;
 }
 
--- trunk/extragear/multimedia/amarok/src/collectionbrowser.cpp #621457:621458
@@ -2102,7 +2102,7 @@
     if ( item == 0 )
     {
         warning() << "getTrueItemText() called for empty CollectionItem" << endl;
-        return QString::null;
+        return QString();
     }
     if ( dynamic_cast<CollectionItem*>( item ) )
     {
@@ -2698,7 +2698,7 @@
             break;
     }
 
-    return QString::null;
+    return QString();
 }
 
 
@@ -2850,7 +2850,7 @@
             break;
     }
 
-    return QString::null;
+    return QString();
 }
 
 // This slot is called when the "browse right" action is activated,
--- trunk/extragear/multimedia/amarok/src/cuefile.cpp #621457:621458
@@ -131,7 +131,7 @@
                         // add previous entry to map
                         insert( index, CueFileItem( title, artist, defaultAlbum, track, index ) );
                         prevIndex = index;
-                        title  = QString::null;
+                        title = QString::null;
                         artist = QString::null;
                         track  = 0;
                     }
--- trunk/extragear/multimedia/amarok/src/database_refactor/collectiondb.cpp #621457:621458
@@ -768,7 +768,7 @@
 
                 // ignore APIC frames without picture and those with obviously bogus size
                 if( imgVector.size() == 0 || imgVector.size() > 10000000 /*10MB*/ )
-                    return QString::null;
+                    return QString();
 
                 QImage image;
                 if( image.loadFromData((const uchar*)imgVector.data(), imgVector.size()) )
@@ -787,7 +787,7 @@
         } // tag is empty
     } // caching
 
-    return QString::null;
+    return QString();
 }
 
 
--- trunk/extragear/multimedia/amarok/src/engine/gst10/streamprovider.cpp #621457:621458
@@ -314,7 +314,7 @@
     int index = str.find( key, 0, true );
 
     if ( index == -1 )
-        return QString::null;
+        return QString();
 
     else {
 
--- trunk/extragear/multimedia/amarok/src/htmlview.cpp #621457:621458
@@ -106,7 +106,7 @@
         const QString CSSLocation = kapp->dirs()->findResource( "data","amarok/themes/" + themeName + "/stylesheet.css" );
         QFile ExternalCSS( CSSLocation );
         if ( !ExternalCSS.open( IO_ReadOnly ) )
-            return QString::null; //FIXME: should actually return the default style sheet, then
+            return QString(); //FIXME: should actually return the default style sheet, then
 
         const QString pxSize = QString::number( ContextBrowser::instance()->fontMetrics().height() - 4 );
         const QString fontFamily = AmarokConfig::useCustomFonts() ?
--- trunk/extragear/multimedia/amarok/src/ktrm.cpp #621457:621458
@@ -422,7 +422,7 @@
 #if HAVE_TUNEPIMP
     return d->title;
 #else
-    return QString::null;
+    return QString();
 #endif
 }
 
@@ -431,7 +431,7 @@
 #if HAVE_TUNEPIMP
     return d->artist;
 #else
-    return QString::null;
+    return QString();
 #endif
 }
 
@@ -440,7 +440,7 @@
 #if HAVE_TUNEPIMP
     return d->album;
 #else
-    return QString::null;
+    return QString();
 #endif
 }
 
@@ -560,7 +560,7 @@
 #if HAVE_TUNEPIMP
     return d->file;
 #else
-    return QString::null;
+    return QString();
 #endif
 }
 
--- trunk/extragear/multimedia/amarok/src/lastfm.cpp #621457:621458
@@ -274,7 +274,7 @@
     if( url.isEmpty() && instance() && instance()->isPlaying() )
         url = instance()->getService()->currentStation();
 
-    if( url.isEmpty() ) return QString::null;
+    if( url.isEmpty() ) return QString();
 
     QStringList elements = QStringList::split( "/", url );
 
--- trunk/extragear/multimedia/amarok/src/mediadevice/generic/genericmediadevice.cpp #621457:621458
@@ -322,7 +322,7 @@
     m_ignoreThePrefix     = false;
     m_asciiTextOnly       = false;
 
-    m_songLocation    = QString::null;
+    m_songLocation = QString::null;
     m_podcastLocation = QString::null;
 
     m_supportedFileTypes.clear();
--- trunk/extragear/multimedia/amarok/src/mediadevice/ifp/ifpmediadevice.cpp #621457:621458
@@ -622,7 +622,7 @@
 QString
 IfpMediaDevice::getFullPath( const QListViewItem *item, const bool getFilename )
 {
-    if( !item ) return QString::null;
+    if( !item ) return QString();
 
     QString path;
 
--- trunk/extragear/multimedia/amarok/src/metabundle.cpp #621457:621458
@@ -788,11 +788,11 @@
         case PlayCount:  return QString::number( playCount( ensureCached ) );
         case LastPlayed: return QString::number( lastPlay( ensureCached ) );
         case Filesize:   return QString::number( filesize() );
-        case Mood:       return QString::null;
+        case Mood:       return QString();
         default: warning() << "Tried to get the text of a nonexistent column! [" << column << endl;
     }
 
-    return QString::null; //shouldn't happen
+    return QString(); //shouldn't happen
 }
 
 QString MetaBundle::prettyText( int column ) const
@@ -1099,7 +1099,7 @@
     if( seconds == Undetermined ) return "?";
     if( seconds == Irrelevant  ) return "-";
 
-    return QString::null; //Unavailable = ""
+    return QString(); //Unavailable = ""
 }
 
 QString
@@ -1622,7 +1622,7 @@
     }
 
     if( !fileref || fileref->isNull() )
-        return QString::null;
+        return QString();
 
     TagLib::ByteVector bv = readUniqueIdHelper( *fileref );
 
@@ -1647,7 +1647,7 @@
             return QString( md5.hexDigest().data() );
         }
         else
-            return QString::null;
+            return QString();
     }
 
     return QString::null;
@@ -1677,7 +1677,7 @@
     if( size != 8 )
     {
         debug() << "Wrong size passed in!" << endl;
-        return QString::null;
+        return QString();
     }
 
     QString str;
--- trunk/extragear/multimedia/amarok/src/moodbar.cpp #621457:621458
@@ -1321,7 +1321,7 @@
         path.truncate(path.findRev('.'));
 
         if (path.isEmpty())  // Weird...
-          return QString::null;
+          return QString();
 
         path += ".mood";
         int slash = path.findRev('/') + 1;
@@ -1341,7 +1341,7 @@
         path.truncate(path.findRev('.'));
 
         if (path.isEmpty())  // Weird...
-          return QString::null;
+          return QString();
 
         path = QString::number( deviceid ) + ','
           + path.replace('/', ',') + ".mood";
--- trunk/extragear/multimedia/amarok/src/playlistbrowser.cpp #621457:621458
@@ -3041,7 +3041,7 @@
     }
     if( dialog.exec() == Accepted )
         return dialog.result;
-    return QString::null;
+    return QString();
 }
 
 PlaylistDialog::PlaylistDialog()
--- trunk/extragear/multimedia/amarok/src/scriptmanager.cpp #621457:621458
@@ -289,7 +289,7 @@
 ScriptManager::specForScript( const QString& name )
 {
     if( !m_scripts.contains( name ) )
-        return QString::null;
+        return QString();
     QFileInfo info( m_scripts[name].url.path() );
     const QString specPath = info.dirPath() + '/' + info.baseName( true ) + ".spec";
 
@@ -794,7 +794,7 @@
             if( it.data().type == type )
                 return it.key();
 
-    return QString::null;
+    return QString();
 }
 
 
@@ -817,7 +817,7 @@
         if( runScript( *it, true ) )
             return *it;
 
-    return QString::null;
+    return QString();
 }
 
 
--- trunk/extragear/multimedia/amarok/src/smartplaylisteditor.cpp #621457:621458
@@ -604,7 +604,7 @@
     QString criteria = m_criteriaCombo->currentText();
 
     if( field.isEmpty() )
-        return QString::null;
+        return QString();
 
     if ( ( field=="statistics.playcounter" || field=="statistics.rating" || field=="statistics.percentage" || field=="statistics.accessdate" || field=="statistics.createdate") )
         searchCriteria += "COALESCE(" + field + ",0)";
--- trunk/extragear/multimedia/amarok/src/statistics.cpp #621457:621458
@@ -568,7 +568,7 @@
     else if( AmarokConfig::useRatings() )
         return i18n( "Rating: %1" ).arg( rating );
     else
-        return QString::null;
+        return QString();
 }
 
 void
--- trunk/extragear/multimedia/amarok/src/tagguesser.cpp #621457:621458
@@ -77,56 +77,56 @@
 QString FileNameScheme::title() const
 {
     if( m_titleField == -1 )
-        return QString::null;
+        return QString();
     return m_regExp.capturedTexts()[ m_titleField ];
 }
 
 QString FileNameScheme::artist() const
 {
     if( m_artistField == -1 )
-        return QString::null;
+        return QString();
     return m_regExp.capturedTexts()[ m_artistField ];
 }
 
 QString FileNameScheme::album() const
 {
     if( m_albumField == -1 )
-        return QString::null;
+        return QString();
     return m_regExp.capturedTexts()[ m_albumField ];
 }
 
 QString FileNameScheme::track() const
 {
     if( m_trackField == -1 )
-        return QString::null;
+        return QString();
     return m_regExp.capturedTexts()[ m_trackField ];
 }
 
 QString FileNameScheme::comment() const
 {
     if( m_commentField == -1 )
-        return QString::null;
+        return QString();
     return m_regExp.capturedTexts()[ m_commentField ];
 }
 
 QString FileNameScheme::year() const
 {
     if( m_yearField == -1 )
-        return QString::null;
+        return QString();
     return m_regExp.capturedTexts()[ m_yearField ];
 }
 
 QString FileNameScheme::composer() const
 {
     if( m_composerField == -1 )
-        return QString::null;
+        return QString();
     return m_regExp.capturedTexts()[ m_composerField ];
 }
 
 QString FileNameScheme::genre() const
 {
     if( m_genreField == -1 )
-        return QString::null;
+        return QString();
     return m_regExp.capturedTexts()[ m_genreField ];
 }
 
--- trunk/extragear/multimedia/amarok/src/tracktooltip.cpp #621457:621458
@@ -262,7 +262,7 @@
 void TrackToolTip::clear()
 {
     m_pos     = 0;
-    m_cover   = QString::null;
+    m_cover = QString::null;
     m_tooltip = i18n( "Amarok - rediscover your music" );
     m_tags    = MetaBundle();
     m_tags.setUrl( KURL() );
Comment 3 Alexandre Oliveira 2007-01-08 21:54:04 UTC
Notice that I removed the "return &QString()" change. This would make the function return a local reference, and it's not a good thing.
Comment 4 Andrew Ash 2007-01-08 21:59:59 UTC
Yes, definitely check these!

Where should I send future patches then?  I don't want to pollute the bug tracker.  Is attaching them to the mailing list ok?
Comment 5 Mark Kretschmann 2007-01-08 22:06:31 UTC
Bug tracker is actually fine.