Add MCP event log for processed window events#11665
Open
tilladam wants to merge 13 commits into
Open
Conversation
tronical
requested changes
May 15, 2026
| /// This is used by the system testing module. Returns a previously set hook, if any. | ||
| pub fn set_window_event_hook( | ||
| hook: Option< | ||
| Box< |
Member
There was a problem hiding this comment.
Please introduce an alias for this - it's rather hard to read when formatted :)
| }; | ||
| if let Some(event_for_hook) = event_for_hook { | ||
| // Take the hook out before calling to allow re-entrant dispatch without a BorrowMutError. | ||
| let hook = self.0.context().0.window_event_hook.borrow_mut().take(); |
Member
There was a problem hiding this comment.
I'd say it would be not to take() but keep the borrow and instead let the hook installation error out if replace fails. And for the borrow to work, the function should be Fn and not FnMut IMO. Or why is FnMut needed?
| /// The event was processed, but the runtime does not expose a more specific result. | ||
| Processed, | ||
| /// The event was accepted by an item or runtime handler. | ||
| Accepted, |
Member
There was a problem hiding this comment.
What really is the difference between processed and accepted? It seems to be that either the event was dispatched or it wasn't. What other outcome could be interesting to the caller?
| optional_fields: &["eventType"], | ||
| }, | ||
| ToolDef { | ||
| name: "get_event_log", |
Member
There was a problem hiding this comment.
I have the feeling that recording should be triggered, i.e. have a request to start recording, followed by a request to stop recording, and the stop returns the log. Then there's no need for a clear and by default the MCP wouldn't accumulate memory from the event loop - only if explicitly requested.
It's like a profiler - usually you press a button when to start profiling and then you stop it :)
Four changes based on tronical's review: 1. Add `WindowEventHook` type alias for the complex hook function type, simplifying `SlintContextInner`, `set_window_event_hook`, and callers. 2. Remove `WindowEventDispatchResult::Accepted`. Only `CloseRequested` ever produced it (when the window actually closed), making it effectively a synonym for `Processed`. Collapse it to `Processed` throughout, including the proto `RecordedEventResult` enum (renumbering `Ignored` from 2 to 1) and the conversion in introspection. 3. Change `WindowEventHook` from `FnMut` to `Fn`. The hook is now invoked via a shared borrow, allowing re-entrant dispatch without the take-and- restore dance. `set_window_event_hook` uses `try_borrow_mut` to return a `PlatformError` rather than panicking if called from within a hook. The chaining closure in `ensure_event_tracking` no longer needs a `RefCell` around the previous hook. 4. Replace the always-on event log with an explicit start/stop recording model. `IntrospectionState` gains a `recording_enabled` flag (off by default) guarding `record_window_event`. Two new methods—`start_recording` (clear + enable) and `stop_recording` (disable + drain)—are exposed via `dispatch::start_event_recording` / `dispatch::stop_event_recording`. New proto messages `RequestStartEventRecording`, `StartEventRecordingResponse`, `RequestStopEventRecording`, and `StopEventRecordingResponse` drive both the binary systest and MCP transports. The MCP tools `get_event_log` and `clear_event_log` are replaced by `start_event_recording` and `stop_event_recording`. For binary systest backward-compatibility, `systest::init` calls `start_recording` automatically.
- api.rs: collapse nested `if let` into a single `if let … && let …` to satisfy clippy::collapsible-if (-D warnings in CI) - introspection/mod.rs: remove the `event_log` and `clear_event_log` free-function wrappers that were left behind when the MCP tools were replaced by `start_event_recording`/`stop_event_recording`; the underlying `IntrospectionState` methods are still used in tests
These were incorrectly removed as dead code — systest.rs uses them for the binary system-testing transport (RequestEventLog and RequestClearEventLog).
query_event_log, dispatch::event_log, and dispatch::clear_event_log are only called by systest.rs which requires system-testing. Without the cfg guard the docs build (mcp only, no system-testing) flags them as dead code with -D warnings.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Testing