Closed
Conversation
Added documentation for the 'slct' mod type, including its source example.
📝 WalkthroughWalkthroughAdds KH2 "slct" (select-menu) support: new Slct and ChoiceEntry types with binary serialization, patcher handling to merge slct listpatches, unit tests for listpatch behavior, and documentation updates including a slct source example. Changes
Sequence Diagram(s)sequenceDiagram
participant ModFile as Mod YAML (slct)
participant Patcher as PatcherProcessor
participant BarStream as Game Bar Stream
participant Serializer as Kh2.Slct (BaseTable)
ModFile->>Patcher: provide slct entries
BarStream->>Patcher: open existing slct data
Patcher->>Serializer: Read existing list from stream
Serializer-->>Patcher: existing List<Slct>
Patcher->>Serializer: Deserialize mod YAML -> List<Slct>
Patcher->>Patcher: Merge by Id (update or append)
Patcher->>Serializer: Write merged List<Slct> to stream (version=2)
Serializer-->>BarStream: write bytes
Patcher-->>ModFile: patch complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
OpenKh.Tests/Patcher/PatcherTests.cs (1)
3190-3274: Consider assertingChoiceandPaddingcontents as well.
Right now the test only checks Id/ChoiceNum/ChoiceDefault; validating choice entries and padding would better guard against serialization regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@OpenKh.Tests/Patcher/PatcherTests.cs` around lines 3190 - 3274, In ListPatchSlctTest, extend the assertions after reading slctStream to validate the Choice and Padding arrays: verify slctStream[0].Choice has length 4 and each element is a Kh2.ChoiceEntry with Id == 0 and MessageId == 0, and verify slctStream[0].Padding has length 25 and contains the expected zero bytes (or the exact values written); update the test (in the ListPatchSlctTest method, referencing slctStream, Choice and Padding) to include these checks to catch serialization regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/tool/GUI.ModsManager/creatingMods.md`:
- Around line 868-892: The fenced code block under the "slct Source Example"
should declare the language to satisfy markdownlint MD040; update the
triple-backtick fence that currently contains the `slct` YAML example to use a
language identifier (e.g., ```yaml) so the block is highlighted and linted
correctly, leaving the inner content (fields like Id, ChoiceNum, ChoiceDefault,
BaseSequence, TitleSequence, Padding, etc.) unchanged.
---
Nitpick comments:
In `@OpenKh.Tests/Patcher/PatcherTests.cs`:
- Around line 3190-3274: In ListPatchSlctTest, extend the assertions after
reading slctStream to validate the Choice and Padding arrays: verify
slctStream[0].Choice has length 4 and each element is a Kh2.ChoiceEntry with Id
== 0 and MessageId == 0, and verify slctStream[0].Padding has length 25 and
contains the expected zero bytes (or the exact values written); update the test
(in the ListPatchSlctTest method, referencing slctStream, Choice and Padding) to
include these checks to catch serialization regressions.
Comment on lines
868
to
892
| ### `slct` Source Example | ||
| ``` | ||
| - Id: 1 | ||
| ChoiceNum: 4 #Amount of options | ||
| ChoiceDefault: 3 #Option to default to | ||
| Choice: | ||
| - Id: 0 #Choice "Function" | ||
| MessageId: 0 #MessageID to assign to the Choice | ||
| - Id: 1 | ||
| MessageId: 1 | ||
| - Id: 2 | ||
| MessageId: 2 | ||
| - Id: 3 | ||
| MessageId: 3 | ||
| BaseSequence: 12 #Signed short, can be negative | ||
| TitleSequence: 13 #Signed short, can be negative | ||
| Information: 14 | ||
| EntryId: 15 | ||
| Task: 16 | ||
| PauseMode: 17 | ||
| Flag: 18 | ||
| SoundPause: 19 | ||
| Padding: #There are 25 padding bytes in total | ||
| - 0 | ||
| ``` |
There was a problem hiding this comment.
Add a language identifier to the slct YAML fence.
markdownlint MD040 flags the fenced block without a language.
💡 Suggested fix
-```
+```yaml
- Id: 1
ChoiceNum: 4 `#Amount` of options
ChoiceDefault: 3 `#Option` to default to
Choice:
- Id: 0 `#Choice` "Function"
MessageId: 0 `#MessageID` to assign to the Choice
- Id: 1
MessageId: 1
- Id: 2
MessageId: 2
- Id: 3
MessageId: 3
BaseSequence: 12 `#Signed` short, can be negative
TitleSequence: 13 `#Signed` short, can be negative
Information: 14
EntryId: 15
Task: 16
PauseMode: 17
Flag: 18
SoundPause: 19
Padding: `#There` are 25 padding bytes in total
- 0🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 869-869: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/tool/GUI.ModsManager/creatingMods.md` around lines 868 - 892, The fenced
code block under the "slct Source Example" should declare the language to
satisfy markdownlint MD040; update the triple-backtick fence that currently
contains the `slct` YAML example to use a language identifier (e.g., ```yaml) so
the block is highlighted and linted correctly, leaving the inner content (fields
like Id, ChoiceNum, ChoiceDefault, BaseSequence, TitleSequence, Padding, etc.)
unchanged.
Clarified comments for PauseMode and Flag fields.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/tool/GUI.ModsManager/creatingMods.md`:
- Around line 890-892: Clarify the semantics of the "Padding" entry: update the
documentation around the "Padding" example (the line showing "- 0") to state
explicitly whether partial specification is allowed and how it's handled — if
partial lists are accepted, say "you may provide N bytes and the remaining bytes
will be zero-padded to a total of 25"; otherwise require authors to list all 25
bytes and replace the truncated example with a complete 25-byte list. Ensure the
text references the "Padding" field and the sample "- 0" so readers know which
behavior applies.
- Around line 884-885: The inline comments for the fields Information and
EntryId are inconsistent/unfinished; update the examples so they match their
comments: either change Information's example value from 14 to 0 if the note
“Value tends to always be 0” is correct, or update the comment to explain why 14
is shown (e.g., what 14 represents) so the example and note agree; also remove
the stray trailing “#” after EntryId or replace it with a concise comment
explaining EntryId (e.g., “# unique entry identifier”) so the annotation is not
left empty—edit the lines containing the Information and EntryId fields in the
docs to implement these changes.
---
Duplicate comments:
In `@docs/tool/GUI.ModsManager/creatingMods.md`:
- Line 869: The fenced code block starting at the noted location lacks a
language identifier (causing markdownlint MD040); update the opening fence from
``` to ```yaml so the block is syntax-highlighted and linted correctly (e.g.,
change the block that contains the snippet with "Id: 1" and "ChoiceNum: 4" to
start with ```yaml).
Comment on lines
+884
to
+885
| Information: 14 #Value tends to always be 0? | ||
| EntryId: 15 # |
There was a problem hiding this comment.
Resolve contradictory/incomplete inline comments on Information and EntryId.
- Line 884: The example value is
14, but the comment reads#Value tends to always be 0?. This directly contradicts the example and will confuse mod authors. Either use0as the example value (matching the stated expectation) or update the comment to reflect why14is shown. - Line 885:
EntryId: 15 #has a trailing#with no comment body — the annotation was started but never completed.
📝 Proposed fix
- Information: 14 `#Value` tends to always be 0?
- EntryId: 15 #
+ Information: 0 `#Value` tends to always be 0
+ EntryId: 15 #<describe what EntryId controls>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Information: 14 #Value tends to always be 0? | |
| EntryId: 15 # | |
| Information: 0 `#Value` tends to always be 0 | |
| EntryId: 15 #<describe what EntryId controls> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/tool/GUI.ModsManager/creatingMods.md` around lines 884 - 885, The inline
comments for the fields Information and EntryId are inconsistent/unfinished;
update the examples so they match their comments: either change Information's
example value from 14 to 0 if the note “Value tends to always be 0” is correct,
or update the comment to explain why 14 is shown (e.g., what 14 represents) so
the example and note agree; also remove the stray trailing “#” after EntryId or
replace it with a concise comment explaining EntryId (e.g., “# unique entry
identifier”) so the annotation is not left empty—edit the lines containing the
Information and EntryId fields in the docs to implement these changes.
Comment on lines
+890
to
+892
| Padding: #There are 25 padding bytes in total | ||
| - 0 | ||
| ``` |
There was a problem hiding this comment.
Clarify whether partial Padding is supported or if the example is intentionally truncated.
The comment states "There are 25 padding bytes in total" but the example supplies only one (- 0). It's unclear whether:
- the reader/patcher accepts a partial list (padding the rest to zero automatically), or
- the example is simply incomplete and mod authors must specify all 25 bytes.
If partial specification is valid, a brief note to that effect would prevent authors from accidentally writing malformed entries.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/tool/GUI.ModsManager/creatingMods.md` around lines 890 - 892, Clarify
the semantics of the "Padding" entry: update the documentation around the
"Padding" example (the line showing "- 0") to state explicitly whether partial
specification is allowed and how it's handled — if partial lists are accepted,
say "you may provide N bytes and the remaining bytes will be zero-padded to a
total of 25"; otherwise require authors to list all 25 bytes and replace the
truncated example with a complete 25-byte list. Ensure the text references the
"Padding" field and the sample "- 0" so readers know which behavior applies.
Contributor
Author
|
Closing, will be superseded by a PR that includes this among some other ones. |
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
1 participant
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.
Adds Slct Listpatching for the file 14mission.bar, documentation, & tests.
Summary by CodeRabbit
New Features
Documentation
Tests