[patch] slightly controversial refactor

[Jeff Mitchell]

On Saturday 20 January 2007 03:43, Andrew Turner wrote:
> The only problem I can see with the code (pre-625102) is that while I
> assume the m_mediumMap should hold the pointers to all the allocated
> objects, you have the situation (when the named medium is found) of
> allocating one object for returnedMedium, but another for m_mediumMap.
> This is presumably because the objects in m_mediumMap can be deleted
> at any time (the same function seems to like deleting and recreating
> them whenever it's run), however this design does place the burden on
> the caller to delete the returned object when they're done with it.
> I'm not quite sure though, if the m_mediumMap contents can't be
> trusted (because we might delete the objects pointed to), what their
> purpose is.

I'll explain a bit about what's going on in getDevice.  The purpose of the 
function isn't just to return a Medium object, it's to refresh that object as 
well.

When the function is called, it retrieves a new list of the current devices 
from mediamanager.  If the new list contains a matching device, it updates 
m_mediumMap with the new object.  The reason for this behavior is that device 
properties can change at any time, so we want to be sure that at the 
particular time that we get a device's information, it's up-to-date.  This is 
used, for instance, when the mediamanager sends out a mediumChanged signal.  
mediumChanged only gives a QString with the name of the device, so we have to 
refresh its information ourselves.  In this instance, getDevice is called on 
that medium, which refreshes the information from mediamanager and then sends 
a pointer to the updated Medium (residing in m_mediumMap) out to clients.

The reason m_mediumMap exists is that there are particular functions that 
iterate through that map to build a list of devices.  This is how the table 
with device information gets generated in the settings dialog box, for 
instance.  At those times, we don't have a specific name of an object that we 
want to query for (at least, not if the medium is an autodetected one), so we 
rely on m_mediumMap to contain that information.

> Also, I don't truly know whether all the m_mediumMap objects get freed
> eventually. Certainly there seems to be code to free them in
> mediumRemoved, but I would have expected a destructor for the
> DeviceManager to free them too. However, just because there isn't one,
> doesn't mean it doesn't get done somewhere.

The onus isn't really on the caller to delete the returned object; we want 
that object to stay valid in m_mediumMap, unless it's refreshed by some 
mechanism (as in getDevice).  So really what should happen is that in the 
DeviceManager's destructor code, all objects in m_mediumMap should be 
deleted.  But that's the only memory leak going on, and it's not a hugely 
important one as the DeviceManager is never deleted until Amarok exits.  
Fixing that would be more of a completeness exercise than anything else but 
should probably be done.

> As for Eric's patches, the first one (already applied) doesn't appear
> to actually fix any leaks, but it won't hurt. The second one does, I
> think, improve clarity and is worth applying. I don't understand why
> deleting and then recreating every device is necessary, though, but
> the old code did it too.

I agree that the patch improves clarity and should be applied, and hopefully 
you now understand that it's not every device that's deleted and recreated 
every time getDeivce is called, just the particular one being refreshed (if 
it exists), and also why it's done.

--Jeff