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.

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

Change icon when muted/deafened #3368

Merged
merged 2 commits into from Mar 11, 2018

Conversation

@davidebeatrici
Copy link
Member

commented Mar 11, 2018

Requested in #3367.

@mkrautz

This comment has been minimized.

Copy link
Member

commented Mar 11, 2018

Hmmmm.

We haven't usually changed the user's icon on the server like this before. I'm not sure what to think.

@bontibon
Copy link
Contributor

left a comment

I don't think this is a bad change, but I think that only your icon should be changing, not everyone elses in the server. If I'm muted with this patch, I can't see other users voice activity.

@tatokis

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2018

@davidebeatrici First of all, thanks for taking the time to bother with this. It is much appreciated.
Aforementioned issues aside, #3367 was about not having any visual indication in the GUI that you are actually muted due to using push to mute, not just being muted.
If the tray icon isn't available, you have no way of knowing that push to mute is actually working while the key is pressed.

The reason I suggested changing the user icon is because push to mute is the opposite of push to talk, which changes the icon itself.

Adding muted_pushtomute.svg to the right, along with the rest of the muted/deafened icons would also work, but it wouldn't be as obvious.

I didn't implement this myself, because aside from being completely unfamiliar with the codebase, I wasn't sure if changing the icon or adding a push to mute indicator for every user, despite it only being needed for the local one was deemed acceptable. After all, push to mute isn't transmitted to the server as far as I know.

Apologies for any inconvenience caused by this.

@davidebeatrici

This comment has been minimized.

Copy link
Member Author

commented Mar 11, 2018

@tatokis I added the push to mute icon, the problem is that it is set only when hovering the user's row, that's why I didn't commit the change yet.

I'm currently trying to fix that.

Thank you very much for the detailed feedback!

@davidebeatrici davidebeatrici force-pushed the davidebeatrici:status-icon branch from bc02082 to f540d9e Mar 11, 2018

@davidebeatrici

This comment has been minimized.

Copy link
Member Author

commented Mar 11, 2018

@tatokis Done.

@davidebeatrici davidebeatrici merged commit 80f3686 into mumble-voip:master Mar 11, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@tatokis

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2018

@davidebeatrici Looks good. Thanks!

@davidebeatrici

This comment has been minimized.

Copy link
Member Author

commented Mar 11, 2018

@tatokis You're welcome!

@bontibon

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2018

After using this patch a little more, there seems to be more UI inconsistency. The talking icon changes based on the mute and deafened state of the user, but only for the current user.

There is also duplicate user state information:

screenshot_2018-03-21_17-11-14

(Deafened is shown twice in the above image)

I propose the following: revert the changes to the user voice activity icon, and when push to mute is triggered, add the push to mute icon to the right hand column in the server tree.

@tatokis

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2018

I'll have to agree with @bontibon, however I do think that changing the icon only for push to mute strikes a good balance in consistency, as push to mute is only shown for the current user anyway, and it's also the opposite of push to talk.

@davidebeatrici

This comment has been minimized.

Copy link
Member Author

commented Mar 25, 2018

Since the push-to-mute state is not sent to the server (see #2736), the client can't know if other users are using the function, meaning that we can show the icon only for the local user.

That breaks consistency, because the main icon changes for every user according to the talking state.

I think that @bontibon's proposal is the best we can do at the moment.

@mkrautz

This comment has been minimized.

Copy link
Member

commented Mar 26, 2018

@davidebeatrici, @bontibon: Agreed. That's the correct place for it.

@davidebeatrici

This comment has been minimized.

Copy link
Member Author

commented Mar 26, 2018

Pull request created: #3383

@davidebeatrici

This comment has been minimized.

Copy link
Member Author

commented Mar 26, 2018

#3383 merged.

@Kissaki

This comment has been minimized.

Copy link
Member

commented Mar 26, 2018

I am a little confused about what I understood as other users supposedly having the user icons change as well? But I don't see that in the snapshot.

That breaks consistency, because the main icon changes for every user according to the talking state.

I have to disagree.

We handle two, or arguably three kinds of user states.

One is temporary state, like talking and push to mute, (continuous transmission is a special case, making the temporary talking state the norm).

The other is persistent, announced state like muted (also serving as a kind of afk/busy indicator, showing intent of muting oneself to others), and deafened (showing intent and that the other user will not hear what we say).

Push to mute is not a persistent mute state. As it only persists as long as the button is pressed, it is a temporary state - the opposite of PTT.

Let's also consider the voice indication though. When we receive voice from another user, we show this by the same talk state as when we talk ourselves. For the user, ignoring networking, this is fine and consistent. The one who talks (and transmits) is in the talking state. Technically behind it are two things though, the own talking state indicating transmission, and others talking state indicating receiving voice.

As such, changing the user talking state for PTT and PTM, but nothing else seems consistent to me.
More persistent intended user modes to the right of the user, also consistent.

@mkrautz

This comment has been minimized.

Copy link
Member

commented Mar 26, 2018

I don't think push-to-mute should be a talkstate.

In essence, push-to-mute is just a "cough button".

If you're transmitting voice, and you press it, your talk state should be "not talking".
That should be the case already, since the talk state is controlled via the Audio Input thread.

Before we added the push-to-mute state to the tray icon, there was nothing to indicate that you were using push-to-mute.

Perhaps it was better that way -- the system tray would simply indicate "not talking". Then you would know you weren't transmitting.

Though if people think having an extra state to know that you're pressing it is the way to go, I don't mind.
But I think it should go as a "user state" not a "talk state". (That is, I think it should be the way it is implemented in #3383.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.