From kde-devel  Mon Sep 28 22:58:52 2009
From: Michael Pyne <mpyne () kde ! org>
Date: Mon, 28 Sep 2009 22:58:52 +0000
To: kde-devel
Subject: Re: libphonon and kdebase4-runtime: Code advice/review
Message-Id: <200909281859.00937.mpyne () kde ! org>
X-MARC-Message: https://marc.info/?l=kde-devel&m=125417882903518
MIME-Version: 1
Content-Type: multipart/mixed; boundary="--===============0109895563=="

--===============0109895563==
Content-Type: multipart/signed;
  boundary="nextPart1632356.YChXjLt9a6";
  protocol="application/pgp-signature";
  micalg=pgp-sha1
Content-Transfer-Encoding: 7bit

--nextPart1632356.YChXjLt9a6
Content-Type: Text/Plain;
  charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable

On Monday 28 September 2009 09:47:08 Colin Guthrie wrote:
> > 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).
>=20
> 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.
>=20
> 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)

You can add a .cpp file if necessary and just make sure it's referenced by=
=20
CMakeLists.txt.

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

All I mean is that there's no reason to put a lot of time into trying to ke=
ep=20
clients from abusing the exported class and yet allow the KCM to work.  If=
=20
it's an internal API for instance but must be public for this reason we jus=
t=20
document it as such in the APIDOX for the methods involved. i.e. @internal

Another evil thing to do is something like our virtual_hook(int method, voi=
d*=20
data) but don't document how to use it. ;)  (When we mean for client code t=
o=20
call a virtual_hook we provide wrapper functions to marshal/demarshal=20
appropriately).

But there's no reason to go this route as long as the public API can be use=
d=20
as documented (which can include documented as "do not use unless you are=20
kcm_phonon")

Regards,
 - Michael Pyne

--nextPart1632356.YChXjLt9a6
Content-Type: application/pgp-signature; name=signature.asc 
Content-Description: This is a digitally signed message part.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.11 (GNU/Linux)

iQIcBAABCAAGBQJKwT+0AAoJEAuvDJx7aunyjdMP/itfYuhudJacKGaDGcyicIkP
LgZXOvD3RjSvDaJnBQouiDRfefdFtxCYSg1J71zZoa9aFkishp8nbG/P5T8w4L7a
dAEcyJfij++qli7rIjMIQy3LtFvKR9MRNgD5XBW1UKkq/iI4CkBd2HHr+cTc4sVN
BdCcdu9JxjZg4niVS7lkrpnHrOb0atqGVG3FUUnYA5j7sHLDpg61iBA4nJ2p2D/y
TDRhU0R89IMjZqKumLRD3b/tfMkSikPe+DA88bGWy1dTNuagY7H9RNiivHOD1aTN
U6VgyNhKpa2nLHFNr69/g5Kf/2OYprPofW2bjUwdjLxbFFIGVL5sCX/mnkWVHTSR
8RdxEXv3UCtRDz6LKSkIhYIR4OU8J3A0ztTk0aRRxgpqy2f19XHzP9v1FNXwCrjk
qoe/qVSpl/bQe5t00RtsPIk3Gp4t4bjiRY/HHyeTlwegOTfkdByEKiJjC6Mgbbzk
/eVLvSWSjMXcpdEs+Acywgl5dW/4NWqWWa1SmpA6O/KOrMbMQXTY/uFUImIxDsaD
AokxERXTvj4n75cIUmSG09L8jfRrhBIr/RDiXv0G84SBK3e9yFqiL0Tf9/5rMWsC
EKB7g8N8D3hwMQsNKKK/XAiD+fybWaSGU2w6QyUr9JqKwT1i096u1O6Fvs1CbtyX
tIb9hRb25nV4EvCW51Go
=FvzC
-----END PGP SIGNATURE-----

--nextPart1632356.YChXjLt9a6--

--===============0109895563==
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: inline

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

--===============0109895563==--