On Tue, Jun 21, 2011 at 03:32:58PM +0200, Marc-André Lureau wrote: > > It's the only place where mute is an int, why not make it a uint8_t as > > everywhere else? > > Another issue I have with mute is that I assume it's a boolean, however > > there is no place at all where it's made obvious, it could very well be a 0 > > to 255 value and me making bad assumptions about it. If it's actually a > > boolean, maybe we could use bool here instead (though "bool" is only used > > in one other place in server/), or maybe it could be treated more like a > > boolean ? Eg use msg->mute = !!mute when sending the value to the server. > > Or naming it is_muted. Or simply adding a comment :) But I really think it > > would be nice to make this slightly more obvious. > > I fully agree with you, but to be consistent with the code around, I > used int (like "celt_allowed", which I will remove in a following > patch in favour a caps check function).
On the other hand, the "celt_allowed" name hints that it's a boolean, which "mute" does not. Can we use "is_muted" or have a comment in the struct mentioning it's a boolean value? > > snd_playback_send_volume has a return value to indicate failure. For now it > > won't happen: the only case when this could happen is if channel is NULL, > > which is tested here, and anyway the code would crash before. But maybe we > > should be robust against future changes and start by trying to call this > > function, and then update sin->st->volume if it succeeded, otherwise report > > an error? (same comment for the other public _set_ functions) > > We need to save current volume unconditionnaly so that later client > connections can receive it. Ah yeah, I was thinking it was a client => server message for some reason, not the other way :-/ Christophe
pgpzkjJxSK3jE.pgp
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/spice-devel
