starred/mumble-mumble-voip

Fork 0

mirror of https://github.com/mumble-voip/mumble.git synced 2026-03-03 00:46:56 -05:00

inconsistency: why are we using uint32 instead of enum type in ContextActionModify #1813

New issue

Closed

opened 2026-02-20 21:14:38 -05:00 by deekerman · 9 comments

deekerman commented

2026-02-20 21:14:38 -05:00

Owner

Originally created by @wfjsw on GitHub (Apr 29, 2020).

Originally assigned to: @Krzmbrzl on GitHub.

github.com/mumble-voip/mumble@8aadee917d/src/Mumble.proto (L404)

Why are we using uint32 here instead of Context ?

P.S. When I'm running git blame I notice many breaking changes here across 1.1.x and 1.2.x and now I'm wondering whether it is safe or not to use this message.

Originally created by @wfjsw on GitHub (Apr 29, 2020). Originally assigned to: @Krzmbrzl on GitHub. https://github.com/mumble-voip/mumble/blob/8aadee917dea7a256b283585d14e1d0f0080fe27/src/Mumble.proto#L404 Why are we using `uint32` here instead of `Context` ? P.S. When I'm running `git blame` I notice many breaking changes here across 1.1.x and 1.2.x and now I'm wondering whether it is safe or not to use this message.

deekerman

2026-02-20 21:14:38 -05:00

closed this issue
added the
documentation
label

deekerman commented

2026-02-20 21:14:43 -05:00

Author

Owner

@Krzmbrzl commented on GitHub (Apr 29, 2020):

I'm not entirely sure, but my guess would be in order to allow for or'ing context flags together. It'd probably also work if we used Context as the type which would then probably be defined to whatever underlying integer type the enuk uses, but if the enuk contents change, that class might too which would then in turn break the compatibility with older protocol versions...

@Krzmbrzl commented on GitHub (Apr 29, 2020): I'm not entirely sure, but my guess would be in order to allow for or'ing context flags together. It'd probably also work if we used `Context` as the type which would then probably be defined to whatever underlying integer type the enuk uses, but if the enuk contents change, that class might too which would then in turn break the compatibility with older protocol versions...

deekerman commented

2026-02-20 21:14:44 -05:00

Author

Owner

@wfjsw commented on GitHub (Apr 29, 2020):

github.com/mumble-voip/mumble@f7d7b99b56 (diff-9f7f9c7762)

And this part looks shocking. We are removing message from the available message list without documenting it somewhere to avoid collision?

@wfjsw commented on GitHub (Apr 29, 2020): https://github.com/mumble-voip/mumble/commit/f7d7b99b56497e22dddfdfba5a6f06945ddc5261#diff-9f7f9c7762f1baf215a55a8c098cd531 And this part looks shocking. We are removing message from the available message list without documenting it somewhere to avoid collision?

deekerman commented

2026-02-20 21:14:44 -05:00

Author

Owner

@Krzmbrzl commented on GitHub (Apr 29, 2020):

I can't say much about commits that old.

Why are you even interested in these ancient commits?

@Krzmbrzl commented on GitHub (Apr 29, 2020): I can't say much about commits that old. Why are you even interested in these ancient commits?

deekerman commented

2026-02-20 21:14:44 -05:00

Author

Owner

@wfjsw commented on GitHub (Apr 29, 2020):

There are unmaintained clients operate at this protocol version (such as Plumble) which I have to take care of to prevent crashing in future.

I'm just having a feeling that, we were introducing major level incompatibility while only increasing minor or patch level version number.

@wfjsw commented on GitHub (Apr 29, 2020): There are unmaintained clients operate at this protocol version (such as Plumble) which I have to take care of to prevent crashing in future. I'm just having a feeling that, we were introducing major level incompatibility while only increasing minor or patch level version number.

deekerman commented

2026-02-20 21:14:45 -05:00

Author

Owner

