On 02/14/2014 01:51 PM, Tanu Kaskinen wrote: > On Fri, 2014-02-14 at 11:55 +0100, David Henningsson wrote: >> On 02/14/2014 10:49 AM, Tanu Kaskinen wrote: >>> Does the grouping have any benefits? If pa_volume_control_info has a >>> field with this pa_cvolume2 type, then it could be argued that for >>> symmetry there should also be function >>> pa_context_set_volume_control_volume() that takes a pa_cvolume2 struct >>> as a parameter. However, when an application sets the attributes of a >>> volume control object, usually it will only modify the overall volume or >>> the balance, not both, and the channel map is immutable, so passing a >>> pa_cvolume2 struct to the setter function is inconvenient. >> >> A volume control UI with presets would typically set both. So would any >> volume control UI do that does not look exactly the way you anticipate. >> >> E g, pavucontrol has two sliders, one left and one right, and gnome >> volume control has main volume + balance. > > I thought about pavucontrol, but not thoroughly enough. I thought that > if you change only one channel, only balance needs to be changed, but of > course that only works if the channel in question isn't the highest > volume channel. > >> The grouping would also enable potential helper functions such as those >> we have for pa_cvolume today. > > Good point. > > I think grouping the overall volume, the balance and the channel map > together makes sense. About the struct naming: do you know what "c" in > "cvolume" refers to? I would guess that it means "channel". pa_cvolume > very concretely has per-channel volume (a pa_volume_t value for each > channel). It could be argued that pa_cvolume2 is more balance-oriented > than channel volume oriented, so here's an idea for a name without > numbers: pa_bvolume. Thoughts? Sure, pa_bvolume sounds as good as anything else to me. >>> I'd like to keep mute controls completely separate from volume. They are >>> not inherently linked to each other (even though they very often appear >>> together). >> >> I don't understand this argument. A mute control *is* a volume control >> (with two distinct values). > > That doesn't change the fact that the volume and mute controls should be > separate: I don't want to bundle two volume controls together either. > > However, I'd be ok with adding a possibly-NULL reference from a volume > control object to a mute control object and vice versa. There are other > ways to correlate a volume control with a mute control, but direct > reference would definitely make it easier. Mute controls and volume controls should be the same type of objects. They will share a lot of logic internally and it would lead to a lot of copy-pasting if they were two different kind of objects. I'd prefer if they also were the same object, because I think that would make it simpler for volume control UIs instead of having to follow links between objects. >> In this case, it's a way to avoid volume >> information lost on muted audio. >> Well, and that everything said so far in this thread would apply equally >> well to mute controls, so I guess you will come back in a few months and >> want to add another core object for mute controls if we don't get it in >> now ;-) > > Indeed, I'd like to have mute control objects too. That's not so high > priority for me, however, so I didn't include it in my initial proposal. > It can be added cleanly later. > >>> What would you think about putting the icon name as a separate field to >>> the pa_volume_control_info struct? I'm not against adding a proplist for >>> undocumented metadata, but let's not abuse the proplist for things that >>> are part of the documented stable API (any more than what we already >>> do). >> >> IIRC, icon names are in proplists everywhere else, so having them in a >> proplist here as well would avoid inconsistency. > > True. While I'd prefer a separate field, this is a very small issue that > can be fixed at any point in the future if we want, so let's agree to > put the icon name to a proplist for now. > >>>> The volume classes however seem more like routing/policy implementation >>>> internals. They could be kept inside the routing/policy module, exported >>>> through property lists, or something like that. Either that, or I didn't >>>> really understand the concept. >>> >>> Applications should have a unified way to query what volume classes >>> exist and what volume control they are associated with. An example of a >>> volume class is the event volume class. If applications want to access >>> that, they currently need to use a module-stream-restore specific API, >>> even though the concept of an event volume class is not specific to >>> module-stream-restore. >> >> The concept "volume class" does not exist in module-stream-restore, at >> least not with that name. >> >> Could you give a clear definition of what "volume classes" are? If it's >> just a way of grouping volume control objects, then volume control >> objects could just have some property of what class they belong to. > > An attempt at a definition: a volume class is a named set of rules for > grouping volumes. > > "A way of grouping volume control objects" is not an entirely accurate > description. You probably were thinking of a setup where each stream has > a volume control object, and those are somehow grouped by a volume > class. In my proposal streams whose volume is governed by the volume > class do not have their own volume control objects. Instead, the streams > reference the volume control object of the volume class. > > This is not a very important distinction, however, because your > suggestion of using a property works either way. You could set a > "volume_class = event" property on a volume control to tell clients that > this volume control object controls the volume of event sounds. I'm still confused. * Is there a 1:1 relation between volume control objects and volume classes? If so we don't need an additional object, they can just be the same object. * Or can several volume control objects belong to the same class? If so "the volume control object of the volume class" does not make sense because there can be many. * Or can a volume control object belong to several classes? If so the property on a volume control does not work (unless you do something like "volume_classes = foo,bar"). > Apart from my usual complaint about "abusing" proplists, there are some > other issues too with the property-on-a-volume-control approach: > > 1) You can't get a list of all volume classes in the system. Ok, you > can, by checking the "volume_class" property on every volume control > object. Not pretty, but you can do it. > > 2) After getting the list of all volume classes, you can't show a > human-readable label for them in a user interface, because all you have > is the non-translatable volume class identifier. The volume control > description can probably be usually used as a replacement, however, so > this is probably not a big issue in practice. (An example where reusing > the volume control label for volume classes might be problematic: on the > Nokia N9 the volume classes point to different volume controls depending > on the current routing.) > > 3) The human-readable label is one example of a volume class attribute. > There could be others in the future, but those can't be implemented if > there's no object to attach the attributes to. > > In my opinion volume classes deserve to be treated as full-blown > objects, but if I'm the only one thinking that, I won't insist on doing > it that way. The property-on-a-volume-control approach works too. About taking things in iterations, would it work for you to get started on the volume control objects and skip the volume classes for the time being? And postpone that to a later iteration. -- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic
