[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-devel
Subject:    Re: libphonon and kdebase4-runtime: Code advice/review
From:       Michael Pyne <mpyne () kde ! org>
Date:       2009-09-28 2:45:31
Message-ID: 200909272245.48325.mpyne () kde ! org
[Download RAW message or body]

[Attachment #2 (multipart/signed)]


On Sunday 27 September 2009 18:14:15 Colin Guthrie wrote:
> Yup, I guess it isn't the best list after all :s  The KDE bits that I
> want to change are all localised in: kdebase/runtime/phonon. So if that
> helps to direct to the right list, please let me know :)

phonon-backends or kde-core-devel then.

> So to answer a very simply question in a complex way, yes this is
> something I'd very much like to land upstream.

OK.

> > For instance I saw a ABI mention in this commit to kdesupport:
> > http://colin.guthr.ie/git/phonon/commit/?h=pulse&id=93e9fbac53fa39dd39f94
> >4c88e48e7078bab98c9 but you don't actually break ABI for C++ merely by
> > adding a virtual method to a class that already has virtual methods. (On
> > the other hand I'd recommend not inlining the method definition in the .h
> > just to be safe).
> 
> My thoughts of inlining it was to not require changes for other
> backends, but it's by no means necessary. (and like I say I've actually
> gone back on this idea now!

Well you could still give the default implementation, but in the .cpp file as 
opposed to the .h.  (having it inline in the .h file means that the compiler 
may insert it like a macro into calling code instead of making the function 
call.  If you later change the function call implementation then programs 
compiled against the older header would not pick up the change, which can lead 
to tricky issues).

> > Exported, yes. But I would only expect it to be used by the Phonon KCM
> > (although if we export it we need to be ready for any Phonon-using app to
> > get a hold of it ;)
> 
> Yeah true, but I guess that isn't too bad overall considering it's a
> fairly simple API. Is there some other trick we could do to make this
> harder? Or perhaps just by copying the header but not the full
> implementation? (although that latter option feels ugly too!)

No need to put the clamps down, client code that really cares about it will 
just edit the phononrc on disk or various other sundry hacks.

> > It sounds acceptable to me. I don't have any background info on why it's
> > not already exported, but the code looks like it's simply trying to defer
> > to the backend and platform plugin.
> 
> Cool. I guess I'm in the same boat (not knowing why its not already
> exported) as it seems to me a fairly obvious candidate for exporting.
> 
> Should I try and factor out the protected bits and wrap it up in such a
> way that this is not exposed? (e.g. the class only has a public interface?

Not sure, you may want to check in with Martin on that.

> > Your phonon-gst fixes look intriguing. :)
> 
> Aye, most of the changes are not gst specific, but I think gst is
> working a bit better now too.
> 
> That said, I did fix several bugs earlier in gst which are all in svn
> now (you can tell as my username is cguthrie on the bits already
> committed to subversion).
> 
> Neoclust (another Mandriva guy) is also fixing serveral bugs in
> phonon-gst too. We're on a bit of a triage mission to get it working
> better before the next Mdv release hits.

Awesome.  I think I'm in the minority of KDE users who use phonon-gst mostly, 
which I think is due only to the bugs present in our backend, so it's nice to 
hear there's work being done on it.

Regards,
 - Michael Pyne

["signature.asc" (application/pgp-signature)]

>> Visit http://mail.kde.org/mailman/listinfo/kde-devel#unsub to unsubscribe <<


[prev in list] [next in list] [prev in thread] [next in thread]