[patch] slightly controversial refactor

[Erik Hovland]

I know I have svn write access so posting here is really for my own warm
fuzzies.

Some of you might have noticed commit version 625102 where I closed a
couple of small memory leaks. When closing those leaks I noticed that
the two callers of DeviceManager::getDevice( name ) were not calling
them to get a device. And that they had leaks specifically because they
could care less if they got a device. Instead they meant to query kded
for any devices and put them in the device manager. This is evident when
you notice that DeviceManager::getDevice( name ) is coded up so that it
intentially has a side effect of reconciling the
DeviceManager::m_mediumMap.

So I decided to do a little refactoring. First, I recoded getDevice() so
that it does get the device and only the device (reconciling only that
device in the m_mediumMap). Then I wrote another member function called
reconcileMediumMap() to do the work of reconciling m_mediumMap to what
ever kded returns. Now switch the two errant callers to the new
function... and viola! Now we have consistent use of DeviceManager.

I even tried this patch out. But I am no device/mount point expert. So
rather then commit my junk and live with it, I figured I would at least
submit it to the list for the once over. Let me know what you think. I
am willing to rework it or drop it since my previous fix does close the
leak.

Thanks

E

-- 
Erik Hovland
mail: erik AT hovland DOT org
web: http://hovland.org/
PGP/GPG public key available on request
-------------- next part --------------
Index: amarok/src/devicemanager.cpp
===================================================================
--- amarok.orig/src/devicemanager.cpp
+++ amarok/src/devicemanager.cpp
@@ -41,7 +41,6 @@ DeviceManager::DeviceManager()
     m_dc = KApplication::dcopClient();
     m_dc->setNotifications(true);
     m_valid = false;
-    m_initMedium = 0;
 
     if (!m_dc->isRegistered())
     {
@@ -69,7 +68,7 @@ DeviceManager::DeviceManager()
             {
                 debug() << "During DeviceManager init, error during DCOP call" << endl;
             }
-            m_initMedium = getDevice( "init" );
+            reconcileMediumMap();
             debug() << "DeviceManager:  connectDCOPSignal returned successfully!" << endl;
         }
     }
@@ -77,7 +76,6 @@ DeviceManager::DeviceManager()
 
 DeviceManager::~DeviceManager()
 {
-    delete m_initMedium;
 }
 
 void
@@ -207,7 +205,6 @@ DeviceManager::getDevice( const QString 
         return NULL;
     debug() << "DeviceManager: getDevice called with name argument = " << name << endl;
     Medium* returnedMedium = 0;
-    Medium* tempMedium = 0;
     MediumList currMediumList = getDeviceList();
 
     Medium::List::iterator it;
@@ -216,19 +213,42 @@ DeviceManager::getDevice( const QString 
         if ( (*it).name() == name )
         {
             returnedMedium = new Medium(*it);
+            MediumIterator secIt;
+            if ( (secIt = m_mediumMap.find( name )) != m_mediumMap.end() ) {
+                Medium* tempMedium = (*secIt);
+		m_mediumMap.remove( name );
+                delete tempMedium;
+            }
+            m_mediumMap[ name ] = new Medium(*it);
+            break;
         }
-        if( m_mediumMap.contains( name ) )
-        {
-            tempMedium = m_mediumMap[(*it).name()];
-            m_mediumMap.remove( (*it).name() );
-            delete tempMedium;
-        }
-        m_mediumMap[(*it).name()] = new Medium(*it);
     }
 
     return returnedMedium;
 }
 
+void
+DeviceManager::reconcileMediumMap()
+{
+    DEBUG_BLOCK
+    if ( !m_valid )
+        return;
+
+    MediumList currMediumList = getDeviceList();
+
+    Medium::List::iterator it;
+    for ( it = currMediumList.begin(); it != currMediumList.end(); it++ )
+    {
+        MediumIterator locIt;
+        if ( (locIt = m_mediumMap.find( (*it).name() )) != m_mediumMap.end() ) {
+	    Medium* mediumHolder = (*locIt);
+	    m_mediumMap.remove( (*it).name() );
+	    delete mediumHolder;
+	}
+	m_mediumMap[ (*it).name() ] = new Medium(*it);
+    }
+}
+
 QString DeviceManager::convertMediaUrlToDevice( QString url )
 {
     QString device;
Index: amarok/src/devicemanager.h
===================================================================
--- amarok.orig/src/devicemanager.h
+++ amarok/src/devicemanager.h
@@ -45,6 +45,8 @@ class DeviceManager : public QObject
 
         MediumMap getMediumMap() { return m_mediumMap; }
         Medium* getDevice( const QString name );
+        // reconciles m_mediumMap to whatever kded has in it.
+        void reconcileMediumMap();
 
         bool isValid() { return m_valid; }
 
@@ -71,7 +73,6 @@ class DeviceManager : public QObject
         DCOPClient *m_dc;
         bool m_valid;
         MediumMap m_mediumMap;
-        Medium* m_initMedium;
 
 };
 
Index: amarok/src/mediumpluginmanager.cpp
===================================================================
--- amarok.orig/src/mediumpluginmanager.cpp
+++ amarok/src/mediumpluginmanager.cpp
@@ -14,6 +14,7 @@
 #include "debug.h"
 #include "deviceconfiguredialog.h"
 #include "mediadevicemanager.h"
+#include "devicemanager.h"
 #include "hintlineedit.h"
 #include "mediabrowser.h"
 #include "medium.h"
@@ -114,10 +115,8 @@ MediumPluginManager::detectDevices( cons
 {
     bool foundNew = false;
     KConfig *config = Amarok::config( "MediaBrowser" );
-    if( redetect ) {
-        Medium *dummyMedium = MediaDeviceManager::instance()->getDevice( "dummyjusttorerundecop" );
-        delete dummyMedium;
-    }
+    if( redetect )
+        DeviceManager::instance()->reconcileMediumMap();
     MediumMap mmap = MediaDeviceManager::instance()->getMediumMap();
     for( MediumMap::Iterator it = mmap.begin(); it != mmap.end(); it++ )
     {