DevHeads.net

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

Hi,

KDE SC 4.7 soft feature freeze is close, and I would like to propose
the UPnP MediaServer KIO slave
(<a href="https://projects.kde.org/projects/playground/base/kio-upnp-ms/" title="https://projects.kde.org/projects/playground/base/kio-upnp-ms/">https://projects.kde.org/projects/playground/base/kio-upnp-ms/</a>) 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 (<a href="http://herqq.org/news.html" title="http://herqq.org/news.html">http://herqq.org/news.html</a>) - 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.

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. I am confident about having it ready by
hard feature freeze.

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.

Regards,
Nikhil

Comments

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

By Friedrich W. H.... at 06/28/2011 - 19:08

Hi,

too bad noone was up to push it finally into kde-runtime when it was ready for
it, but let's pick it up again now:

Mardi, le 26 avril 2011, à 14:53, Nikhil Marathe a écrit:
But then Nikhil had his job life interfering, so could not spend any time on
pushing this request forward :(
I did a short code review, looked fine (could not comment on HUPnP usage, but
who can besides Nikhil?), and some code optimization patches I did even got
merged by Nikhil meanwhile.

I also would consider it ready for first serious usage. After all it will
enable all KIO-enabled programs to access the media content on UPnP media
servers out there (e.g. Xbox), at least to a certain degree.

So, release-team, what would you think about an exception to still include the
upnp-ms kio-slave in kde-runtime 4.7? After all it was ready, just noone
replied to him besides Kevin and me both saying "Keep it coming!".
It does not affect any other code, as it is an isolated kio-slave. Might not
even be compiled if some distro has not yet HUpnp packages. But if there are
HUpnp packages the Places integration (Dolphin/filedialog) for UPnP media
servers will be useless without this kio-slave. So distros would have to
install this kio-slave anyway. So it might make sense to have the kio-slave
already in kde-runtime, instead of extragear, where I would like to push it
otherwise, so it can be released together with the SC 4.7 officially and
distros know about this additional feature/kio-slave.

If you say No (understandable):
What is the process to make the playground/base repo of kio-upnp-ms an
extragear/base repo in times of git repos? Could not find it documented on
techbase, pointers welcome.
And how would that repo be marked that it should be included in the SC 4.7
release process?

If you say Yes (would be welcome):
Tomorrow/Today evening (wednesday, 29 june) I would see to create a branch
from kde-runtime master where the commit history of the kio-upnp-ms repo is
merged (been there, done that), have that reviewed by someone and then
backport that merge to 4.7 branch. Okay?

Cheers
Friedrich

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

By Friedrich W. H.... at 05/11/2011 - 19:04

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:
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?

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.

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
<a href="https://git.reviewboard.kde.org" title="https://git.reviewboard.kde.org">https://git.reviewboard.kde.org</a>, 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

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

By Nikhil Marathe at 05/11/2011 - 19:38

On Wed, May 11, 2011 at 4:04 PM, Friedrich W. H. Kossebau
< ... at kde dot org> wrote:
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.
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

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

By Friedrich W. H.... at 05/22/2011 - 07:49

Hi Nikhil,

Jeudi, le 12. mai 2011, à 01:38, Nikhil Marathe a écrit:
For starting on a job, I guess? So congrats for that, wish you much joy! :)

I would think there is a chance, if you can list the reasons:
Do you know of distris which have the upnp-ms kio-slave in use?
Which versions of Amarok make use of it/depend on it?
And what ever else you think makes the release-team and others confident the
inclusion now will be still okay and worth an exception.

From what grep tells me, there are only three strings to be translated (and
some more in the tests, but I do think you can/should remove i18n from there,
testers usually don't need/want translated strings), and only for errors, so
translators (and users) might be okay with an exception here.

<snipped content="comments on code optimizations" />

So do I, and surely do Kevin and the metalworkers (because e.g. the Places
integration would be useless otherwise). But you must push for it yourself,
you are the maintainer. Or ask somebody else to do that in-place for you if
life currently has swamped you with even more important tasks :) I would be
willing to do so, if you want.

Cheers
Friedrich

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

By Friedrich W. H.... at 06/02/2011 - 17:39

Hi,

(all follow-ups only to kde-core-devel please, copying also amarok-devel this
time to get your attention and comments, amarokers)

guess you are still heavily occupied by more important things, Nikhil :)
Still I think it would be good to have the upnp-ms kio-slave as part of the
SC 4.7 release, for now in extragear, not kde-runtime, given the state in the
release cycle (can move to kde-runtime for 4.8).
If you can share a minute please tell if you are okay with me pushing to get
your kio-slave into extragear, so it still can be officially part of SC 4.7

