Murmur sends a UserState message with both self_mute and self_deaf when a user mutes themselves

[ianling]

If a user mutes themselves but does not deafen, Murmur will sound out a UserState message that contains both the self_mute and self_deaf fields, even though only one changed:

session: 40
actor: 40
self_mute: true
self_deaf: false

Likewise, when they unmute themselves, it again sends both of the fields, even though only one changed.

session: 40
actor: 40
self_mute: false
self_deaf: false

This is inconsistent with the UserState message that is sent out when a server admin mutes a user:

session: 191
actor: 40
mute: true

Only the field that was altered is sent out.

[ElizabethCody]

Why exactly should optional fields only be sent in the event that they are updated? If the argument is for consistency, then it would be equally consistent to always send optional fields.

But why should we even strive for consistency? There really isn't a specification for the Mumble protocol beyond the Protobuf message definitions, so it seems to me that whatever decisions are most practicable from an implementation point of view are the ones that get made.

If you feel that matters like this are important to the Mumble project, then the discussion should be regarding whether or not Mumble should have to adhere to a prescribed specification. Such a specification could eliminate the need for discretionary implementation decisions like this, but it would also be quite the undertaking.

That is of course if this is just for consistency's sake, if not -- if there is some practical advantage to only sending modified fields, then I would suggest that you outline that advantage rather than just underscoring an inconsistency.

[ianling]

The practical advantage of consistency is... consistency. The official implementations of the protocol shouldn't behave in unpredictable or undocumented ways. I agree that if all of the messages always included all of the optional fields, it would be fine. Or even if this specific UserState message always sent all the optional fields, it would be fine. However, this specific message only sends one additional optional field, and only in this one specific scenario.

It's just one additional undocumented oddity that you will only discover when you are implementing the protocol in your own program and run into a bug because you were only expecting fields that were altered to be present in a message, since that just happens to be the case everywhere else in the official implementation of the protocol.

Everyone should strive for consistency when coding. There was no practical reason to deviate from the standard set by the implementation of all the other messages in the protocol. Keeping it is lazy and wastes other developers' time.

If nothing else, it wastes a few bytes of bandwidth for useless data. I don't especially care about this issue, just pointing out the inconsistency when compared to every other protobuf message in the Mumble protocol.

[unterwulf]

Consistency is important from the principle of least astonishment's perspective. When everywhere else in the code unchanged optional fields are not being sent it makes an impression that if such a field is yet being sent it is made for a reason. And if there were no real reason such code is just misleading and frustrating.

From the practical point of view I noticed that at least some fields are handled by Mumble regardless of their current values. E.g. whenever server includes the suppress field into a UserState message, Mumble prints a message even if the state is the same. (Have seen this behavior on 1.2.18 from Debian repo. Haven't tested on the master.) This creates a feeling that the common approach is not to send optional fields unless their values have changed.