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 crash in simple_window example #644

Merged
merged 1 commit into from
Sep 20, 2023
Merged

Conversation

ids1024
Copy link
Member

@ids1024 ids1024 commented Jul 25, 2023

We need to handle or ignore events from wl_shm and wl_surface, since the server does in fact produce events for those.

delegate_noop!(State: wl_surface::WlSurface);
delegate_noop!(State: wl_shm::WlShm);
delegate_noop!(State: ignore wl_surface::WlSurface);
delegate_noop!(State: ignore wl_shm::WlShm);
Copy link
Member

Choose a reason for hiding this comment

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

Wait why are we ignoring WlShm?

I'd think that knowing what formats are supported would be effectively mandatory?

Copy link
Member Author

Choose a reason for hiding this comment

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

All renderers should support argb8888 and xrgb8888 but any other formats are optional and may not be supported by the particular renderer in use.

https://wayland.app/protocols/wayland

So I think it's perfectly valid for the example to simply assume wl_shm::Format::Argb8888 is available.

@softmoth
Copy link

softmoth commented Aug 6, 2023

I'm still seeing a crash here, even with adding ignore to these two lines. If I revert commit be86c69 then the crash goes away and I get a successful window.

% RUST_BACKTRACE=1 cargo run
   Compiling wl-alttab v0.1.0 (/home/trs/g/wl-alttab)
    Finished dev [unoptimized + debuginfo] target(s) in 0.27s
     Running `target/debug/wl-alttab`
Starting the example window app, press <ESC> to quit.
thread 'main' panicked at 'internal error: entered unreachable code', src/main.rs:121:1
stack backtrace:
   0: rust_begin_unwind
             at /rustc/371994e0d8380600ddda78ca1be937c7fb179b49/library/std/src/panicking.rs:593:5
   1: core::panicking::panic_fmt
             at /rustc/371994e0d8380600ddda78ca1be937c7fb179b49/library/core/src/panicking.rs:67:14
   2: core::panicking::panic
             at /rustc/371994e0d8380600ddda78ca1be937c7fb179b49/library/core/src/panicking.rs:117:5
   3: <wl_alttab::State as wayland_client::event_queue::Dispatch<wayland_client::protocol::wl_buffer::WlBuffer,()>>::event
             at /home/trs/.cargo/registry/src/index.crates.io-6f17d22bba15001f/wayland-client-0.30.2/src/event_queue.rs:843:17
   4: wayland_client::event_queue::queue_callback
             at /home/trs/.cargo/registry/src/index.crates.io-6f17d22bba15001f/wayland-client-0.30.2/src/event_queue.rs:651:5
   5: wayland_client::event_queue::EventQueue<State>::dispatching_impl
             at /home/trs/.cargo/registry/src/index.crates.io-6f17d22bba15001f/wayland-client-0.30.2/src/event_queue.rs:473:13
   6: wayland_client::event_queue::EventQueue<State>::dispatch_pending
             at /home/trs/.cargo/registry/src/index.crates.io-6f17d22bba15001f/wayland-client-0.30.2/src/event_queue.rs:376:9
   7: wayland_client::event_queue::EventQueue<State>::blocking_dispatch
             at /home/trs/.cargo/registry/src/index.crates.io-6f17d22bba15001f/wayland-client-0.30.2/src/event_queue.rs:404:9
   8: wl_alttab::main
             at ./src/main.rs:37:9
   9: core::ops::function::FnOnce::call_once
             at /rustc/371994e0d8380600ddda78ca1be937c7fb179b49/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

@softmoth
Copy link

softmoth commented Aug 7, 2023

Ah, I see the problem. It is WlShm and WlBuffer that need to be ignored, not WlSurface. I.e., this does not crash:

// Ignore events from these object types in this example.
delegate_noop!(State: wl_compositor::WlCompositor);
delegate_noop!(State: wl_surface::WlSurface);
delegate_noop!(State: ignore wl_shm::WlShm);
delegate_noop!(State: wl_shm_pool::WlShmPool);
delegate_noop!(State: ignore wl_buffer::WlBuffer);

I'm using a freshly updated +nightly, and my Cargo.toml says:

[dependencies]
wayland-client = { version = "*" }
wayland-protocols = { version = "*", features = ["client"] }
tempfile = "*"

I don't know if different environments cause different events. I'm on Arch Linux, running sway with the nvidia 535.86.05 driver.

@softmoth
Copy link

softmoth commented Aug 7, 2023

By the way, it would be simplest to ignore all of these, since that is what the code did before be86c69 and was probably the intention of that commit.

We need to handle or ignore events from `wl_shm` and `wl_surface`, since
the server does in fact produce events for those.
@codecov
Copy link

codecov bot commented Aug 31, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (799c772) 73.11% compared to head (947455f) 73.12%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #644   +/-   ##
=======================================
  Coverage   73.11%   73.12%           
=======================================
  Files          47       47           
  Lines        7693     7692    -1     
=======================================
  Hits         5625     5625           
+ Misses       2068     2067    -1     
Flag Coverage Δ
main 58.61% <ø> (+<0.01%) ⬆️
test-- 78.10% <ø> (ø)
test--server_system 61.30% <ø> (ø)
test-client_system- 69.13% <ø> (ø)
test-client_system-server_system 51.32% <ø> (ø)

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

see 1 file with indirect coverage changes

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

@i509VCB
Copy link
Member

i509VCB commented Sep 20, 2023

@elinorbgr comments?

@elinorbgr elinorbgr merged commit e1be14c into Smithay:master Sep 20, 2023
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants