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.

Sign up for GitHub

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

Jump to bottom

Starting with Automatic Encoding will disable quality menu #4427

Closed
GUAN-Xihao opened this issue Nov 26, 2024 · 6 comments
Closed

Starting with Automatic Encoding will disable quality menu #4427

GUAN-Xihao opened this issue Nov 26, 2024 · 6 comments
Labels
bug Something isn't working

Comments

@GUAN-Xihao
Copy link

GUAN-Xihao commented Nov 26, 2024 •
edited
Loading

Describe the bug
Following by #4398
Now in v6.2.1, if you start without --encoding option, the encoding will be recorded as "auto"
image
If you manually click the auto option like #4398 describes, then due to the following code:
https://github.com/Xpra-org/xpra/blob/master/xpra/client/mixins/encodings.py#L302
The encoding will be "".

To Reproduce
Steps to reproduce the behavior:

  1. Star Xpra, encoding is automatic and the quality menu is NOT available.
  2. Now, click automatic encoding. Then quality menu is enabled. As the above screenshot shows.

System Information (please complete the following information):

  • Client OS: Windows 11
  • Xpra Client Version 6.2.0
@GUAN-Xihao GUAN-Xihao added the bug Something isn't working label Nov 26, 2024
GUAN-Xihao added a commit to GUAN-Xihao/xpra that referenced this issue Nov 26, 2024
@GUAN-Xihao
Xpra-org#4427 mark auto encoding as-is
b9e6165
@GUAN-Xihao GUAN-Xihao mentioned this issue Nov 26, 2024
Closed
@totaam
Copy link
Collaborator

totaam commented Nov 26, 2024

What is the problem with having encoding = ""?
What behaviour does this change fix?

IIRC, the empty value has different semantics.

@GUAN-Xihao
Copy link
Author

GUAN-Xihao commented Nov 26, 2024 •
edited
Loading

What is the problem with having encoding = ""?
What behaviour does this change fix?
IIRC, the empty value has different semantics.

From commit 2c9301c,
GENERIC_ENCODINGS = ("", "stream", "grayscale")
You may choose to mark automatic encoding to "" either "auto"
But v6.2.1 behavior is 1) if you start without --encoding, then it will mark as "auto".
https://github.com/Xpra-org/xpra/blob/master/xpra/client/mixins/encodings.py#L158
2) if you manually select encoding as automatic from the menu, then it will be marked as "".
https://github.com/Xpra-org/xpra/blob/master/xpra/client/mixins/encodings.py#L302
You may use "auto" or "" on the above two statuses.

And the commit is trying use "auto" in both cases.

@GUAN-Xihao
Copy link
Author

From your IIRC info, maybe
GENERIC_ENCODINGS = ("", "auto", "stream", "grayscale")

is what you prefer. If so let me know and I can update the PR.

@totaam
Copy link
Collaborator

totaam commented Nov 26, 2024

I will have to test this myself to understand.
Hopefully tomorrow.

@GUAN-Xihao
Copy link
Author

I will have to test this myself to understand. Hopefully tomorrow.

Hi totaam, what do you think about it?

totaam added a commit that referenced this issue Dec 3, 2024
@totaam
#4427 unset also means 'auto'
2692ac3
totaam added a commit that referenced this issue Dec 3, 2024
@totaam
#4427 set and send 'auto' encoding value
01b4061 
the empty value should still behave the same as before,
so that if somehow there is another way of getting this value
then this should not break anything
@totaam
Copy link
Collaborator

totaam commented Dec 3, 2024

But v6.2.1 behavior is 1) if you start without --encoding, then it will mark as "auto".

I don't know what "mark" means here.
The code you linked to in this comment is only ever used by "info" queries (in the get_info method), and nothing else AFAICT, so how is it relevant?

if you manually select encoding as automatic from the menu, then it will be marked as "".

This sets self.encoding.

This attribute is also used here:

xpra/xpra/client/mixins/encodings.py

Line 192 in 8940633

if self.encoding and e != self.encoding:

It is only ever triggered when processing the handshake at present, but this may well change: #4315
So this has to be taken into account - I think the first change above is safe for backporting.
The second one will only be in 6.3 and later.

@totaam totaam closed this as completed Dec 3, 2024
totaam added a commit that referenced this issue Dec 3, 2024
@totaam
#4427 unset also means 'auto'
6d4e633
totaam added a commit that referenced this issue Dec 3, 2024
@totaam
#4427 the value 'auto' can be set from the tray menu
91d2a5c
totaam added a commit that referenced this issue Dec 3, 2024
@totaam
#4427 the value 'auto' can be set from the tray menu
0319aaa
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

#4427 mark auto encoding as-is GUAN-Xihao/xpra
2 participants
@totaam @GUAN-Xihao