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

[Friedrich W. H. Kossebau]

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!

Mardi, le 26 avril 2011, à 14:53, Nikhil Marathe a écrit:
> Hi,
> 
> KDE SC 4.7 soft feature freeze is close, and I would like to propose
> the UPnP MediaServer KIO slave
> (https://projects.kde.org/projects/playground/base/kio-upnp-ms/) be
> included into the set of kio slaves shipped with kde-runtime. The
> slave was created as part of GSoC 2010 - Amarok and KDE UPnP support
> and it was decided that it should be merged into kde-runtime at some
> point.
> 
> A couple of reasons I believe the slave is now ready for standard release
> is: 1) HUpnp (http://herqq.org/news.html) - the Qt based UPnP library used
> by the slave has a stable API and ABI with the release of 1.0.0 about 3
> weeks ago.
> 2) The slave has been considerably simplified and single threaded, and
> stable now.
> 3) The slave is independent and can be conditionally compiled and
> installed if HUpnp is installed. kdelibs already contains a
> FindHUpnp.cmake to find the HUpnp library.
> 4) The Solid UPnP backend (enabled in 4.7, again if HUpnp is found)
> automatically launches UPnP media servers in the file manager with the
> slave.

Works nicely*. And given that, the Solid UPnP backend + showing up the devices 
are completely/almost useless if there is no UPnP MediaServer kio-slave to 
access them, so please let's see to have this kio-slave available with the 
next SC release.

*Besides, why isn't e.g. for images not the largest possible size delivered? 
Guess similar for audio/video streams only some medium quality is fetched per 
default? I think following what e.g. the MediaTomb web UI does, giving the 
best possible quality, would be better here.
Also could deliver a little bit more data, like Mimetype and Timestamps?

> My exams get over this week and I can ensure that krazy checks pass
> and the code is cleaned up some more.
> There is inline documentation where required, and the search and
> browse API documentation exists. There is no user
> manual since it is a slave.

Had a short look at the code, could not see any big flaws. And it works, so 
nothing which should stop the inclusion in kde-runtime
(Beware, I have no clue of HUPnP, so could not stop any misuse of its API, and 
have no idea how to talk exactly to a MediaServer)

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();

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.

> 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.

Cheers
Friedrich
-- 
Desktop Summit 2011 - Berlin, Germany - August 6-12th - www.desktopsummit.org