[patch] slightly controversial refactor

[Andrew Turner]

Jeff,

I read what you wrote. I just disagree about what the code used to do.

code snippet from pre-patched version
(http://websvn.kde.org/trunk/extragear/multimedia/amarok/src/devicemanager.cpp?rev=613534&view=markup):

    for ( it = currMediumList.begin(); it != currMediumList.end(); it++ )
    {
        if ( (*it).name() == name )
        {
            returnedMedium = new Medium(*it);
        }
        if( m_mediumMap.contains( name ) )
        {
            tempMedium = m_mediumMap[(*it).name()];
            m_mediumMap.remove( (*it).name() );
            delete tempMedium;
        }
        m_mediumMap[(*it).name()] = new Medium(*it);
    }

pseudo code equivalent (imho):

void getDevice(named device)
{
   <snip>
   for (current device = iteration through all kded devices)
   {
       if (current device is named device)
          create current device and return pointer later;
       if (_named_ device already exists in mediumMap)   // look
          delete _current_ device and remove from mediumMap;  // here
       create current device and store in mediumMap;
   }
   <snip>
}

I have to say, having written the pseudo-code, I've realised it
doesn't do what I thought it did, but I'm guessing it doesn't do what
you thought it did either. The fact that it tests for the named device
being in the mediumMap, but then deletes the current device seems to
me like a mistake which, if the named device was present, would
recreate all devices.

(I'll admit, I originally read it as testing for the current device
existing in the mediumMap.)

Anyway, I'm happy with what we've ended up with, so this is mainly
academic, really.

Andrew

On 21/01/07, Jeff Mitchell <kde-dev at emailgoeshere.com> wrote:
> On Saturday 20 January 2007 15:00, Andrew Turner wrote:
> > and by removing the unexpected side-effect of
> > updating all devices from getDevice to its proper place in a separate
> > function.
>
> ????
>
> I guess you didn't read my earlier email in this thread...so to clarify,
> getDevice never updated all devices.  It did request a new list from kded's
> mediamanager, but only to refresh the one you were looking for by name.
> This behavior hasn't changed.
>
> As a side effect, it would populate m_mediumMap if new devices were found,
> (because why not, you've already spent the processing time so it's free), but
> if a device already existed, it only refreshes the one passed in.  Hence
> passing in "init" and "dummyjusttorerundcop" before (it would never find
> those, so it would, in the process of searching, add all mediums that
> mediamanager detected to m_mediumMap).
>
> Erik's new function is orthogonal; it does a refresh on every device.  This is
> really good functionality to have around because it updates all of the device
> characteristics (in case they've changed).  With getDevice it would update a
> specific device we told it to, but not all devices (since the only thing it
> would do with devices not matching the one we were looking for was add it to
> the map, if they were encountered before the one we wanted), so there wasn't
> a great way to do a full refresh of our m_mediumMap.  With Erik's function we
> don't have to know ahead of time what devices we are looking for in order to
> update them.  Plus it gets rid of the silly arguments that were being passed
> into getDevice...
>
> --Jeff
>
> >
> > Andrew
> >
> > On 20/01/07, Jeff Mitchell <kde-dev at emailgoeshere.com> wrote:
> > > Erik, no problem.  I appreciate that you worked on this.
> > >
> > > --Jeff
> > >
> > > On Saturday 20 January 2007 14:30, Erik Hovland wrote:
> > > > On Sat, Jan 20, 2007 at 08:52:44PM +0000, Andrew Turner wrote:
> > > > > Wait, ignore me. The patch didn't fix the memory leak, but Jeff did,
> > > > > and in the same commit.
> > > >
> > > > I knew my patch needed more work.
> > > >
> > > > Thanks to Jeff for taking a look and sticking with it. I was struggling
> > > > with it on my end.
> > > >
> > > > E
> > >
> > > _______________________________________________
> > > Amarok mailing list
> > > Amarok at kde.org
> > > https://mail.kde.org/mailman/listinfo/amarok
> >
> > _______________________________________________
> > Amarok mailing list
> > Amarok at kde.org
> > https://mail.kde.org/mailman/listinfo/amarok
>