Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Allow disabling of TTS for specific users #4287

Merged
merged 2 commits into from
Jun 17, 2020
Merged

Allow disabling of TTS for specific users #4287

merged 2 commits into from
Jun 17, 2020

Conversation

Popkornium18
Copy link
Contributor

These changes make it possible to disable Text-To-Speech for specific
users while still receiving the messages as normal text.

A use case for this would be muting text to speech for a music bot.

Krzmbrzl
Krzmbrzl reviewed Jun 14, 2020
View reviewed changes
src/mumble/Database.cpp Show resolved Hide resolved
src/mumble/Database.cpp Show resolved Hide resolved
src/mumble/MainWindow.cpp Show resolved Hide resolved
@Krzmbrzl Krzmbrzl added client feature-request This issue or PR deals with a new feature labels Jun 14, 2020
@streaps
Copy link

streaps commented Jun 15, 2020

a screenshot would be helpful

@Popkornium18
Copy link
Contributor Author

image

Krzmbrzl
Krzmbrzl reviewed Jun 17, 2020
View reviewed changes
Copy link
Member

@Krzmbrzl Krzmbrzl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from that signal-thingy, this LGTM.

Please have a look at our commit guidelines though: https://github.com/mumble-voip/mumble/blob/master/COMMIT_GUIDELINES.md
If possible, it'd be nice if you could adapt your commits accoridngly (squash everything but the translation update into a single FEAT(tts) commit.

You don't have to re-formulate the translation update commit though ☝️

src/mumble/ClientUser.cpp Outdated Show resolved Hide resolved
@Popkornium18
Copy link
Contributor Author

Please have a look at our commit guidelines though: https://github.com/mumble-voip/mumble/blob/master/COMMIT_GUIDELINES.md
If possible, it'd be nice if you could adapt your commits accoridngly (squash everything but the translation update into a single FEAT(tts) commit.

Ah shiet I used FEAT(client), but that shouldn't be a problem, right? Also there are 2 new LOC, since I somehow missed to read the values from the DB, causing the checkbox to be always unchecked. (Messages.cpp:507)

@Popkornium18 @Krzmbrzl
FEAT(tts): Allow disabling Text-To-Speech per user
1a4d134 
These changes make it possible to turn of TTS for a specific user while
still receiving their messages.

This setting can be managed from the context menu of a user that is
connected to the server and is only visible when TTS is enabled
globally.

A new DB table called 'ignored_tts' had to be created to persist this
setting.
@Popkornium18 @Krzmbrzl
Translation update
06e558a 
Updating 'mumble_en.ts'...
    Found 1873 source text(s) (4 new and 1869 already existing)
@Krzmbrzl
Copy link
Member

I changed the commit message to be FEAT(tts). It's more specific than FEAT(client) :)

@Krzmbrzl Krzmbrzl merged commit cd60ea5 into mumble-voip:master Jun 17, 2020
@Krzmbrzl
Copy link
Member

Thank you for your contribution :)

@Popkornium18 Popkornium18 deleted the disable_tts_per_user branch June 17, 2020 19:36
@Krzmbrzl Krzmbrzl mentioned this pull request Jun 20, 2020
Merged
@Krzmbrzl
Copy link
Member

Just FYI: We have forgotten to check for null at some point - see #4307 :)

Krzmbrzl added a commit that referenced this pull request Jun 20, 2020
@Krzmbrzl
Merge pull request #4307: FIX(text-chat): Fix crash on TextMessage fr…
94ed179 
…om server

1a4d134 (part of #4287) introduced the ability to
disable TTS for specific users. The change however didn't account for
messages that are sent from the server. In these cases pSrc is nullptr
but that commit introduced code that access it without checking for
null. This lead to a SegFault as soon as a text message from the server
is received.

This commit fixes this by simply adding a null-check before accessing
the variable.
@rhetr
Copy link

rhetr commented Aug 21, 2021

how do i use this feature? i don't see it in release 1.3.4 or in the latest 1.4.0 development snapshot.

@Krzmbrzl
Copy link
Member

That it is not in the 1.3.x series is expected (we don't backport new features) but it should be present in the 1.4.0 snapshots.
In fact this was included since the very first snapshot (see changelog at https://www.mumble.info/blog/first-mumble-1.4.0-development-snapshot/). Are you sure you are in fact starting the snapshot and not the 1.3.4 version?

@rhetr
Copy link

rhetr commented Sep 17, 2021 •
edited
Loading

Screen Shot 2021-09-16 at 8 25 47 PM

it says i'm using the development snapshot. i've tried on both windows and macos.

@Krzmbrzl
Copy link
Member

The button will only show if you actually have TTS enabled in the first place. Is that the case?

mumble/src/mumble/MainWindow.cpp

Lines 1568 to 1569 in 937f4e2

if (Global::get().s.bTTS)
qmUser->addAction(qaUserLocalIgnoreTTS);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Krzmbrzl Krzmbrzl Krzmbrzl approved these changes

Assignees
No one assigned
Labels
client feature-request This issue or PR deals with a new feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Popkornium18 @streaps @Krzmbrzl @rhetr