FEAT(client): Message Thresholding #4808

Skewjo · 2021-02-28T16:19:43Z

Checks

My commits follow the commit guidelines

This PR adds a new column named "Threshold" to the "Messages" settings tab to allow users to turn off message notifications when the server is above a certain number of users:

src/mumble/Log.cpp

src/mumble/Log.ui

Skewjo · 2021-03-01T12:36:52Z

Also, I haven't tested this functionality in any way, other than building & opening mumble to make sure the UI looked somewhat correct.

Is there a way I can mock a bunch of users on a server?
- If not, is there a well-known server I can jump into to check it?
Do I need to write any unit tests for this functionality?

Krzmbrzl · 2021-03-01T12:53:05Z

Is there a way I can mock a bunch of users on a server?

You can use something like Botamusique to create a bunch of bots and let them join a locally hosted server

If not, is there a well-known server I can jump into to check it?

If you set the threshold low enough and @Natenom allows it, you could use his server (I'll not give out any server details before he agreed though)

Do I need to write any unit tests for this functionality?

As it stands we barely have any tests in the first place, so I don't see an easy way of adding test cases for this feature. We want to change that eventually but for now: no you don't have to

Skewjo · 2021-03-01T13:07:08Z

Also, here's what the new elements currently look like:

Krzmbrzl · 2021-03-01T14:21:24Z

I think it'd look better if the threshold column was moved one position to the left so that the Path column remains the last column. That way all checkboxes would stay grouped together ☝️

Kissaki · 2021-03-07T16:03:31Z

Can you please include a description of the (overall) changes and context in the PR description? This may be what is also written in an individual commit message. It should be enough context to get an overview for unfamiliar readers, and will land in the merge commit which again should be easy to digest to know what changed and why.

Skewjo · 2021-03-07T16:16:50Z

Can you please include a description of the (overall) changes and context in the PR description? This may be what is also written in an individual commit message. It should be enough context to get an overview for unfamiliar readers, and will land in the merge commit which again should be easy to digest to know what changed and why.

Yes, I'm going to clean up the commits after I'm done with my changes. It's just auto-pushing from my fork :/

Skewjo · 2021-03-07T20:38:13Z

@Krzmbrzl
I, mistakenly, installed a visual studio update before finishing this PR, and now my build process stays stuck at -- Qt5 component found: Widgets | Version: 5.15.2 for an hour or more (I've attempted to rebuild it 3 times now). ¯_(ツ)_/¯
So close, yet so far.

davidebeatrici · 2021-03-07T20:41:03Z

Prune the build directory.

Krzmbrzl · 2021-03-07T20:50:52Z

And also try waiting. Start the build and then go have lunch or something. On Windows it can take forever for this to work out.

Skewjo · 2021-03-07T20:58:58Z

@davidebeatrici, sorry, not sure how to prune the build directory. Is pruning referring to modifying one of the CMakeLists.txt files or a special prune command I can pass to CMake maybe?

Skewjo · 2021-03-07T22:39:16Z

Ok... I've built it, the threshold settings save, and it works (but not for server connect and disconnect).

Kissaki · 2021-03-07T22:40:18Z

Try removing the out and src folder, and check them out again. This should remove any intermediate and broken files.

Then generate and make steps again.

Skewjo · 2021-03-07T22:58:40Z

I'd like to see this polished a bit, but if I don't submit this PR now, I'm never going to finish it. I've checked the functionality for 2 different message types, but not all of them.

This functionality does not work for "Server Connected" or "Server Disconnected". (I'm guessing the functionality I'm using to get the current number of users on the server can't run before you're connected (or after you're disconnected). Those should likely be greyed out.
I'd like to make the qCheckBox (qcbMessageThreshold) and qSpinBox (qsbMessageThresholdUsers) part of the same UI component, but I believe that would involve changing the layout of the Misc. box from layout class="QVBoxLayout"" to layout class="QGridLayout" but that felt like an overreach.
- The spinbox should probably be greyed out when the checkbox is unchecked.
I'm worried the name of the column ("threshold") is too generic and may get confused with other functionalities.

Krzmbrzl · 2021-03-08T09:06:33Z

This functionality does not work for "Server Connected" or "Server Disconnected". (I'm guessing the functionality I'm using to get the current number of users on the server can't run before you're connected (or after you're disconnected). Those should likely be greyed out.

It's definitely not possible for the Connected situation. And for the Disconnected one I can imagine the disconnect already having happened when this part of the code is reached.
But then again you don't get spammed by these messages anyways, since you will only get them once each.

I'd like to make the qCheckBox (qcbMessageThreshold) and qSpinBox (qsbMessageThresholdUsers) part of the same UI component, but I believe that would involve changing the layout of the Misc. box from layout class="QVBoxLayout"" to layout class="QGridLayout" but that felt like an overreach.

I think that'd be quite important to do in order to clarify the context. So changing the layout is fine

The spinbox should probably be greyed out when the checkbox is unchecked.

Yes

I'm worried the name of the column ("threshold") is too generic and may get confused with other functionalities.

It is rather cryptic, yes. I am having a hard time finding a better column header though. Maybe Limit might be better (with a tooltip of what exactly is being limited)?

Krzmbrzl · 2021-03-08T09:08:07Z

Note also that you have merge conflicts.

In order to resolve them, run git pull --rebase https://github.com/mumble-voip/mumble.git master and then resolve the merge conflicts as to https://www.freecodecamp.org/news/how-to-handle-merge-conflicts-in-git/

Skewjo · 2021-03-13T20:45:29Z

@Krzmbrzl

Holy guacamole, I think I'm finally done.

Change to grid layout so the 2 buttons and text can be aligned ✅
Make spinbox gray out when option is disabled ✅
Added additional clarifying sentence to "Threshold" tooltip ✅

Krzmbrzl

I think "Limit" is probably the better name for this feature.
a) It seems to be more easily understandable
b) It's shorter and thus the column header doesn't require as much space

In that spirit, I think you should rename all variables, setting keys and all UI strings that I might have overlooked to use "limit" instead of "threshold". Unless you disagree with that choice of course ☝️

src/mumble/Log.cpp

Krzmbrzl · 2021-03-14T08:26:50Z

src/mumble/Log.cpp

 		twi->setWhatsThis(ColStaticSound, tr("Click here to toggle sound notification for %1 events.<br />If checked, "
 											 "Mumble uses a sound file predefined by you to indicate %1 events. Sound "
 											 "files and Text-To-Speech cannot be used at the same time.")
 											  .arg(messageName));
 		twi->setWhatsThis(ColStaticSoundPath,
-						  tr("Path to sound file used for sound notifications in the case of %1 events.<br />Single "
+						 tr("Path to sound file used for sound notifications in the case of %1 events.<br />Single "


Unrelated change

You guys don't follow the "boy scout" rule? 😛

It depends. Fixing this is fine, but it should be separated from the main changes. So if you wrap this up in its own FORMAT commit, then that'd be fine :)

src/mumble/Log.cpp

src/mumble/Log.ui

Skewjo · 2021-03-30T19:05:47Z

src/mumble/Log.cpp

@@ -223,6 +240,9 @@ void LogConfig::load(const Settings &r) {

 #endif
 	qcbWhisperFriends->setChecked(r.bWhisperFriends);
+	qcbMessageLimit->setChecked(r.bMessageLimit);
+	qsbMessageLimitUsers->setDisabled(!r.bMessageLimit);


This should resolve the uninitialized box issue.

Kissaki · 2021-03-30T22:03:34Z

From the column title “Threshold” I would expect it to be a message count or rate threshold, not a user count threshold. What do you think about qualifying it to “User Threshold” or some other terminology, like “User Limit” or “Max Users”?

There also seems to be no connection between the column and the setting below. I’m not sure if they are supposed to be connected or alternatives, but either way if they represent the same functionality they should use the same terminology.

Skewjo · 2021-03-30T22:56:05Z

Oh sorry, I thought I already changed that column header to be "Limit". Let me look again.

Skewjo · 2021-03-30T22:59:24Z

Yep, just needed to change the screenshot.

Krzmbrzl · 2021-03-31T14:54:15Z

There also seems to be no connection between the column and the setting below. I’m not sure if they are supposed to be connected or alternatives, but either way if they represent the same functionality they should use the same terminology.

Yep that is a good point. We should probably simply have the checkboxes in the column and below only have the spin box for setting the user count at which limiting starts 🤔

Skewjo · 2021-04-06T12:13:48Z

There also seems to be no connection between the column and the setting below. I’m not sure if they are supposed to be connected or alternatives, but either way if they represent the same functionality they should use the same terminology.

Yep that is a good point. We should probably simply have the checkboxes in the column and below only have the spin box for setting the user count at which limiting starts 🤔

That should be easy enough.

Skewjo · 2021-04-09T13:53:40Z

Ok @Krzmbrzl, mind giving it another look?

Krzmbrzl

Furthermore I think the menu entry for setting the user threshold for the limiting feature should be a simple form. Something like

Notification limit threshold: <spinBox> users

Also sorry for the delay. I didn't get to review this earlier.

src/mumble/Log.cpp

src/mumble/Settings.cpp

src/mumble/Settings.h

src/mumble/Settings.cpp

src/mumble/Settings.h

Skewjo · 2021-05-04T18:43:10Z

Ok, @Krzmbrzl, I think we're good now. Sorry for the delay. Got a new job and have limited time to work on this issue.

Skewjo · 2021-05-30T11:47:22Z

Ok @Krzmbrzl, I've got 2 days to finish this thing. Any chance you could take a look at it some time today?

Krzmbrzl

The changes LGTM.

The only thing I notices was that when you select the Limit option for all messages, all checkboxes are checked as epxteced. However unchecking a single checkbox for a particular message then causes all boxes to go unnchecked again.
This is a general problem of that settings page though and as such unrelated to your changes. Thus that is fine :)

I rebased your PR against the latest master branch and also added the missing translation commit.

I assume you have tested these changes to work as expected?

As requested in mumble-voip#2556, this commit implements the ability to turn off notifications when the server is hosting a number of clients defined by the user. Implements mumble-voip#2556

Krzmbrzl · 2021-05-31T12:58:21Z

Thank you very much for your contribution 👍

Skewjo · 2021-05-31T15:39:19Z

You're welcome! Glad to have been able to help. Hoping to make some time to do some documentation PRs some time in the near future.

Skewjo force-pushed the master branch from 36bb6ed to 0b1840f Compare February 28, 2021 18:02

Krzmbrzl reviewed Mar 1, 2021

View reviewed changes

src/mumble/Log.cpp Outdated Show resolved Hide resolved

Skewjo force-pushed the master branch from 0b1840f to 61d9bed Compare March 1, 2021 12:21

Skewjo commented Mar 1, 2021

View reviewed changes

src/mumble/Log.cpp Outdated Show resolved Hide resolved

Skewjo commented Mar 1, 2021

View reviewed changes

src/mumble/Log.ui Outdated Show resolved Hide resolved

Skewjo commented Mar 1, 2021

View reviewed changes

src/mumble/Log.ui Outdated Show resolved Hide resolved

Skewjo force-pushed the master branch from 315f2d1 to 9cc1592 Compare March 1, 2021 13:24

Skewjo marked this pull request as draft March 1, 2021 20:25

Skewjo force-pushed the master branch from e48baa3 to 221a36b Compare March 7, 2021 22:42

Skewjo marked this pull request as ready for review March 7, 2021 22:44

Skewjo changed the title ~~WIP Feat: Message Thresholding #2556~~ Feat: Message Thresholding #2556 Mar 7, 2021

Skewjo force-pushed the master branch 2 times, most recently from 29e488d to 57ffe53 Compare March 13, 2021 20:42

Krzmbrzl requested changes Mar 14, 2021

View reviewed changes

Skewjo force-pushed the master branch 4 times, most recently from 42d35b7 to aad0f6b Compare March 30, 2021 19:00

Skewjo commented Mar 30, 2021

View reviewed changes

Skewjo requested a review from Krzmbrzl March 30, 2021 23:07

Skewjo force-pushed the master branch from aad0f6b to 22fc293 Compare April 9, 2021 13:53

Krzmbrzl requested changes Apr 16, 2021

View reviewed changes

Skewjo force-pushed the master branch from 118d784 to d201036 Compare May 4, 2021 18:40

Krzmbrzl added feature-request This issue or PR deals with a new feature client labels May 30, 2021

Krzmbrzl approved these changes May 30, 2021

View reviewed changes

Krzmbrzl force-pushed the master branch 2 times, most recently from 1199969 to 4f49edc Compare May 30, 2021 16:05

Krzmbrzl changed the title ~~Feat: Message Thresholding #2556~~ FEAT(client): Message Thresholding May 31, 2021

Skewjo and others added 2 commits May 31, 2021 13:54

FEAT(client): Add notification thresholding

6a10b46

As requested in mumble-voip#2556, this commit implements the ability to turn off notifications when the server is hosting a number of clients defined by the user. Implements mumble-voip#2556

TRANSLATION: Update translation files

0827028

Krzmbrzl force-pushed the master branch from 4f49edc to 0827028 Compare May 31, 2021 11:55

Krzmbrzl merged commit adf3859 into mumble-voip:master May 31, 2021

Hartmnt mentioned this pull request Aug 1, 2022

Automatically temporarily disable user join/left sounds when users count in channel exceed a certain amount #2556

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FEAT(client): Message Thresholding #4808

FEAT(client): Message Thresholding #4808

Skewjo commented Feb 28, 2021 •

edited

Loading

Skewjo commented Mar 1, 2021

Krzmbrzl commented Mar 1, 2021

Skewjo commented Mar 1, 2021

Krzmbrzl commented Mar 1, 2021

Kissaki commented Mar 7, 2021

Skewjo commented Mar 7, 2021

Skewjo commented Mar 7, 2021

davidebeatrici commented Mar 7, 2021

Krzmbrzl commented Mar 7, 2021

Skewjo commented Mar 7, 2021

Skewjo commented Mar 7, 2021

Kissaki commented Mar 7, 2021

Skewjo commented Mar 7, 2021

Krzmbrzl commented Mar 8, 2021

Krzmbrzl commented Mar 8, 2021

Skewjo commented Mar 13, 2021

Krzmbrzl left a comment

Krzmbrzl Mar 14, 2021

Skewjo Mar 15, 2021 •

edited

Loading

Krzmbrzl Mar 15, 2021

Skewjo Mar 30, 2021

Kissaki commented Mar 30, 2021

Skewjo commented Mar 30, 2021

Skewjo commented Mar 30, 2021

Krzmbrzl commented Mar 31, 2021

Skewjo commented Apr 6, 2021

Skewjo commented Apr 9, 2021

Krzmbrzl left a comment

Skewjo commented May 4, 2021

Skewjo commented May 30, 2021

Krzmbrzl left a comment

Krzmbrzl commented May 31, 2021

Skewjo commented May 31, 2021

FEAT(client): Message Thresholding #4808

FEAT(client): Message Thresholding #4808

Conversation

Skewjo commented Feb 28, 2021 • edited Loading

Checks

Skewjo commented Mar 1, 2021

Krzmbrzl commented Mar 1, 2021

Skewjo commented Mar 1, 2021

Krzmbrzl commented Mar 1, 2021

Kissaki commented Mar 7, 2021

Skewjo commented Mar 7, 2021

Skewjo commented Mar 7, 2021

davidebeatrici commented Mar 7, 2021

Krzmbrzl commented Mar 7, 2021

Skewjo commented Mar 7, 2021

Skewjo commented Mar 7, 2021

Kissaki commented Mar 7, 2021

Skewjo commented Mar 7, 2021

Krzmbrzl commented Mar 8, 2021

Krzmbrzl commented Mar 8, 2021

Skewjo commented Mar 13, 2021

Krzmbrzl left a comment

Choose a reason for hiding this comment

Krzmbrzl Mar 14, 2021

Choose a reason for hiding this comment

Skewjo Mar 15, 2021 • edited Loading

Choose a reason for hiding this comment

Krzmbrzl Mar 15, 2021

Choose a reason for hiding this comment

Skewjo Mar 30, 2021

Choose a reason for hiding this comment

Kissaki commented Mar 30, 2021

Skewjo commented Mar 30, 2021

Skewjo commented Mar 30, 2021

Krzmbrzl commented Mar 31, 2021

Skewjo commented Apr 6, 2021

Skewjo commented Apr 9, 2021

Krzmbrzl left a comment

Choose a reason for hiding this comment

Skewjo commented May 4, 2021

Skewjo commented May 30, 2021

Krzmbrzl left a comment

Choose a reason for hiding this comment

Krzmbrzl commented May 31, 2021

Skewjo commented May 31, 2021

Skewjo commented Feb 28, 2021 •

edited

Loading

Skewjo Mar 15, 2021 •

edited

Loading