-
Notifications
You must be signed in to change notification settings - Fork 43
Added wireplumber mixer widget #307
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
Conversation
|
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. |
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.
Hi, thank for your PR! Please consider fixing the issues.
src/panel/widgets/wireplumber.hpp
Outdated
|
|
||
| static std::vector<WayfireWireplumber*> widgets; | ||
|
|
||
| static void init_wp(); |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use Glib::Variant instead of C glib functions.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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; |
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.
auto *
src/panel/widgets/wireplumber.cpp
Outdated
|
|
||
| WpPipewireObject *obj = (WpPipewireObject*)object; | ||
|
|
||
| const gchar *type = wp_pipewire_object_get_property(obj, PW_KEY_MEDIA_CLASS); |
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.
std::string_view
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
196b586 to
7f49733
Compare
applied to not only the changes here but also in general WayfireWM#307 (comment)
ammen99
left a comment
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.
A few things I noticed. I haven't read all the wireplumber bits in detail, I will test instead once the PR is finalized ;)
src/panel/panel.cpp
Outdated
| #ifdef HAVE_WIREPLUMBER | ||
| return Widget(new WayfireWireplumber()); | ||
| #else | ||
| #warning "Wireplumber not found, mixer widget will not be available." |
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.
I think this warning, if emitted at all, should be emitted from meson.build, not from the source file.
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.
Sure, based myself on the pulseaudio volume widget. Should i change it as well ?
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.
Sounds good!
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.
if emitted at all
Changed it to be just a message, assuming that’s what you’d want.
src/panel/widgets/wireplumber.cpp
Outdated
|
|
||
| /* Setup popover */ | ||
| popover->set_child(master_box); | ||
| popover->get_style_context()->add_class("volume-popover"); |
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.
same here, should be a different name for proper css styling support.
src/panel/widgets/wireplumber.hpp
Outdated
| virtual ~WayfireWireplumber(); | ||
| }; | ||
|
|
||
| namespace WpCommon |
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.
I wonder whether this really has to be in the .hpp file, it is just an implementation detail and used only in wireplumber.cpp, right?
| </option> | ||
| <option name="wp_scroll_sensitivity" type="double"> | ||
| <_short>Scroll sensitivity</_short> | ||
| <default>1</default> |
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.
We could set a value for this (0?) Otherwise, we could allow negative values but then we can remove the invert option.
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.
I’m sorry, i’m not quite sure what you mean ?
As for invert, we could indeed remove it, but i included it because i can’t help but feel it is way nicer as a user to have a toggle rather than making the value negative, at least when using wcm. Should it go ?
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.
I’m sorry, i’m not quite sure what you mean ?
You can set minimum and maximum values for integer options. <min>0</min> for example. I am fine with either retaining both options and setting the minimum to 0, or merging them together (in which case minimum makes no sense). Pick whichever one you like more.
src/panel/widgets/wireplumber.cpp
Outdated
| #include "volume-level.hpp" | ||
| #include "wireplumber.hpp" | ||
|
|
||
| namespace WpCommon |
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.
This one seems like it should be a singleton class.
src/panel/widgets/wireplumber.cpp
Outdated
| * We re-use the core, manager and all other objects | ||
| */ | ||
|
|
||
| WpCommon::widgets.push_back(this); |
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.
Once you have a singleton class for the WpCommon stuff, you can move all this code in a method there (add_widget / remove_widget)
src/panel/widgets/wireplumber.cpp
Outdated
| "Audio/Source", | ||
| }; | ||
|
|
||
| static VolumeLevel volume_icon_for(double volume) |
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.
This is the same code as in volume.cpp, I think we can avoid duplication here. pa_volume_t is a uint32_t, so we can simply use double for the type everywhere.
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.
i’ve actually been writing a display brightness control widget that ends up having roughly the same thing again. Should i preemptively make it even more generic to account for it (or other stuff that might have be similar), and if it’s done soon enough, should i add it to this pr or make another one ?
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.
Better leave it for later, lets finish this PR first and then worry about the next one.
src/panel/widgets/wireplumber.cpp
Outdated
| void WayfireWireplumber::reload_config() | ||
| { | ||
| // big matching operation | ||
| WfOption<std::string> str_face_choice{"panel/wp_face_choice"}; |
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.
These can be marked as static to avoid reloading them every time. Their values will change automatically when the config changes.
|
@AKArien I see you have already done most of the changes, when you are ready with the rest, let me know to test this PR and hopefully merge afterwards. |
|
@ammen99 i think i have addressed everything pointed out here, but i have a few little ui bugs to fix. There’s also an issue that i really don’t understand and would like you to keep an eye on in your testing / review : sometimes, under circumstances i cannot nail down, the tray or the menu widgets will segfault, with gdb showing a backtrace in the Watcher destructor for the tray and the toggle_menu method for menu. This never happens without wireplumber in my config, and removing it is always an immediate fix. My widgets are usually like this : I hardly see how this could be related to my widget, but it seems i’m missing something ? Since removing wireplumber always stops it from happening. Again, i’m sorry if this has very little detail, but it is litterally all that i could find to be consistent. |
|
@AKArien I would strongly recommend compiling wf-shell with address sanitizer in this case. It is likely that you see the result of a memory corruption somewhere and address sanitizer very often catches issues like these. |
|
Also these are probably the UI things you mentioned need improvement, but: when I start the panel, the widget icon is empty and I need to change the volume once for an icon to show. Probably should choose a default face at startup? Also the button should have the flat style to match the rest of the panel's widgets. Look at the menu widget, it sets the flat class twice because the GtkMenuButton used here is special. |
Yup, i had added something that would make the widget try to have a face, but it seems to not work. For the style, will do. |
|
@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
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.
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
|
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. |
|
@ammen99 This time should be final. Hopefully. Finally, i was wondering if you would like this widget to replace volume in the default configuration ? |
name change in metadata added spacing to boxes
b38560a to
27b203f
Compare
Would be best to first let the people test it for a while :) Otherwise I agree that this widget is in pretty much all ways superior to the old widget. |
|
Alright then ! Glad you like it :) I’ll fix the default, and then probably wait a few weeks before i’m 100% absolutely sure i don’t have anything to add or fix anymore. |
|
I think we are ready to merge, provided you fix the conflicts that GitHub seems to see in this PR .. |
|
Excuse me, but i'm really confused ? Github shows me that there's no conflicts, i rebased on latest master earlier today |
|
Thanks! |
* Added wireplumber mixer widget * Added scroll control in full mixer * Added middle click to mute * add configuration options for wireplumber widget * uncrustify and style compliance * uncrustify ? * remove section of the code pending on pr 306 (extra options) * uncrustify ; though i cannot agree oops lol * Revert "remove section of the code pending on pr 306 (extra options)" This reverts commit 81bfaf3. * remove logic pertaining to pr 306 (vertical panel layouts) * fixed include in animated-scale.cpp * add popup on change option * added support for mute gestures to individual control * added defaults, fallbacks and corrected compliance with config options * fix scroll gesture, malformed panel.xml * cleaned up a bit middle click to mute * fixed improper naming of on -> handle_config_reload * removed obsolete functions * fixed left click actions on widget ; comments * added deselection guard for default + explanation * adjust logic of updates in on_mixer_changed ; removed unused enum ; comments * cleaup * fix popover jank and incorrect settings reloading * uncrustify * fixed gestures connection both in control and widget ; cleanup * lil forgotten * uncrustify * fixed doubling in existing widgets when catching up new one * de-duplicated icon_name_from_state * removed non breaking space and french quotes from comments * use std::string_view for string comparison * fixed configuration to use .value() missed it oops * unique instead of raw pointers * separators and labels not by new * add autos, make construction less barbaric * cleanup * exported volumelevel and icons to be common between volume and wireplumber * fixed broken setting of default node * added explanation to volume-level.hpp * fix indentation in util meson * uncrustify * fix headers * removed stuff from old pulseaudio volume code * added and cleaned up comments, declarations and small moves * separate wireplumber styles from volume and add size option * uncrustify * now uses a singleton, split into multiple files * fix face choice for default sink/source, static for config * remove warningns for volume and wireplumber + ipc header * minimum value of 0 for wireplumber scroll sensitifivy * improved icons handling and made it in common between volume and wp * added volume-level to build and build messages for volume/wp being unavailable * uncrustify * use c++ glib instead of C glib functions * fixed on_mixer_changed logic * fixed popups and ignore control that just did the change * add uniform styles * cancel hiding of the popover after user interaction * fixed scrolling fixedsegfaults on face = default sink/source try harder to not be faceless actually show face initially properly always refresh face/controls icons * uncrustify * put more of setting default to wpcommon formatting fixed node media classes added proper guard to getting mute and volume * oh come on what was i thinking * uncrustify sigh * fixed setting of face for face = default device * drop overloaded WfWpControlDevice copy to get a normal control for face * maybe i should think of running uncrustify myself * how long have the labels been blank oops lol * goofed up * fixed initialisation with default sink/source as face * fixed crashes without face / now shows icon when faceless * uncrustify * uncrustify * added slider length option * don’t repeat getting volume and mute * still don’t like it but if now if it fails it’s probaly less bad * don’t take cube root there, since it’s already done * forgot negation * restored mute oops * switch the priority of names * fix <long> formatting * fixed typo * face -> quick target everywhere * add wireplumber to example * switch naming from WayfireWireplumber to WpMixer * changed name for user-facing documents and added a quick explanation of pipewire and pulseaudio to example ini + moved the quick target selection to the top of config sections * word change + uncrustify * updated meson paths and includes for rename * made the names not drive up the size of the grid name change in metadata added spacing to boxes * add tooltips to main widget for current quick target * change icon for oor volume to emblem-unreadable * added option to put icosn on left * uncrustify + update some comments * make WfOptions read in member functions static * added margins and use helper lambda * fix wall of critical warnings * fix label attaching and clean up update_gestures + uncrustify * clean up gestures initialisation * add wireplumber-dev package to the build ci * prevent external changes from replacing the full mixer view * prevent new control appearance from replacing full mixer view * uncrustify * fixed memory leak on reload * change default target to default_sink and slight ini changes * adjust naming to popup * make spacing a proper config option instead of hacking it and make it properly reload * also update the ini ex
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 library.
Code for icon selection and the custom gtk scale have been moved out of the pulseaudio volume widget for re-use.
This PR slightly alters the volume widget, by changing the « volume out of range » icon the « emblem-unreadable » icon instead of « audio-volume-muted »
Additionally, the compile-time warning that was emitted if libpulse was not found and the volume widget not built has been moved to a meson notice.