Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

FR#1119 Transmission mode switching in toolbar #174

Closed
wants to merge 7 commits into
from

Conversation

Projects
None yet
5 participants
Contributor

askreet commented Sep 15, 2013

As requested in https://sourceforge.net/p/mumble/feature-requests/1119/.

I'm not sure why this is including my commit for CycleTransmitMode (1a42558), that appears to be merged with mumble-voip/master already.

Contributor

askreet commented Sep 16, 2013

Okay, I figured out what commit 1a42558 was I duplicated some code, fixed in commit 5db4d51 and bugfix in ec99439 for compatibility with Cycle Transmit Mode to keep the new indicators updated.

Contributor

askreet commented Nov 24, 2013

Ping. I'd like this feature to be in the next build.

@Kissaki Kissaki was assigned Dec 7, 2013

@mkrautz mkrautz commented on an outdated diff Dec 7, 2013

src/mumble/MainWindow.h
@@ -130,6 +130,7 @@ class MainWindow : public QMainWindow, public MessageHandler, public Ui::MainWin
void setOnTop(bool top);
void setShowDockTitleBars(bool doShow);
void updateTrayIcon();
+ void MainWindow::updateTransmitModeIcons();
@mkrautz

mkrautz Dec 7, 2013

Owner

void updateTransmitModeIcons();

Contributor

askreet commented Jan 2, 2014

Sorry for the delay, I fixed that function declaration finally. :-)

Owner

Kissaki commented Jan 4, 2014

These changes add three icons to the toolbar for switching the transmission mode between VA, PTT, C.

mumble -- compiled jan 4 2014 02_28_47_2014-01-04_02-40-57

Owner

Kissaki commented Jan 4, 2014

From our discussion we decided to not accept this PR as it is.

The toolbar should only contain (very) commonly used actions. We feel changing the transmission mode is a very rare use case. Probably very few people regularly change them. A shortcut for cycling transmission modes is bindable to (keyboard) input.

Also, for this rare usecase three buttons in the toolbar seem a bit excessive. Rather, it should be one drop-down.

I guess we would discuss this again if

  1. instead of three buttons it would be one drop-down
  2. it would not be in the toolbar by default, but optionally activateable in Mumbles settings

Ultimately, the toolbar should be user-customizable - which it currently is not.

The implementation itself seems to work fine and at a first glance the code looks good (and minimal).

Even though we will not accept it as it is, thank you for your concrete contribution, we appreciate it.

@Kissaki Kissaki closed this Jan 4, 2014

Contributor

askreet commented Jan 4, 2014

Personally I use this feature each day, in addition to the cycle transmit mode feature. I think a drop-down menu is an improvement and I will modify this branch for that. I don't know about adding it as an option, though, as it would be the first option that modified that main toolbar. Seems to me that customizable toolbars is a bit more complex than Mumble requires.

Contributor

bendem commented Jan 4, 2014

I'd use this feature everyday as well, thanks for improving mumble! Can't wait to see it finally merged...

danboid commented Jan 4, 2014

I waited months to see if this would get merged, then today - my birthday of all days - the first thing I discover is that its been rejected! What crappy timing for a poor outcome! :(

It seems you're incorrect in deciding this option is rarely changed as we can already see from these comments that at least three or four people following Mumble's dev wanted to see this get added. Like askreet, I'm constantly changing the transmit mode when I'm using mumble due to its poor echo cancellation. Ideally, I could leave it on constant transmit mode but I can only enable that when I'm wearing headphones and sat in front of the mic/computer. Adding such a shortcut also highlights transmit mode which I firmly believe is one of the most important of Mumbles settings for the same reason (working round the poor echo cancellation).

Due to there being plenty of free space in the Mumble toolbar, I thought adding three icons for each transmit mode would be fine as there is still plenty of space left after adding them in for additional shortcuts. The individual icons allow you to switch transmit mode easier than using a drop-down which I doubt is going to save that many horizonal pixels, if any, in comparison unless the options in the dropdown are abbreviated like on the icons. However, I'd much rather have the drop-down menu than not see this implemented at all.

I'm running 1.2.4 but I don't see any option to bind a keyboard shortcut to changing transmit mode - maybe this has been added in git since?

Contributor

bendem commented Jan 4, 2014

@danboid You shouldn't criticize the decision taken by the mumble core team without properly reading 😉
The feature has not been rejected, only this PR.

I will modify this branch for that

And I agree with them in the fact that "3 or 4 people following mumble's dev" are not, at all, representative of the mumble user community.

It's also correct to say that adding 3 buttons (which are a bit poorly name btw) is big addition to the toolbar and should not be done without thinking of the main part of the users (even if generally the devs don't) who fears like hell changing settings...

danboid commented Jan 4, 2014

Its not been rejected? So what is the status of this patch / suggestion?

I've not tried the patch myself yet as I was waiting for it to get pulled but when you mouse-over the icons it should explain in full what each one does, which would make up for the rather cryptic icons.

Changing the transmit mode is nothing to worry about and poses no danger to any user. Instead I expect new and experienced Mumble uses alike will appreciate this feature being given more attention and made more easily accessible.

Contributor

bendem commented Jan 4, 2014

If you read what as been said :

I guess we would discuss this again if

  1. instead of three buttons it would be one drop-down
  2. it would not be in the toolbar by default, but optionally activateable in Mumbles settings

So the author of the patch said

Personally I use this feature each day, in addition to the cycle transmit mode feature. I think a drop-down menu is an improvement and I will modify this branch for that.

danboid commented Jan 4, 2014

OK - thanks for clarifying that bendem!

Owner

Kissaki commented Jan 4, 2014

It sucks we would have to introduce yet another option into our overly complex settings.

We could really use a plugin/customizable toolbar system. :-/

Contributor

askreet commented Mar 18, 2014

Just tossing an update here for anyone who's tracking the progress of this. Finally spent a bit of time on this today, have an up-to-date branch with a working QComboBox in the menu. I need to add a configuration option to enable this, and I'll be submitting a new PR.

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