[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:       Colin Guthrie <gmane () colin ! guthr ! ie>
Date:       2009-09-28 13:47:08
Message-ID: h9qeor$gpu$1 () ger ! gmane ! org
[Download RAW message or body]

'Twas brillig, and Michael Pyne at 28/09/09 03:45 did gyre and gimble:
> 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.

Cool. I'll follow up specific queries there if needed.

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

There is no default .cpp file here: it's an abstract interface. It's up 
to all the backends to implement it themselves, so there would be no 
default implementation I could make except in the header file. If there 
had been a .cpp file for this, I would have put it there.

Have I missed something or was this just a general "it should go in the 
cpp file" type code review answer? (I agree totally if it is!! :p)

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

Well this is really what I'm trying to avoid. While the config file will 
be used in some cases, what I'm actually implementing at the backend is 
an alternative to the config file used when the audio backend ultimately 
uses pulseaudio. So the main design goal from my perspective here is to 
make sure client code is totally unaware of any config file at all... 
i.e. it must have an API to access, query and change the config, never 
access it directly via a file.

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

Cool, I'll do that via the more appropriate channels mentioned earlier.

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

Yeah we use it by default in Mandriva for various reasons, and I stick 
with it mainly due to some perverse opinion that it's technically most 
appropriate than therefore is the one I should use, even if it does have 
annoying bugs, problems and limitations in it's current form.

Fingers crossed we can bring it up to speed.

Thanks loads for your replies Michael.

Col


-- 

Colin Guthrie
gmane(at)colin.guthr.ie
http://colin.guthr.ie/

Day Job:
   Tribalogic Limited [http://www.tribalogic.net/]
Open Source:
   Mandriva Linux Contributor [http://www.mandriva.com/]
   PulseAudio Hacker [http://www.pulseaudio.org/]
   Trac Hacker [http://trac.edgewall.org/]

 
>> Visit http://mail.kde.org/mailman/listinfo/kde-devel#unsub to unsubscribe <<
[prev in list] [next in list] [prev in thread] [next in thread]