FEAT(client): Add User and Listener volume slider to context menus

[fgilcc]

As requested in #4748 this commit implements a volume control slider for User local volume and Listener local volume in the relevant context menus for easier reach and quicker adjustments.

The VolumeSliderWidgetAction was implemented as a base class for UserLocalVolumeSlider and ListenerLocalVolumeSlider which were then added as QActions in the MainWindow to the context menus qmUser and qmListener respectively.

I've left both dialogs as they act as a label for the slider in the menu and as a more robust option for volume control. The tool tip appears when moving the #slider. I have not tested the Listener menu. I can't find how to reach it.

Implements #4748

[Krzmbrzl]

I have not tested the Listener menu. I can't find how to reach it.

In order to reach that you have to add a listener to a channel and then right-click on it. Note however that you can only add a listener (via a channel's context menu) on servers that are running at least Mumble 1.4.0 (aka a development snapshot). If you host the server you have built locally, that should be easily possible. I can also test that for you, if you prefer though.

[fgilcc]

I have tested the Listener and it seems to be working as expected. Thanks for the comments, I'll try to address the issues.

[fgilcc]

I believe I've dealt with most of the issues.

1. ListenerLocalVolumeSlider: Now that a new action is no longer created every time a menu is requested, it just updates the target channel or user, I can't find a way to make channel work with references. onSliderValueChanged can be called without initializing Channel if prepareLocalListener is not called first. With pointers I can at least initialize it and account for nullptr. I have mostly programmed in C so I'm not too acquainted with references. The same problem with clientSession on UserLocalVolumeSlider. Is there a client session ID and channel I can target when first initializing the Action objects? Should I insert a mechanism to make sure the "prepare" methods were called? in onSliderValueChanged. Can I just put a comment that prepareLocalListener must be called before adding the action to a Menu.

#include "UserLocalVolumeSlider.h"
#include "ListenerLocalVolumeSlider.h"

I've left both these includes in MainWindow.h as I've also put UserLocalVolumeSlider *qaUserLocalVolumeSlider; and ListenerLocalVolumeSlider *qaListenerLocalVolumeSlider; next to some other widgets/actions in the same file.

Thank you for your time.

[Krzmbrzl]

1. Yes indeed. References can't be re-assigned. Thus when using the switchable approach, we have to indeed fallback to using pointers.

Is there a client session ID and channel I can target when first initializing the Action objects?

No there are no special IDs that could be used for that. I think we should probably introduce a boolean flag that indicates whether that function has been called before or not.

Should I insert a mechanism to make sure the "prepare" methods were called? in onSliderValueChanged.

That'd probably be best, yes. If it is explicitly accounted for in code then there is no way it can be forgotten in the future. We could either add an assertion for this or explicitly call that function if it has not been called before. Given that the method not having been called seems like a program error though, I would tend towards the assertion :thinking:

I've left both these includes in MainWindow.h as I've also put UserLocalVolumeSlider *qaUserLocalVolumeSlider; and ListenerLocalVolumeSlider *qaListenerLocalVolumeSlider; next to some other widgets/actions in the same file.

As long as you are only using these types as pointers, you can easily forward-declare them in the header and then only include the headers in the cpp file. That keeps the header file as small as possible which should help maintaining a good compilation speed.

[fgilcc]

Why use a reference instead of the ID? I assume that in most places when calling this function the caller already has a reference to the respective user object, thus they might as well pass it into this function, saving us the extra call to ClientUser::get which has to do locking and the actual lookup. Plus we avoid the possibility of letting the caller set an invalid/wrong session ID.

I went trough why I can't use a const reference in my reply to this but I've been playing around with some solutions and I think I understand why the UserLocalVolumeDialog chose to store the sessionId instead of passing references or pointers.

If the user disconnects while you're changing the volume, the references or pointers eventually become invalid, crashing the program if you try to change the volume by calling ClientUser::setLocalVolumeAdjustment(float adjustment). By storing the sessionId and just looking up every time the volume changes, you can still crash the program but the likelihood of doing so becomes extremely insignificant as the pointer has to become invalid exactly between making the lookup and the volume change itself.

I think this could only be completely fixed by implementing ClientUser::setLocalVolumeAdjustment(unsigned int uiSession, float adjustment), and use the lock to make sure the user still exists while the volume is being changed.

I can do it for this pull request. Just wanted to make sure if this makes sense and it is OK.

[Krzmbrzl]

I think this could only be completely fixed by implementing ClientUser::setLocalVolumeAdjustment(unsigned int uiSession, float adjustment), and use the lock to make sure the user still exists while the volume is being changed.

I can do it for this pull request. Just wanted to make sure if this makes sense and it is OK.

Sounds good, yeah. It should only be done in a separate commit. I think this commit should use the REFAC type :thinking:

[fgilcc]

Sounds good, yeah. It should only be done in a separate commit. I think this commit should use the REFAC type 🤔

After researching a bit more, getting the pointer at the start of the method seems to be safe enough. I've reverted it to how it is done in the VolumeDialogs by storing the ID and just looking up the pointer when needed to guarantee it is valid.

I could probably also lookup just once on setUser and store it in a QPointer.

[Krzmbrzl]

Before doing that though I want to bring something else up: With these changes the slider and the preceding Local Volume Adjustments... text are independent entries, right? So when hovering over them, only one of them is selected at a time. I am wondering whether that might be confusing for users as from that it'd seem as if the slider and the original actin don't have anything in common. I think it might be better to remove the old action and instead add a label above the slider. And if the action is clicked outside of the slider, then open the original dialog. If the user however clicks on the slider, use the slider as intended. This would mean a bit more work. but I think it would make for a better user experience.

What do you think?

[fgilcc]

Before doing that though I want to bring something else up: With these changes the slider and the preceding Local Volume Adjustments... text are independent entries, right? So when hovering over them, only one of them is selected at a time.

Your comment prompted me to look at the hovering behaviour with more attention and I noticed it is quite...crap.

In these prints I was either clicking or hovering the slider. It seems QMenu hover doesn't play well with QWidgetActions. In the first example I approached the slider from the top and in the second example, from the bottom. It seems possible to remove selection from the highlighted QAction by adding an event filter to the menus but I need to look further into this.

I think it might be better to remove the old action and instead add a label above the slider. And if the action is clicked outside of the slider, then open the original dialog. If the user however clicks on the slider, use the slider as intended.

I tried this quickly on my initial attempt with a Label and Slider inside a grid but it looked really bad with crappy proportions and I couldn't make it look good. I might spend some time exploring this option.

[Krzmbrzl]

I think the issue seems to be that a) You have to manually implement highlighting the action and b) The action that was highlighted before won't get un-highlighted. See https://bugreports.qt.io/browse/QTBUG-10605.

Manually highlighting seems feasible but the bug causing the old action to not get un-highlighted seems to be a major dealbreaker :thinking:

I tried this quickly on my initial attempt with a Label and Slider inside a grid but it looked really bad with crappy proportions and I couldn't make it look good. I might spend some time exploring this option.

I think I would suggest a RowLayout instead. Maybe that helps resolving the issue :shrug:

[fgilcc]

Manually highlighting seems feasible but the bug causing the old action to not get un-highlighted seems to be a major dealbreaker 🤔

enabling mouse tracking on the Slider widget made it remove highlight from the previous actions properly.

I think I would suggest a RowLayout instead. Maybe that helps resolving the issue 🤷

The label doesn't respect the padding in QMenu.

added 17px margin on the label.

I can make the QWidgetAction highlight-able by changing the stylesheet on hover, but I don't know how to make it look like the highlight in QMenu.

[Krzmbrzl]

enabling mouse tracking on the Slider widget made it remove highlight from the previous actions properly.

:+1:

added 17px margin on the label.

I am rather worried that hard-coding the padding for this will end up working on a particular system only... The interesting question is: Why on earth is the padding respected for the slider but not the label? How do you add these components? Have you tried creating a QWidget that acts as a container for the label and the slider?

I can make the QWidgetAction highlight-able by changing the stylesheet on hover, but I don't know how to make it look like the highlight in QMenu.

I did not yet find a good way of getting these default colors from Qt. In theory this is what QPalette is for. However it seems that as soon as stylesheets are in the game (which is the case for us) the colors obtained via QPalette are garbage. Nonetheless you could try to get the palette of the widget and then extract the highlight() color and see what that yields...

[fgilcc]

I tried looking into extracting the highlight data but I couldn't find a decent solution, I might not be proficient enough for this. The QPalettedoesn't help as expected due to the stylesheets. Even if I managed to extract it, I experimented with the colors in the stylesheet and I couldn't make the combo of label + slider highlighted at the same time look good. changing the background on the slider looks meh.

Why on earth is the padding respected for the slider but not the label?

To be fair, I don't think the slider respects the padding either, it just has bigger margins by default and it looks ok.

How do you add these components? Have you tried creating a QWidget that acts as a container for the label and the slider?

That's what I did with a QGridLayout. RowLayoutseems to be just GridLayoutbut limited to one row if I've read the documentation properly.

I am rather worried that hard-coding the padding for this will end up working on a particular system only...

I also don't think this a good solution it was just for reference.

I think the sanest thing to do is leave it as it is, just with the added mouse tracking fix. I believe it's plenty clear as it is.

Or we could add a separator: Or 2:

[Krzmbrzl]

Does clicking the label instead of the slider do anything? I was thinking that this would open the traditional volume dialog. As such I think not highlighting the entry is a bit weird.

Interestingly the label aligns with the labels of the actions listed below. I did not notice that before :thinking:

In general I think it might be best to change the label to Volume and move the entire thing to the bottom of the menu (with a separator on top). That way potential misalignment does not look that out-of-place, I think :eyes: And in case clicking the label brings up the traditional dialog, I think we should drop that. Just remove the old dialogs completely as I think they don't really provide anything that the slider does not (except a reset button maybe, but then again resetting the slider manually should not be that hard)

[fgilcc]

Interestingly the label aligns with the labels of the actions listed below. I did not notice that before

oops, sorry for the confusion. In the last prints I had reverted to before adding the label to the QWidgetAction. the local user volume adjustment... in those is just the normal QAction. here's a print with the different margins lined up.

gray -> Checkable QAction blue -> QAction red -> Label inside a QWidgetAction

In general I think it might be best to change the label to Volume and move the entire thing to the bottom of the menu (with a separator on top)

If we take discord as the inspiration for this change. They also have volume in the middle with two separators:

In Discords case the volume label is not highlight-able and serves only to label the slider with no dialog when clicked. This is how a QWidgetAction with a QWidget container having both a Label and a Slider would behave. The only problem is lining up the label which I don't know how to go about it.

[Krzmbrzl]

oops, sorry for the confusion. In the last prints I had reverted to before adding the label to the QWidgetAction. the local user volume adjustment... in those is just the normal QAction. here's a print with the different margins lined up.

Ah okay :bulb:

In Discords case the volume label is not highlight-able and serves only to label the slider with no dialog when clicked.

Yes I think that'd be fine for us as well

The only problem is lining up the label which I don't know how to go about it.

Have you tried what happens if you leave the slider away and only add the label to the action widget?

[fgilcc]

Sorry for only responding now but I've been away a couple of days.

Have you tried what happens if you leave the slider away and only add the label to the action widget?

Yes. It still looks bad. It has less margin than with the gridlayout.

[Krzmbrzl]

Have you tried asking about this on StackOverflow yet? There has to be a way to get this to work...

[fgilcc]

I think the best compromise from what I've tried is to add an entry into the stylesheet files for a QLabel that can be inserted into a Widget Action and be part of a menu. This allows us to customize this label and not have to hardcode in the src files things like margin and hover behaviour. An example for lite.qss and how it looks.

I haven't pushed anything because I'm not sure how this project handles stylesheets and if they can be manually edited. I also wouldn't be able to verify how it looks in linux and mac.

It would probably be best to to just push this version for this pull request, image below, with just the slider as a widget action and keeping the volume dialog QAction as a label. It's clear and works as expected. Messing with QLabels and Stylesheets could be another pull request.

I also tried to run scripts/updatetranslations.sh but I couldn't find it so I ran scripts/updatetranslations.py with no success

Sorry for taking so long to respond but I have been busy. Thank you for your time.

[fgilcc]

Ok. after reverting the ts files that I have no idea how they got modified the script gets stuck in this loop.

[Krzmbrzl]

Have you tried asking about this on StackOverflow yet?

Did you?

Indenting by StyleSheet has almost the same drawbacks as indenting by a fixed amount, so I'm not really sure we'd win anything by doing that :thinking:

It would probably be best to to just push this version for this pull request, image below, with just the slider as a widget action and keeping the volume dialog QAction as a label. It's clear and works as expected. Messing with QLabels and Stylesheets could be another pull request.

Yes that is probably correct :+1:

I also tried to run scripts/updatetranslations.sh but I couldn't find it so I ran scripts/updatetranslations.py with no success

Yes the Bash script got replaced by a more portable Python script just recently.

Ok. after reverting the ts files that I have no idea how they got modified the script gets stuck in this loop.

The TS files get modified when you build the project and did not specify -Dtranslations=OFF when invoking cmake. It's pretty annoying but so far we have not found a better way of handling translations during the build :shrug:

As for the loop: You apparently ran into the same issue as I did in #3743. This means that I finally have to go and fix this damn script xD

[Krzmbrzl]

The script should be fixed in https://github.com/mumble-voip/mumble/pull/4904

[fgilcc]

Did you?

I ended up not asking. I found similar enough issues that it felt like I wouldn't be able to get an answer good enough to justify the question.

Indenting by StyleSheet has almost the same drawbacks as indenting by a fixed amount, so I'm not really sure we'd win anything by doing that 🤔

The stylesheets are already full of stuff like indentation for almost every qt class, including the QMenu::item, for QAction styling and even styles for some specific objects. Would this be any different?

[Krzmbrzl]

Oh okay I did not know that :eyes:

Would this be any different?

No indeed it would not.

Before doing that though, let me check Qt's source code to see what they are doing there. Given that these entries can be checkable my current suspicion is that the extra padding stems from an invisible checkbox that is placed to the left of all labels...

[Krzmbrzl]

Having looked into this somemore I am about 90% sure that what is taking up the space to the left of labels in regular actions is a QIcon that can optionally be shown for an action. I can't seem to find a way to query its width nor am I currently able to find the corresponding source code in which menu entries for regular QActions are rendered.

I still think that this should be asked on SO, so if you don't want to do it, please give me a minimal example and I'll go ask myself :point_up:

[Krzmbrzl]

@fgilcc I think I have found a promising candidate for figuring out how much the widgets should be indented: https://doc.qt.io/qt-5/qstyle.html#pixelMetric - you/we presumably only have to figure out the suitable type of pixel metric that we are interested in.

[Krzmbrzl]

Closing this as abandoned

[Krzmbrzl]

I will have to re-check this PR in the light of #5771

[Hartmnt]

Now that I have worked on context menus and local volume adjustment, I would like to help with this. This will obviously need a rebase with merge conflict due to the changes in LocalVolumeAdjustment.

[Krzmbrzl]

Awesome! :+1: Let me know, if you need anything