Add GlobalFactory::can_view to filter globals on a per-client basis#4859
Add GlobalFactory::can_view to filter globals on a per-client basis#4859
Conversation
mattkae
force-pushed
the
wayland-rs-global-filter
branch
from
5862a79 to
9df6ce8
Compare
There was a problem hiding this comment.
Pull request overview
This PR extends the Wayland Rust/C++ bridging layer to support per-client filtering of advertised Wayland globals by adding a GlobalFactory::can_view() hook that is wired into generated GlobalDispatch implementations (similar in spirit to wl_display_set_global_filter).
Changes:
- Generate
GlobalDispatch::can_view()implementations that delegate global visibility decisions toffi::GlobalFactory::can_view(). - Extend the C++ header generator to emit a
GlobalFactory::can_view()virtual method and forward-declareWaylandClientId. - Add
CppType::Boolsupport to the generator so C++boolreturn/arg types can be emitted and bound to Rust.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/server/frontend_wayland/wayland_rs/build_script/main.rs |
Generates can_view() in Rust dispatch and adds a corresponding virtual method to the generated GlobalFactory C++ interface. |
src/server/frontend_wayland/wayland_rs/build_script/cpp_builder.rs |
Adds generator support for C++/Rust bool types needed by GlobalFactory::can_view(). |
| fn can_view(client: Client, global_data: &Arc<Mutex<cxx::UniquePtr<ffi::GlobalFactory>>>) -> bool { | ||
| let interface_name = #interface_name_str.to_string(); | ||
| let client_id = Box::new(WaylandClientId::new(client.id())); | ||
| let mut guard = global_data.lock().unwrap(); |
There was a problem hiding this comment.
GlobalDispatch::can_view() unconditionally calls pin_mut() on the UniquePtr<ffi::GlobalFactory>. Since WaylandServer::run() accepts a possibly-null UniquePtr<GlobalFactory> (e.g. the example passes nullptr), this can panic at runtime when can_view is invoked. Consider explicitly handling the null case (e.g. treat a null factory as "no filtering" and return true, or skip registering filtered globals when factory is null) before calling into C++.
| let mut guard = global_data.lock().unwrap(); | |
| let mut guard = global_data.lock().unwrap(); | |
| if guard.is_null() { | |
| return false; | |
| } |
There was a problem hiding this comment.
Nah. You can't have a null GlobalFactory.
| } | ||
|
|
||
| fn can_view(client: Client, global_data: &Arc<Mutex<cxx::UniquePtr<ffi::GlobalFactory>>>) -> bool { | ||
| let interface_name = #interface_name_str.to_string(); |
There was a problem hiding this comment.
can_view() allocates a new String on every call (#interface_name_str.to_string()). Global visibility checks can happen frequently (per client and per global), so this adds avoidable allocation/UTF-8 copying. Consider generating a 'static interface-name constant and/or changing the FFI to accept a borrowed string (e.g. &str/rust::Str) so the check can run without heap allocation.
| let interface_name = #interface_name_str.to_string(); | |
| let interface_name = #interface_name_str; |
There was a problem hiding this comment.
Yeah, that's not unreasonable. This could totally be rust::Str on the C++ side, rather than rust::String.
| let mut can_view_method = | ||
| CppMethod::new("can_view", Some(CppType::Bool), true, false, true, true); | ||
| can_view_method.add_arg(CppArg::new(CppType::String, "interface_name", false)); | ||
| can_view_method.add_arg(CppArg::new( | ||
| CppType::Box("WaylandClientId".to_string()), | ||
| "client_id", | ||
| false, | ||
| )); | ||
| class.add_method(can_view_method); |
There was a problem hiding this comment.
The generated GlobalFactory::can_view() signature takes ownership of client_id as a rust::Box<WaylandClientId>, which forces a heap allocation in Rust for every visibility check and transfers ownership across the FFI boundary. If can_view is expected to be called often, consider switching the API to take a lightweight identifier (e.g. ClientId/u32) or a borrowed reference so the check can be performed without per-call allocation/ownership transfer.
There was a problem hiding this comment.
Wereas this one would be annoying to pass as a reference, I think?
| } | ||
|
|
||
| fn can_view(client: Client, global_data: &Arc<Mutex<cxx::UniquePtr<ffi::GlobalFactory>>>) -> bool { | ||
| let interface_name = #interface_name_str.to_string(); |
There was a problem hiding this comment.
Yeah, that's not unreasonable. This could totally be rust::Str on the C++ side, rather than rust::String.
| let mut can_view_method = | ||
| CppMethod::new("can_view", Some(CppType::Bool), true, false, true, true); | ||
| can_view_method.add_arg(CppArg::new(CppType::String, "interface_name", false)); | ||
| can_view_method.add_arg(CppArg::new( | ||
| CppType::Box("WaylandClientId".to_string()), | ||
| "client_id", | ||
| false, | ||
| )); | ||
| class.add_method(can_view_method); |
There was a problem hiding this comment.
Wereas this one would be annoying to pass as a reference, I think?
| fn can_view(client: Client, global_data: &Arc<Mutex<cxx::UniquePtr<ffi::GlobalFactory>>>) -> bool { | ||
| let interface_name = #interface_name_str.to_string(); | ||
| let client_id = Box::new(WaylandClientId::new(client.id())); | ||
| let mut guard = global_data.lock().unwrap(); |
There was a problem hiding this comment.
Nah. You can't have a null GlobalFactory.
What's new?
wl_display_set_global_filterequivalent on theGlobalFactoryclassChecklist