@Krzmbrzl commented on GitHub (Apr 29, 2020):

Ok, then I see your problem here.

(In case that helps you: Someone has picked up the Plumble development under the name Mumla: https://gitlab.com/quite/mumla)

I'm just having a feeling that, we were introducing major level incompatibility while only increasing minor or patch level version number.

This appears to be the case, yes. I guess that shouldn't have happened, but what's done is done. Not much we can do about this now 🤷

@Krzmbrzl commented on GitHub (Apr 29, 2020): Ok, then I see your problem here. (In case that helps you: Someone has picked up the Plumble development under the name `Mumla`: https://gitlab.com/quite/mumla) > I'm just having a feeling that, we were introducing major level incompatibility while only increasing minor or patch level version number. This appears to be the case, yes. I guess that shouldn't have happened, but what's done is done. Not much we can do about this now :shrug:

deekerman commented

2026-02-20 21:14:45 -05:00

Author

Owner

@wfjsw commented on GitHub (Apr 29, 2020):

I do think we should document them somewhere to prevent reuse of that message enum in the future. Imaging we add a new message and it uses that enum, then old client will try to decode the new message with the old protobuf definition and it would certainly result in a crash.

P.S. Mumla crashed on my server lol.

@wfjsw commented on GitHub (Apr 29, 2020): I do think we should document them somewhere to prevent reuse of that message enum in the future. Imaging we add a new message and it uses that enum, then old client will try to decode the new message with the old protobuf definition and it would certainly result in a crash. P.S. Mumla crashed on my server lol.

deekerman commented

2026-02-20 21:14:45 -05:00

Author

Owner

@Krzmbrzl commented on GitHub (Apr 30, 2020):

Are you referring to the ContextActionAdd and ContextActionRemove messages? Or what enum are you referring to?
Either way I agree that this should be documented :)

P.S. Mumla crashed on my server lol.

Woops. Guess you can file a bug for it then ^^
To be fair though the development hasn't started that long ago yet, so one can't expect miracles ^^

@Krzmbrzl commented on GitHub (Apr 30, 2020): Are you referring to the `ContextActionAdd` and `ContextActionRemove` messages? Or what *enum* are you referring to? Either way I agree that this should be documented :) > P.S. Mumla crashed on my server lol. Woops. Guess you can file a bug for it then ^^ To be fair though the development hasn't started that long ago yet, so one can't expect miracles ^^

deekerman commented

2026-02-20 21:14:45 -05:00

Author

Owner

@wfjsw commented on GitHub (Apr 30, 2020):

I was thinking about ContextActionRemove until I find out these commits are made on the same day. ContextActionModify has taken the place of ContextActionAdd but using a compatible signature except Operation left uninterpreted. Possibly a note about this behavior for pre-1.2.4 clients.

@wfjsw commented on GitHub (Apr 30, 2020): I was thinking about `ContextActionRemove` until I find out these commits are made on the same day. `ContextActionModify` has taken the place of `ContextActionAdd` but using a compatible signature except `Operation` left uninterpreted. Possibly a note about this behavior for pre-1.2.4 clients.

deekerman commented

2026-02-20 21:14:45 -05:00

Author

Owner

@Krzmbrzl commented on GitHub (Apr 30, 2020):

Feel free to create a PR for this. It was you who dug this out, so you might as well take the credit (in form of the commit-authorship).
If you don't want to, I'll do it :)

@Krzmbrzl commented on GitHub (Apr 30, 2020): Feel free to create a PR for this. It was you who dug this out, so you might as well take the credit (in form of the commit-authorship). If you don't want to, I'll do it :)

deekerman referenced this issue

2026-02-20 21:15:18 -05:00

New startup wizard #1830

deekerman referenced this issue

2026-02-20 21:16:04 -05:00

allow to link a carddav addressbook to Mumble #1849

deekerman referenced this issue

2026-02-20 21:16:42 -05:00

Mumble rewrite #1864