mumble-voip / mumble

Mumble is an open-source, low-latency, high quality voice chat software.
https://www.mumble.info
Other
6.26k stars 1.1k forks source link

Add option to change background color in TalkingUI #5411

Open terenc3 opened 2 years ago

terenc3 commented 2 years ago

Context

Using the TalkingUi as window source in OBS to show viewers who is in your channel and who is speaking. Similar to Discord StreamKit Voice Widget (https://streamkit.discord.com/overlay).

Description

Add option to change the background color to make it easy to apply a chroma key filter for background removal.

Furthermore it could be useful to hide the channel name completely or hide the small border around the channel, but this can be archived through cropping in OBS too.

mumble_talkingUi_obs

Mumble component

Client

OS-specific?

No

Additional information

No response

Krzmbrzl commented 2 years ago

Implementing an option to choose a custom color for the background should be relatively easy. If someone wants to give this a try, ping me and I can give you an outline of what changes are likely necessary.

Removing the channel name on the other hand is not as that would probably require rewriting large parts of the TalkingUI code.

mhayworth commented 2 years ago

@Krzmbrzl I'm an undergraduate student looking to contribute to an open source project as part of my Intro to Software Engineering Class. I'm interesting in working on this issue. Would you be willing to help me solve this as my first contribution to an OSS?

Krzmbrzl commented 2 years ago

Would you be willing to help me solve this as my first contribution to an OSS?

As already stated on Matrix: I will gladly guide you through the process. In fact I will use this as a general motivation to write up a bit of documentation on the topic of how to approach the Mumble source code... More specific help will follow after that :point_up:

Krzmbrzl commented 2 years ago

@mhayworth the docs are available at https://github.com/mumble-voip/mumble/pull/5500

From there, I would recommend you trying to try to grasp the general concept of the TalkingUI by yourself (you said you wanted to learn that). As a hint: I like to trace down functionality from the "userspace" aka the UI to its actual implementation. For this specific example, (since the general TalkingUI source files are obvious to find - so no need to first search them in general) I would start by searching for the already existing TalkingUI settings and then tracking down where these are actually set and where they are applied.

If you have more questions or need more help, feel free to ask either here or on Matrix.

mhayworth commented 2 years ago

Screenshot 2022-01-21 155645 I'm not sure if I'm missing something, but it seems that changing the background color is already possible with the existing settings, in this picture you can see I've changed the TalkingUI Overlay color to pink

EDIT: I think I was confusing the TalkingUI with the in-game overlay, disregard this.

Krzmbrzl commented 2 years ago

Yeah - these are two different things :)

mhayworth commented 2 years ago

Screenshot 2022-01-21 160816 Just to confirm, this is the TalkingUI, right?

Krzmbrzl commented 2 years ago

No - that's the main UI. You can activate the Talking UI from the same dropdown menu that you also use to open the settings (I think it's called Configure)

mhayworth commented 2 years ago

Thanks, I believe I found where the color is determined (please correct me if I'm wrong): TalkingUIEntry.cpp, line 101: m_backgroundWidget->setAutoFillBackground(true); I looked on the Qt documentation for background color (https://wiki.qt.io/How_to_Change_the_Background_Color_of_QWidget), and it says I could add some code using a color palette after this line to change the color of the widget, is this much correct? Thanks

Krzmbrzl commented 2 years ago

Yes that sounds about right. However, using the color palette won't work because Qt is really silly in that regard. Since we are using stylesheets in our application, colors can only be set via stylesheets as well. The color palette won't work.

You will probably want to use a MultiStyleWrapper for performing this task (the class is defined in the widgets subdir iirc). This class works around the issue that setting the stylesheet on a given widget overwrites the previous one, even if a different property is set.

mhayworth commented 2 years ago

gotcha. There already is a MultiStyleWidgetWrapper called m_backgroundWidgetStyleWrapper in TalkingUIEntry.h, how would I modify this to change the background color?

Krzmbrzl commented 2 years ago

Iirc there should be a member function for changing the background color already. If so, just use that. Otherwise you'd have to extend the class to provide such a function. I think the working principle of it should get clear by looking at the current implementation, but if not feel free to ask.

mhayworth commented 2 years ago

Hey there, whats the difference between the setBackgroundColor and setBackgroundColorSelector functions of the MultiStyleWidgetWrapper class, and which would be more applicable for setting the background of the TalkingUI?

mhayworth commented 2 years ago

and I guess this comment also ties into how we want the user to choose a background color. It seems like a selector is the best option, no? what would be the process for making the color selector pop-up window? Thanks again for all the help

Krzmbrzl commented 2 years ago

I think you are misunderstanding what a "selector" in this case means. That's a term used in the context of stylesheets - maybe have a look at e.g. https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Selectors

So to answer the question: The former is what you want to use. Maybe you have to use a selector in order to prevent the background color to get inherited throughout the entire children of the TalkingUI - not quite sure atm :thinking:

In order for the user to pick a color in the settings, we should probably use https://doc.qt.io/qt-5/qcolordialog.html

mhayworth commented 2 years ago

Okay, I've created a list of what I've done and what I think I need help with. I have:

  1. Added a member function setBackgroundColor to the talkingUIEntry interface and the classes that inherit it
  2. Added functionality to update the background color when the settings change in the on_settingsChanged function from talkingUI.cpp
  3. Added and initialized variable qsTalkingUI_BackgroundColor to #000000 in settings.cpp What I need to do: -Add functionality to open color dialog in Mumble's configure options -update the settings to that changed color

How should I go about this?

Krzmbrzl commented 2 years ago

@mhayworth it might be easiest if you were to create a draft PR at this point, so I could immediately see the exact changes and we could work with that.

With regards to your points though:

  1. Did you try setting the background color on the TalkingUI class itself rather than on its entries? I would have imagined that it should be possible to change the background color on that and then let it inherit through the chain :thinking:
  2. :ok:
  3. From the naming scheme it seems like you chose to use a QString to represent a color. I would advice to use QColor instead
  4. You'll have to edit the ui file that contains the TalkingUI settings and add a new entry in there. I would go with a label + button combination where the button simply displays the currently set color. Then in the corresponding cpp file, add an event handler for when the user clicks on that button and in there open the QColorDialog and init it to the currently set color. Saving happens also in the corresponding cpp file (there is a save function in there - just extend it to also save the color preference)
mhayworth commented 2 years ago

I read through your most recent reply and am taking those into consideration. I figured I should just do my pr as-is so you can evaluate what i've done so far. I mentioned this in the pr message, but I chose a QString because the MultiStyleWidgetWrapper background color function takes a QString param, though I could have TalkingUIEntry's functions accept a QColor and use QColor::name(). As for setting the background color in the TalkingUIClass itself, it doesn't have a MultiStyleWidgetWrapper declared, that is actually declared inividually in each of the derived classes.

amberhandal commented 2 years ago

I was wondering if I could also give this a shot?

Krzmbrzl commented 2 years ago

@amberhandal there's already a PR for this: #5526 Thus, there is no need to have someone work on this as well :eyes:

We have a lot of other good issues for first-time contributors: https://github.com/mumble-voip/mumble/labels/good%20first%20issue

terenc3 commented 1 year ago

Since there was no progress here, i did this myself but concluded that another background color did not enhance the end result. A browser page which can be included in obs is the better way for me.

Bildschirmfoto zu 2023-04-18 20-40-13

changeBackgroundColor.patch

My version did not properly persist the setting when closing mumble and also the wrong color is returned when the picker is canceled.