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

FIX(client): Respect unlimited imagemessagelength #4611

Merged
merged 1 commit into from Dec 9, 2020

Conversation

merua
Copy link
Contributor

@merua merua commented Dec 5, 2020

When you set the imagemessagelength to 0 on the server and paste an
image into the client chat no error should pop up that the image is too
large.

When you set the imagemessagelength to 0 on the server and paste an
image into the client chat no error should pop up that the image is too
large.
@merua
Copy link
Contributor Author

merua commented Dec 5, 2020

I really missed pasting images directly into the Mumble chat and thought this is covered by this issue and wanted to work on it. But the current master already implements this so maybe this issue can be closed?

Would it be possible that the client compresses the image to the size the server allows before sending it? Or does that violate any design choices? As a user I wouldn't expect Mumble to transmit images without loss but I find it quite cumbersome to compress them manually before sending it.

@Krzmbrzl
Copy link
Member

Krzmbrzl commented Dec 5, 2020

In the documentation on that config option (https://wiki.mumble.info/wiki/Murmur.ini#imagemessagelength) I don't see any mention of a special treatment if the value is set to 0. Where did you find any evidence that the behavior you implemented is actually intended?
Or is this not a fix but rather a new feature? 🤔

I really missed pasting images directly into the Mumble chat and thought this is covered by this issue and wanted to work on it. But the current master already implements this so maybe this issue can be closed?

Absolutely. Thanks for bringing this to our attention 👍

Would it be possible that the client compresses the image to the size the server allows before sending it?

I guess in principle this should be possible. Afaik though the client does not know the value of this limit and therefore it can't act on it (not 100% sure though).
Thus in order to implement it, the client would first have to fetch that information from the server...

@merua
Copy link
Contributor Author

merua commented Dec 8, 2020

I don't see any mention of a special treatment if the value is set to 0. Where did you find any evidence that the behavior you implemented is actually intended?

I found it here.

I guess in principle this should be possible. Afaik though the client does not know the value of this limit and therefore it can't act on it (not 100% sure though).
Thus in order to implement it, the client would first have to fetch that information from the server...

As far as I understood the code the client does fetch the value of this limit (otherwise how would it know to reject a too large image?) and I think it happens here.

@Krzmbrzl
Copy link
Member

Krzmbrzl commented Dec 9, 2020

Ah so the wiki does not contain all information. I added the information there.
Besides I just noticed that (as stated in the title) this PR is only about the client. The server already behaves in this way:

if (iMaxImageMessageLength > 0
&& (msg.texture().length() > static_cast< unsigned int >(iMaxImageMessageLength))) {
PERM_DENIED_TYPE(TextTooLong);
return;
}

This then of course means that you are absolutely correct by saying that the limit is transferred to the client (and the linked code part should be the correct one for that).

Thanks for the clarification 👍

@Krzmbrzl Krzmbrzl added backport-needed bug A bug (error) in the software client labels Dec 9, 2020
@Krzmbrzl Krzmbrzl merged commit f8f9442 into mumble-voip:master Dec 9, 2020
@Krzmbrzl
Copy link
Member

Krzmbrzl commented Dec 9, 2020

Thank you very much for your contribution! :)

@merua
Copy link
Contributor Author

merua commented Dec 9, 2020

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A bug (error) in the software client
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants