-
Notifications
You must be signed in to change notification settings - Fork 19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add ALSA-based volume control and test logic #8
base: ev3dev-jessie
Are you sure you want to change the base?
Conversation
How about calling it "Sound" instead?
I would rather just have volume up and down (eventually, we should just have a graphic slider, but text is OK for now). If we make the change step by 10%, then you don't really need the half/min/max. Also, if the volume is set to 0, then behind the scenes, it should activate the mute if it exists, so we don't need a separate mute option.
It sounds like you are doing MVVM in which case I would call it IMixerElementViewModel. However, this is not how any of the other views/controllers are implemented. In other controllers, we bind properties of the data model to the view rather than binding properties of a view model in the view as you are doing here. There's nothing wrong with doing it the way you have here, but I would rather stay consistent with the existing patterns. By using the property binding feature of GObjects, we don't have to implement a function to attach to each notify signal. We only need a translate function for special cases like converting an enum to a string.
The alsa library is in pkg-config, so for now, you can add it to |
OK, I'll change that.
I think there needs to be either a mute option or a "minimum" option; having to hit the "volume down" button 10 times to mute the audio would probably get pretty annoying. How about I remove the mute button as it stands along with the half and max buttons and rename the "minimum" button to "Mute"? That way, you can still easily mute it, and it all fits on one page. Barring a slider, I was originally hoping for something simple that would let you use the left/right buttons to modify the volume so that you didn't need to select the separate menu items. The current capabilities of ev3devKit weren't enough to lay that out without significant additions, however. We'll have to revisit this sound UI if/when someone implements a slider or other similar control.
Yes, I aimed for something closer to MVVM or MVC because it seemed like there was a fair amount of manual UI management in the controller that I wanted to avoid. Essentially, I wanted to avoid the controller having any references to
Edit: After looking through the code, I've decided I won't try to add bindings to replace notify signals. There isn't anywhere meaningful that it would help simplify the code. The biggest issues are with interfacing with the control panel GUI, and that is limited by Gtk+'s capabilities, not our code.
Will-do.
Oh, I think I see what I did wrong -- I didn't clear the cache. After doing that, I was able to remove my other hack and it seems to be working. |
PR updated. |
|
||
namespace BrickManager { | ||
public class AlsaBackedMixerElement: IMixerElementViewModel, Object { | ||
private const SimpleChannelId primary_channel_id = SimpleChannelId.MONO; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you planning on changing this value? I don't see a need to. So, we don't really need a constant for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the equivalent of a "magic number" and is used in two places, so having it as a constant makes the code more readable and maintainable. While I don't intend to change it, I already had to change it a few times in development, so there's no reason to think I wouldn't need to change it again.
IMixerElementViewModel element_b = b.represented_object as IMixerElementViewModel; | ||
|
||
// Group by name, and sort by index within the same name | ||
if (element_a.name == element_b.name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{}
around if/else bodies
On the mixer element view model, you have the properties Also, the "Is muted" checkbox is disabled in the desktop test control panel. We should be able to change this unless |
mixer_select_window = new MixerElementSelectorWindow (); | ||
|
||
mixer_select_window.mixer_elem_selected.connect ((selected_element) => { | ||
if (volume_window == null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{}
The
I disabled the |
I've commented on the few items that require discussion; I'll make these changes later today or tomorrow. |
🆕 |
I haven't been in brickman in quite a while, but I have thought about this a bit (I haven't looked at the code in a while, so I may be out of touch with reality but...). I'm still not a fan of using a view model in brickman. The reason being that view model makes sense for a framework like WPF where you have automatic data binding via the IDE. We don't have that here, so essentially, we are doing the data binding twice, once in the view model and again in the controller. I would rather just do it once. |
I decided to create that view model class only after I had implemented a fair amount of functionality without it. Every time I would implement something new, whether it was a property in the test app or the volume controls in the main window, I found myself cringing at the code I was writing. Part of my unhappiness was definitely a result of the much uglier code required to get the desktop test properly configured, which was actually the first thing I implemented -- even before the real controls or display. But fundamentally, I decided that most of the things I didn't like were a result of the fact that I was trying to model controls which modified a set of individual devices without any class to represent a single device, which is something that goes against the fundamental idea of OOP. So, I locally committed what I had and modified the code to use the mixer class that's in there now. I can't remember the specifics, but after comparing the two versions I decided that the one which used the view model class was cleaner and shorter in most cases, so I continued with that version. I have no idea if there are things I could've done better with the original to make it less ugly or if there were design properties I wasn't considering (after all, I had never used GTK+ before, and the interaction between that and the GObject model was something that seriously confused/upset me) but I made a judgement call between the two options nonetheless. To a large extent, I chose the direction that was the most common in the OOP projects that I have built and been involved in, because I have a deep understanding of its benefits and pitfalls at the "design pattern" level. If you are unhappy with the way I implemented this, I have no issue with you taking whatever parts of it you think are useful and re-structuring it to remove the view model. However, at this point I've moved my focus to other ev3dev-related things (I'm currently working on drastically improving the ev3dev-lang-python README as well as planning a new structure for the ev3dev docs) so I don't have time to allocate to changing the structure of this code. If you don't decide to do it yourself, I might be able to in a few weeks once I am happy with the state of these other things. |
Yes, this is old, comeone? Can we get some updates? This looks amazing and should be ready for a push. |
This PR adds an "Audio" section in the main menu and associated sub-windows to allow volume control from brickman. The primary volume page looks like this:
![volume control window](https://cloud.githubusercontent.com/assets/3310349/16213296/2f017ce0-3703-11e6-8b37-c6aa359dd5b0.png)
There is also a list window that only appears if there are no mixer elements or there is more than one mixer element. That window can be accessed from the desktop test.
That menu has the following options:
This audio system is architected slightly differently from other current modules in brickman. I created a
ITestableMixerElement
interface (not a perfect name, but the best I could come up with) with two implementations: one of them is theAlsaBackedMixerElement
and the other is theFakeMixerElement
. The views operate onITestableMixerElement
s, so the controllers just instantiate whichever implementation they need and hand it to the view and the view handles the common display logic.The views connect to the
notify
signals on elements they are given, so they update automatically whenever the underlying elements change. This is mostly used by the test implementation, but it is also how the volume percentage on the main page operates: when the user hits a volume button, the view emits a signal; the controller then updates the volume value on the base element accordingly and the view gets the property change notification and updates the label.Some notes:
CMakeLists.txt
additions correctly. Specifically, on line 139, I addedasound
after${DEPS_LIBRARIES}
because I couldn't figure out where that variable was coming from. Guidance on where that should go would be appreciated.