Skip to content

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: Patch coverage is 60.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 73.17%. Comparing base (8581b9d) to head (078ebd7).
Report is 138 commits behind head on master.

Files with missing lines 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.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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.

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