Request: Inclusion of kio-upnp-ms to kde-runtime KIO slaves

[Nikhil Marathe]

On Wed, May 11, 2011 at 4:04 PM, Friedrich W. H. Kossebau
<kossebau at kde.org> wrote:
> Hi Nikhil,
>
> you know that today, April 12th, is already Hard Feature Freeze?! Would be
> really sad if your upnp-ms kio-slave misses the deadline now!

Hi Fredrik,
I have been moving halfway across the world the last 4-5 days and so I
couldn't really do anything.
I am at work, and don't have access to my laptop right now, but I'll
ensure your changes are in by tonight.
Aren't there circumstances under which certain features get some
leeway in being merged even after hard feature
freeze due to exceptional reasons? Any chance we can do this for the
slave, since the code itself has been pretty
well tested already.
>
> But there are a few possibilites for optimisation:
> * no use of QLatin1String, "" should be QString(), "'" should be (QLatin1)Char
> * QString::replace( "bla", "") used instead of QString::remove()
> * QHash::contains()/QHash::operator[] used instead of it=QHash::find/it!=0/it
> * toAscii() used instead of toLatin1()
>
> controlpointthread.cpp:
> * the global QRegExp searchCriteria should made const and static
> * the strings used to calculate the expression should be "define"s, so the
> preprocessor does the string calculation already
> * the timeout 35000 should be turned into a static const int at the begin of
> the file, so it can be easily found (could be even turned into a build time
> parameter)
> * ControlPointThread::slotEmitSearchEntry: signal listEntry emitted before
> state is finalized
>
> didlparser.cpp:
> * Parser::parseResource: use Resource::insert(Key, Value) instead of r[k]=v;
>
> objectcache.cpp:
> * ObjectCache::attemptIdToPathResolution: "m_idToPathRequestsInProgress =
> false;" should be before emit
>
> persistentaction.cpp:
> * some magic values, better as const static ints at begin:
>    m_delay = 1000;
>    m_timer->start( 5000 );
>
> kio_upnp_ms.cpp:
> * isn't that a busy loop in the end?
>    while( m_statBusy )
>        QCoreApplication::processEvents();
>
will work on these.

> Also think that instead of connect and disconnect of signal and slot with
> ControlPointThread you should rather use some good'ol'fashioned callback
> functions and some other way to exchange data with the worker thread.
well when the slave was multithreaded, this was required, and i've
stuck with them, but maybe they can be changed later.
>
>> If there is no objection I would like to request a merge into
>> kde-runtime. I will edit the 4.7 feature plan for the same.
>
> I would think this is already a request ;) But if there is not enough time now
> and others would like to do their own review before it gets into kde-runtime,
> as there has not been an official merge request yet via
> https://git.reviewboard.kde.org, let's at least do the trick to still be part
> of the next release wave by "upgrading" the repo from playground to
> Extragear/Base as fast as possible.

Do you mean I should abandon the inclusion in 4.7 and instead aim for 4.8?
I really think this should go in 4.7.

Thanks for the detailed response,
Nikhil