Added wireplumber mixer widget #307
Conversation
AKArien
changed the title
|
Actually, the audio sources controls seem very broken. Re-marking as draft. |
|
Should be good !'m not running into bugs in my own use anymore. |
src/panel/widgets/wireplumber.hpp
Outdated
|
|
||
| static std::vector<WayfireWireplumber*> widgets; | ||
|
|
||
| static void init_wp(); |
There was a problem hiding this comment.
Do not define static functions and objects in headers. These functions should go into wireplumber.cpp.
Also I don't like the global variable above...
There was a problem hiding this comment.
Also I don't like the global variable above...
As far as i see, the alternative is connect the signals with the WayfireWireplumber as user data, for each widget ? Is that better or worse ?
src/panel/widgets/wireplumber.cpp
Outdated
| #include <gtkmm.h> | ||
| #include <iostream> | ||
| #include <glibmm.h> | ||
| #include "gio/gio.h" |
There was a problem hiding this comment.
Please use <...> for library headers.
src/panel/widgets/wireplumber.cpp
Outdated
| mute_conn = button.signal_toggled().connect( | ||
| [this, id] () | ||
| { | ||
| GVariantBuilder gvb = G_VARIANT_BUILDER_INIT(G_VARIANT_TYPE_VARDICT); |
There was a problem hiding this comment.
Please use Glib::Variant instead of C glib functions.
There was a problem hiding this comment.
So, I’m sorry but for some reason i can’t seem to figure it out… Do you know how to do it ?
My best shot is something like this :
auto dict = Glib::VariantDict::create();
dict->insert_value_variant("mute", Glib::Variant<bool>::create(button.get_active()));
gboolean res FALSE;
g_signal_emit_by_name(WpCommon::mixer_api, "set-volume", id, dict->gobj(), &res);But it aborts on a type info check assertion and i seem to phenomenally fail at looking anything up.
There was a problem hiding this comment.
I am no dbus expert but take a look at this other PR, maybe it would give you an inspiration: #311 (comment)
src/panel/widgets/wireplumber.cpp
Outdated
| return; | ||
| } | ||
|
|
||
| std::map<VolumeLevel, std::string> icon_name_from_state = { |
There was a problem hiding this comment.
icon_name_from_state can be made static const.
src/panel/widgets/wireplumber.cpp
Outdated
|
|
||
| WfWpControl*WfWpControl::copy() | ||
| { | ||
| WfWpControl *copy = new WfWpControl(object, parent); |
There was a problem hiding this comment.
Do not return objects by raw pointers. Either use std::unique_ptr or return by value.
Also why don't you just use the default copy constructor? Ah, because there are callbacks that store a pointer to the object. Then you probably want to make the object non-movable.
src/panel/widgets/wireplumber.cpp
Outdated
| if (g_strcmp0(type, "Audio/Sink") == 0) | ||
| { | ||
| which_box = &(widget->sinks_box); | ||
| control = new WfWpControlDevice(obj, widget); |
There was a problem hiding this comment.
Don't manage memory with new/delete.
Probably objects_to_control should own the objects through std::unique_ptr.
src/panel/widgets/wireplumber.cpp
Outdated
| { | ||
| // adds a new widget to the appropriate section | ||
|
|
||
| WpPipewireObject *obj = (WpPipewireObject*)object; |
src/panel/widgets/wireplumber.cpp
Outdated
|
|
||
| WpPipewireObject *obj = (WpPipewireObject*)object; | ||
|
|
||
| const gchar *type = wp_pipewire_object_get_property(obj, PW_KEY_MEDIA_CLASS); |
src/panel/widgets/wireplumber.cpp
Outdated
| WfOption<std::string> str_wp_right_click_action{"panel/wp_right_click_action"}; | ||
| WfOption<std::string> str_wp_middle_click_action{"panel/wp_middle_click_action"}; | ||
|
|
||
| if ((std::string)str_face_choice == (std::string)"last_change") |
There was a problem hiding this comment.
Don't cast a string constant to std::string for comparison. std::string has operator== with const char *.
Also please use .value() method of WfOption instead casting it to std::string: it looks better and would allow to optimize unnecessary copy if value() would return a reference instead of copy (it currently doesn't, but I think we should fix it).
src/panel/widgets/wireplumber.cpp
Outdated
| reload_config(); | ||
|
|
||
| // boxes hierarchy and labeling | ||
| auto r1 = Gtk::Orientation::HORIZONTAL; |
There was a problem hiding this comment.
Please don't use such aliases. It decreases code readability.
Feel free to do so if you find it better. We already do that for window-list, simply create a subdirectory and place all files in it. |
This reverts commit 81bfaf3.
fixedsegfaults on face = default sink/source try harder to not be faceless actually show face initially properly always refresh face/controls icons
formatting fixed node media classes added proper guard to getting mute and volume
|
@ammen99 things are starting to feel pretty good here ! at least to me. I think it should be ready for testing. Updated the initial message, which should now list everything. |
|
Also, i’ve noticed the wayfire github wiki, under contributing, says to use #pragma once instead of include ifdefs. With all of wf-shell seeming to use ifdefs, is it only a policy for wayfire itself ? |
In the beginning we used ifdefs, nowadays moving towards pragmas :) |
ammen99
left a comment
ammen99
left a comment
There was a problem hiding this comment.
LGTM now, thanks for your excellent work! I tested and the basics work fine. If there are any leftover bugs, we can fix them later. One last typo I mentioned. Other things to consider now:
- Potentially add wireplumber to the default ini file, maybe commented out, but it should be mentioned there. This way, new users can discover your widget.
- If you are up to the task, add some documentation to the wiki as well: https://github.com/WayfireWM/wf-shell/wiki/wf%E2%80%90panel
| <option name="wp_popup_on_change" type="bool"> | ||
| <_short>Pop up on change</_short> | ||
| <_long> | ||
| Wether to show a popup with the new values upon a change to the quick action target |
There was a problem hiding this comment.
| Wether to show a popup with the new values upon a change to the quick action target | |
| Whether to show a popup with the new values upon a change to the quick action target |
|
Thanks ! I’m glad you like it :)
I mean, i’d be honored if it wasn’t :P One last thing i’d really like to figure out is to limit the names widgets sizes, as soon as i figure out how to use contraints (currently, they can stretch out their control if they’re too long). There’s also the linear mode for the mixer api directly that i would like to figure out, as calculating it ourselves rarely leads to the slider recieving some enormous number and being visually to the max, while the volume is actually to 0. Also, i think it would be good to unify and make more descriptive the naming of face/quick target (latter is the naming in metadata), and potentially a more representative one than « controls » ? I’m not sure they are quite understandable when not already knowing the whole features of the widget. Any ideas/oppinions ? Finally, for documenting, should the example config file present the default configuration explicitely, or show another one one could use ? |
Alright, if you want to work more on this before merging, I can wait.
Indeed, the 'face' thing had me confused at first. I think quick target makes more sense. Controls seems ok to me.
I think showcasing the default configuration would be fine. |
3 participants
The widget gives an interface for controlling the volume and muting of sinks, sources and output streams, as well as selection of the default sinks and sources (input/output devices).
By default, upon changes to a volume / mute, it pops up an interactible bubble where volume/mute can be changed again.
Has scroll to change volume and possibility of showing the controls of only a single track at a time, through the « face/quick action target »
Has the following settings :
It’s build is behind a new build option and uses the wireplumber C api.
Code for icon selection and the custom gtk scale have been moved out of the pulseaudio volume widget for re-use.
Also, the compile-time warning in panel.cpp if volume widget is absent has been switched to a notice in meson, with the same behaviour for the wireplumber widget.