Conversation
There was a problem hiding this comment.
Pull Request Overview
Refactors the i3 backend to use a Node struct with serde_json deserialization, replacing the old manual Value–based logic and consolidating JSON fixtures with schema validation.
- Introduced
backend/i3/json.rswithNodeand methods for tabs/visibility. - Removed the old
compass.rsand updatedBackendto parse directly intoNode. - Moved test JSON fixtures into
rust/jsons/, addednode.jsonschema, and wired schema checks into the Makefile.
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| rust/src/types/rect.rs | Added conditional Deserialize derive for Rect when i3 feature is enabled |
| rust/src/backend/i3/mod.rs | Renamed compass module to json |
| rust/src/backend/i3/json.rs | New Node struct with serde parsing, tree-traversal methods, and tests |
| rust/src/backend/i3/compass.rs | Removed old manual JSON parsing implementation |
| rust/src/backend/i3/backend.rs | Updated to parse i3 tree into Node and use its methods |
| rust/jsons/*.json | Moved test fixtures out to dedicated files |
| rust/jsons/node.jsonschema | Added JSON schema to validate test fixtures |
| rust/Makefile | Added schema target to run check-jsonschema before tests |
| rust/Cargo.toml.in | Added optional serde dependency under the i3 feature |
Comments suppressed due to low confidence (1)
rust/src/types/rect.rs:11
- [nitpick] You can merge these two derives into one attribute:
#[derive(Deserialize, Debug, Clone, Copy, PartialEq, Eq)]for clearer code.
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
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
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.
Switched to serde_json struct parsing.
Now we have a Node struct that requires most of the fields, which are present in i3 tree, to be present in source json.
The main benefit is much clearer and simpler logic, with a bunch of unwrapping removed.
The downside though is that test jsons growed substantialy, so I've decided to move the jsons to their own files. This however allowed to verify them with check-jsonschema, which double checks that we're not testing on incorrect jsons.
I'm pretty confident in the result. Performance gains are minimal if any (under 10%). XCB still remains the performance leader. However it's definietely a move in the right direction as far as compliance with common practices go. And honestly. Not losing on performance is already a pretty big win.
This will close #20