Repository navigation
Conversation
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: glyvut The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideUpdates the snap-target protocol XML to report invalid seat arguments separately from busy snap sessions and aligns the surrounding start, failure event, and surface documentation with the clarified behavior. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="public/treeland-snap-target-unstable-v1.xml" line_range="29" />
<code_context>
1. Raises the surface above normal windows and their popup panels.
2. Disables window open/close animations.
3. Excludes the surface from the multitask view and window switcher.
- 4. Disables automatic positioning by the compositor's layout engine.
Such a surface does not affect the snapping protocol; snapping
</code_context>
<issue_to_address>
**nitpick:** Removing item 4 makes the protocol documentation no longer state that snap-mask surfaces are excluded from automatic compositor positioning, even though the surrounding list still presents the remaining items as the surface configuration applied by the compositor.
**Suggested fix:** Restore the automatic-positioning item unless that compositor behavior was intentionally removed as part of this change.
</issue_to_address>
### Comment 2
<location path="public/treeland-snap-target-unstable-v1.xml" line_range="123-127" />
<code_context>
<description summary="reasons why snapping failed"/>
<entry name="snap_busy" value="0"
summary="another client has an active snap session"/>
+ <entry name="invalid_seat" value="1"
+ summary="the seat argument is not a valid wl_seat"/>
</enum>
</code_context>
<issue_to_address>
**issue (bug_risk):** A non-null `object` argument declared with `interface="wl_seat"` is type-checked by the Wayland dispatcher before `start` is delivered; an unknown, null, or non-`wl_seat` object causes a protocol error rather than a `failed` event with `invalid_seat`. The new reason therefore cannot distinguish the malformed `wl_seat` arguments described by its summary.
**Triggers:** When the client passes an unknown, null, destroyed, or object of another interface as `seat`.
**Suggested fix:** Define `invalid_seat` in terms of a compositor-level seat validation failure for an otherwise valid `wl_seat`, or document that malformed object arguments are protocol errors and cannot produce this event.
```suggestion
If snapping cannot be started because another client's snap session
is already active, the compositor sends a failed event with the
snap_busy reason instead. If the seat argument is unknown, null, or
not a wl_seat, the request causes a protocol error and no failed event
is sent. If compositor-level validation of an otherwise valid wl_seat
fails, the compositor sends a failed event with the invalid_seat
reason.
```
</issue_to_address>Removed unnecessary wording from the snap-target protocol spec: the auto-positioning exclusion clause, the seat policy note, and the snap_busy explanation. 精简snap-target协议规范文本:移除自动定位排除条款、seat策略说明和snap_busy解释。 Log: 精简snap-target协议规范文本 Influence: 协议规范文本更精简,不影响协议接口的功能和语义。
|
TAG Bot New tag: 0.7.0 |
Summary by Sourcery
Refresh the unstable snap-target protocol documentation to reflect current behavior.
Enhancements:
Documentation: