-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
FEAT(client): Add possibility to change notification volume #5725
Conversation
@Krzmbrzl Can you take a look at the feedback requests? |
I think these are the options for the
|
Sorry for the late response, but I am currently quite busy 👀
Default values in constructor parameters should be fine
We can add the additional parameter in a new version of the plugin API, ensuring that whenever the old version is used, a sensible default (probably
legacyLoad is only required for settings that existed before the settings format change to ensure that the old settings file can still be read (and thus making sure the settings are not lost). New settings only need to implement the macro-stuff required for the JSON (de)serialization.
Can't we just use the same approach that we also use for volume adjustments, where we simply use a dB scale? See e.g. the For feedback on the UI side, I'll need to find a bit more time than I currently have at hand. |
That's probably what I would do. Auto-applying the notification volume to samples might not make sense as not all samples played by plugins can be (in general) thought of as notifications. However, by exposing the volume setting to the pluign, it can fetch this setting, if it is indeed playing a notification and wants to adhere to the setting. |
7b0fd5c
to
638f256
Compare
@Krzmbrzl I implemented the advanced version of the feature and there is basically just the API change and the UI decision missing.
I understand what you are saying and agree, but I fail to immediately comprehend what actual code changes are necessary for that, because I'm not familiar with this kind of API design. Do I literally just add a default method parameter and call it a day? Or do I create a separate |
@Krzmbrzl I understand you are busy, but could you give me a few lines of text on how you want the API change implemented? I'd like to do the necessary changes myself, but going by trail and error would probably waste everyone's time. |
The necessary changes for the API are
|
c4115de
to
9d0fb19
Compare
@Krzmbrzl Hi, and thanks for the reply. I have implemented your API change directions (i think) in this commit, but there is still a structural problem. The code now defines two structs each for one of the supported API versions (1.0 and 1.2) in the respective Do you want me to create distinctive include guards or are there side effects? What do we do about |
Ah right.
No, no, you did exactly as I instructed. That's just a bit of a flaw in my original design of how these files come together.
Yes, I think this would be the best solution and I currently don't see any potential side-effects of this. The definition of #ifndef EXTERNAL_MUMBLE_PLUGIN_API_NO_MUMBLE_API_T_DEFINE
typedef struct MumbleAPI_v_1_0_x mumble_api_t;
#endif Do this in both of the API header files and then inside |
9d0fb19
to
6e09c17
Compare
@Krzmbrzl Now the build for the current commit fails, because the I think this needs a new structure. I will think about the problem myself, but I also of course await further instructions. |
We need the structs for backwards compatibility, but would it be possible to define the rest of the header files only for the most recent API version (1.2 in this case) including Plugins compiling against whatever the current source code version is would work the same. And legacy plugins don't need any of the other definitions as they are already compiled, right? They just need the backwards compatibility struct. The only thing that would not work is compiling legacy plugins with a more recent code base. So you cant compile a plugin with API 1.0 using mumble source code with API 1.2. Am I missing something? |
Hm. We could wrap those definitions inside some
Yes and no. In principle yes, but the point of explicitly keeping the older headers around is exactly that legacy plugins can still be compiled without requiring changes to their code. So ideally, every of these headers would always work as a stand-alone header when it is the only header that is included but "do the right thing" if multiple headers are included (as in our case). For the macro-side of things we could easily What I could think of would be the following: #ifndef MUMBLE_PLUGIN_API_MAJOR_MACRO
# define MUMBLE_PLUGIN_API_MAJOR_MACRO 1
# define MUMBLE_PLUGIN_API_MINOR_MACRO 0
# define MUMBLE_PLUGIN_API_PATCH_MACRO 0
#endif Ensuring that in such a cascading event the version number of the top-most (most recent) header is used for all definitions. This also reduces the amount of copied code but has the distinct disadvantage that any plugin developer now has to have all of those header files in their include path instead of only the latest. Maybe we could also just guard all of these "convenience definitions" (aka everything except the bare struct definition) via an |
@Krzmbrzl Could we create some sort of Then plugin authors only include the plugin header they need, and the defines would still work. And we include "latest" giving us all the structs but only the most recent defines |
@Krzmbrzl Check out this commit, where I introduced my idea. This now at least compiles correctly. Tell me what you think about this non-recursive design. Edit: Changed some format, comments and naming |
dcfdce1
to
bff9fac
Compare
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 like your latest approach to including the API structs.
I am wondering whether we need the auxiliary definitions at all in our code base though. So why not remove them from all headers by also including the most recent header only after the macro definition that prevents the defines from happening?
bff9fac
to
9f6f9b2
Compare
We are providing "the real" I've pushed a new commit. What do you think? |
No - only when you hit |
@Krzmbrzl Noted. Upon investigating on how to theoretically do this, I have noticed it would require quite some change to Anyway, I am going to squash the commits now. I have successfully tested the functionality and the USE_NO_TTS code path. I want to test compiling and installing a plugin with the new API version. What I can not test right now is TTS volume, but I suspect this still works as before since I only changed the slider position... What do I do about the translations? Run the translation python script and add a new commit? |
9bf3276
to
0d3bcbf
Compare
Exactly. Once the code changes are done, just run the script and it will create a separate commit with the translation update. Then just push both commits. |
A heads up: I messed up the argument order in the API and will push the commit again now |
0d3bcbf
to
3a855d7
Compare
@Krzmbrzl My tests have concluded. The I will await your final review now and then add the translation commit. I would like to add, or suggest to add, documentation for developers in PS: The plugin file caching seems kinda broken in Mumble right now. The file is not removed on "unload" and re-added to the list when you restart the application. I had to manually remove the plugin file between tests. |
Sure, more docs is always welcome. Just add it in a separate
Well unloading is not supposed to remove any files. However, it should remove the plugin from the list of currently loaded plugins and on systems like windows, it should stop the access to that file so that the file can be modified. It's meant for hot-swapping plugins. What you seem to have searched for is an unistall button. At this point that does not yet exist. |
I see. So the workflow on Linux is to just manually replace the file in XDG_SOMETHING and reload/restart Mumble, right? Because that's essentially what I did |
What's XDG_SOMETHING meant to be? The workflow on Linux is to simply overwrite the plugin |
The xdg default config folder on Linux where Mumble stores its settings. I do not remember the exact name off the top of my head. |
@Hartmnt I took the liberty and pushed two further commits to your branch.
If you are okay with 1), then please just squash that into your feature commit, but please just leave 2) as a separate one. |
53441d2
to
c3c2b40
Compare
@Krzmbrzl I added the translation and the documentation |
Previously all notification sounds were played as is, without taking anything into account. The only way to change the volume was to manually edit the sound files, or change the overall volume of the entire Mumble application. This commit adds the ability for the user to adjust the volume of notification sounds and audio cues. There are two new settings added "notificationVolume" and "cueVolume" to adjust the volume independently. Sliders in the "Messages" have been added and together with the existing TTS volume slider make up the new group "Message Volume". A side effect is the centralization of the db <-> factor conversion functions in the "VolumeAdjustment" class. Furthermore, this commit also introduces a change to the "playSample" API call, accepting a volume parameter, and therefore bumps the Mumble API version to 1.2.x. Implements mumble-voip#3963
When using the Qt-provided TTS implementation, the setVolume function call would simply pass our setting for the TTS volume to Qt unchanged. However, while our code uses an integer ranging from 0 to 100 to indicate TTS volume, Qt's docs explicitly state > This property holds the current volume, ranging from 0.0 to 1.0. and thus we are essentially always passing in values that are way too large (which presumably just get truncated to 1.0, effectively breaking being able to adjust the TTS volume). This commit fixes this by making sure to convert our TTS volume into the proper range before passing it to Qt.
Describe necessary steps to implement a new API version for plugins.
c3c2b40
to
41ce5de
Compare
Thank you very much for implementing this 👍 |
Fixes #3963
Here is the background information about my naive implementation:
AudioOutputSample
objects - used for playing audio files, andAudioOutputSpeech
objects - used for playing user speech, both implementAudioOutputUser
.AudioOutputUser
objects are queued inqmOutputs
inAudioOutput.cpp
and mixed in itsmix
method.mix
which tries to cast the queuedAudioOutputUser
objects toAudioOutputSpeech
to determine, if the object is user speech or not. CODEiNotificationVolume
value from the settings and stores it in thevolumeAdjustment
variable already used for local volume adjustments and priority speaker status.My naive implementation has the drawback that you can not independently set the volume of "Audio on/off/mute cues" and regular notification sounds. I want to improve the implementation, but that requires changing a lot more code.
Improved implementation considerations:
AudioOutputSample
.AudioOutputSample
objects are exclusively created in theplaySample
method ofAudioOutput.cpp
. Add an additional option to set the volume when callingplaySample
which then creates anAudioOutputSample
with the corresponding value set.playSample
API changemix
, but forAudioOutputSample
and if it succeeds, setvolumeAdjustment
to the value stored in the sample object.playSample
function and determine whether or not to set the volume option to a specific value or a stored setting such asfNotificationVolume
orfCueVolume
TODO:
playSample
Update Notification&Cue sound volume in real time (as local volume adjustment is done)Log.ui
in theMisc
box. If we implement the ability to change audio cue volume, we will have 3 different sliders for "notification volumes". One for TTS, one for audio cues, and one for actual notifications. Where do we put these sliders in the UI? (P.S.: Why is the Misc section in Log.ui not the last group item?)* Does not change existing UI much
* Volume right next to specific option
* All volume sliders in same tab
* Group of functionally similar options
* Audio cue still in audio input
* Difference between audio cue volume and notification volume easily visible
* TTS slider requires pre-processor macro and has to be turned off if compiled without TTS enabled