@Amarok people, please see below for a question to you.

Dimanche, le 22 mai 2011, à 13:49, Friedrich W. H. Kossebau a écrit:
@Amarok people: So since 2.4.0 Amarok makes use of the upnp-ms kio-slave,
right? Can you tell which distros already ship it?
And do you make use of that kio-slave's upnptypes.h, which has been renamed to
upnp-ms-types.h now? How is integration exactly done, what are the
dependencies, how do you detect the kio-slave?

Just pushed a branch "codeOptimization" with my proposals for those
optimization. If you find time, please give it a review and comment on it or
merge it to master, Nikhil :)
Adding the FindHUpnp.cmake is needed, as kdelibs does not install them, so
kio-upnp-ms needs a copy.

Cheers
Friedrich

Re: Re: Request: Inclusion of kio-upnp-ms t

By Bart Cerneels at 06/03/2011 - 06:40

On Thu, Jun 2, 2011 at 23:39, Friedrich W. H. Kossebau < ... at kde dot org> wrote:
Since Nikhil is busy I'm maintaining the upnp-ms collection in Amarok,
obviously we do depend on a KDE release with the upnp-ms KIO slave.
I'll try to take a look at your branch, but I'm expecting core devs to
pick up the slave.

Bart

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

By Friedrich W. H.... at 05/07/2011 - 07:51

Hi Nikhil,

Mardi, le 26 avril 2011, à 14:53, Nikhil Marathe a écrit:
Looking forward to that!
Having all the KIO-supporting code being able to access what is stored on
those UPnP mediaservers should make a few people happy!

Hope you are satisfied with your exams, now here are some quick comments on
the current master:

* Needed in toplevel CMakeLists.txt so FindHUpnp.cmake is found (might stop
people giving it a try):
set( CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} ${CMAKE_CURRENT_SOURCE_DIR} )
* As there is no docu yet, this line should be removed in
kio_upnp_ms.protocol:
DocPath=kioslave/kio_upnp_ms.html
* But then you might want to add a short docu about this protocol :)
* upnptypes.h better is renamed to upnp-ms-types.h or similar
* renamed upnptypes.h also would be added to kdelibs, as kde-runtime should
not be a compile-time dep. And then ideally merged into kio/udsentry.h, like
e.g. the Nepomuk ones are already

Have not looked more closely on the actual code, so no comment from me on that
for now.

Future:
Any plans for supporting writing?

Hoping to see your merge request for kio-upnp-ms popping up soon,
Cheers
Friedrich

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

By Nikhil Marathe at 05/09/2011 - 04:12

On Sat, May 7, 2011 at 5:21 PM, Friedrich W. H. Kossebau
< ... at kde dot org> wrote:
FindHUpnp.cmake is already in kdelibs/cmake. So I don't think this is required.

How should I approach this? Should I create a separate review request
for the UDSEntry addition
later, or should I do it now? Will this break binary compatibility?
The compile time dependency is only for programs which will treat UPnP
as special, and not normal kioslave users who use them via the Job interface.

Nikhil

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

By Friedrich W. H.... at 05/09/2011 - 18:01

Lundi, le 9 mai 2011, à 10:12, Nikhil Marathe a écrit:
Well, this was for the current repo, not for when included in kde-runtime :)
kdelibs does not install FindHUpnp.cmake, so when I ran cmake to test kio-
upnp-ms after I git clone'd the repo cmake complained:
--- 8< ---
CMake Error at CMakeLists.txt:10 (find_package):
Could not find module FindHUpnp.cmake or a configuration file for package
HUpnp.
--- 8< ---
You might have installed FindHUpnp.cmake yourself sometime, or have some env
variable setup properly, that maybe why you did not see that.

good.

Now, I think, as the kio-slave depends on it when compiled, so it should get
in first, so when people test your merge request for the inclusion of kio-
upns-ms to kde-runtime, they can (well, have to) compile it with the lastest
kdelibs.

Break binary compatibility of what? If you just add some more entries to the
enum StandardFieldTypes, starting with 29 or whatever the lastest number used
(+1) before UDS_EXTRA is in the list, this should not affect

Or do you mean programs using kio-upnp-ms already (like Amarok?)? Ah, true.
Tricky. Would not work then, okay. So it would stay just a separate header,
with the old enumeration.

Yes, but AFAIK this would be the first header installed from kde-runtime. And
as it is just a header, it should be fine in kdelibs.

Cheers
Friedrich

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

By Kevin Ottens at 04/26/2011 - 09:11

On Tuesday 26 April 2011 14:53:11 Nikhil Marathe wrote:
Yes! At last!

/me fairly happy to see that getting close to completion.

Regards.