On Wed, Aug 10, 2011 at 08:38:01AM -0700, Keith Packard wrote: > On Wed, 10 Aug 2011 15:48:30 +0200, Luc Verhaegen <[email protected]> wrote: > > > This was not done for a few reasons: > > 1) it clearly splits between clientside and server internal usage. The > > comment that i added to RRDeleteOutputProperty in the very first version > > of the patch underlined that this was only for server internal use due > > to immutable (this was the code tested on the N9, but was never > > committed). > > 2) ValidAtom is only ever called in the client side functions, while > > the check for immutable is once in general and once in client side, > > putting the balance client side, so i picked client side. > > 3) it is much more invasive. > > I'd prefer to see these two approaches mixed. > > I like not duplicating code, which means using RRDeleteOutputProperty > and having that take the 'force' parameter. It's a bit ugly in that > server internal paths will end up casting the return to (void) as the > function cannot fail when force == TRUE, but I balance that with not > duplicating the whole function with one check inserted. > > But, I also think the ValidAtom check should only be in the client path; > I would have to go look at the server reset code carefully to know > whether the atom table is cleared before or after the RandR outputs are > destroyed. Not changing the semantics of RRDeleteOutputProperty as it is > used internally seems fairly important.
A cleaner way, which keeps us from nowing the internal structure of the property list, and which does not require us to change any functions besides the ProcRRDeleteOutputProperty, is: * call ValidAtom(stuff->property) [throw BadAtom] * retrieve the property structure by calling RRQueryOutputProperty(output, stuff->property) [throw BadName] * check the property's immutable flag [throw BadAccess] * call RRDeleteOutputProperty(output, stuff->property) * return Success But all the other client side handlers walk the list manually. Luc Verhaegen. _______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
