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

Include advertised version in BindError::UnsupportVersion #671

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

hw0lff
Copy link
Contributor

@hw0lff hw0lff commented Oct 29, 2023

Reporting the advertised version is helpful because I can then report this to the user in case of an error.
This way I can get more information in a reported issue.

I also added two code clean up commits since the code was right next to each other.

The tests did not run successfully but they don't run through on master either.
Both branches fail at client_receive_generic_error:

---- client_receive_generic_error stdout ----
Protocol error 42 on object wl_compositor@3:
Protocol error 42 on object wl_compositor@3:
thread 'client_receive_generic_error' panicked at wayland-tests/tests/protocol_errors.rs:56:9:
assertion `left == right` failed
  left: ""
 right: "I don't like you!"

@codecov
Copy link

codecov bot commented Oct 29, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (8581b9d) 73.02% compared to head (078ebd7) 73.17%.
Report is 16 commits behind head on master.

Files Patch % Lines
wayland-client/src/globals.rs 60.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #671      +/-   ##
==========================================
+ Coverage   73.02%   73.17%   +0.14%     
==========================================
  Files          47       47              
  Lines        7779     7829      +50     
==========================================
+ Hits         5681     5729      +48     
- Misses       2098     2100       +2     
Flag Coverage Δ
main 58.50% <0.00%> (+0.16%) ⬆️
test-- 78.15% <60.00%> (?)
test--server_system 61.17% <60.00%> (-0.18%) ⬇️
test-client_system- 69.15% <60.00%> (+0.04%) ⬆️
test-client_system-server_system 51.24% <60.00%> (-0.19%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

BindError::UnsupportedVersion {} => {
write!(f, "the requested version of the global is not supported")
BindError::UnsupportedVersion(version) => {
write!(f, "the requested version {version} of the global is not supported")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wording suggests that {version} is the version that was requested by the user, not that advertised by the compositor.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah my mistake, I got confused there!
Should I remove the {version} completely or reorder the sentence to announce the actually available version?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe change the sentence so that it uses version but in a meaningful way

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think of the available version {version} of the global is lower than the requested version?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sound good yes 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@elinorbgr It is now in the updated commit

@elinorbgr
Copy link
Member

The client_receive_generic_error is unrelated and the tests actually pass, it's just that cargo features interact badly with running tests for the whole workspace at once. The reference is the CI (on which the test pass as needed). I'm confused about the check-minimal CI job, but that seems unrelated to your PR.

@@ -232,7 +226,7 @@ impl From<InvalidId> for GlobalError {
#[derive(Debug)]
pub enum BindError {
/// The requested version of the global is not supported.
UnsupportedVersion,
UnsupportedVersion(u32),
Copy link
Member

@elinorbgr elinorbgr Nov 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a breaking change, so please include a changelog entry for this

@MarijnS95
Copy link
Contributor

Would a similar change be accepted for NotPresent?

In this specific case I used to get:

thread 'main' panicked at .cargo/registry/src/index.crates.io-6f17d22bba15001f/smithay-client-toolkit-0.16.1/src/environment.rs:193:21:
[SCTK] A missing global was required: wl_subcompositor

On winit 0.28 and all the Wayland dependencies it used back then, but nowdays on winit 0.29 it is a way less descriptive:

called `Result::unwrap()` on an `Err` value: Os(OsError { line: 95, file: ".cargo/registry/src/index.crates.io-6f17d22bba15001f/winit-0.29.4/src/platform_impl/linux/wayland/event_loop/mod.rs", error: WaylandError(Bind(NotPresent)) })

I'd like to include (String) with the interface name, if that's fine :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants