-
Notifications
You must be signed in to change notification settings - Fork 21
refactor: extract ToCString trait into separate crate #456
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Conversation
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey - 我发现了 3 个问题,并给出了一些整体反馈:
- enum 辅助方法(COUNT/NAMES/VARIANTS、from_u8/from_str_opt、unsafe transmute)现在在 ClientType、TaskType 和 TouchMode 中都有重复;可以考虑把它们提取到一个小的宏或共享工具里,以减少重复,并把不安全的模式集中管理。
- ClientType 的 serde 实现只支持从字符串反序列化,但序列化时却用的是 u64,这在行为上是不对称的;如果这不是刻意设计的,建议要么像 TaskType/TouchMode 那样也支持数字反序列化,要么改成序列化为字符串,以与预期的输入格式保持一致。
给 AI Agent 的提示
Please address the comments from this code review:
## Overall Comments
- The enum helpers (COUNT/NAMES/VARIANTS, from_u8/from_str_opt, unsafe transmute) are now duplicated across ClientType, TaskType, and TouchMode; consider extracting this into a small macro or shared utility to reduce repetition and keep the unsafe pattern centralized.
- ClientType’s serde implementation deserializes only from string but serializes as a u64, which is asymmetric; if not intentional, consider either supporting numeric deserialization as well (like TaskType/TouchMode) or serializing as a string to match the expected input format.
## Individual Comments
### Comment 1
<location> `crates/maa-types/src/client_type.rs:140-149` </location>
<code_context>
+
+impl std::error::Error for UnknownClientTypeError {}
+
+#[cfg(feature = "serde")]
+impl<'de> serde::Deserialize<'de> for ClientType {
+ fn deserialize<D: serde::Deserializer<'de>>(deserializer: D) -> Result<Self, D::Error> {
+ struct ClientTypeVisitor;
+
+ impl serde::de::Visitor<'_> for ClientTypeVisitor {
+ type Value = ClientType;
+
+ fn expecting(&self, formatter: &mut std::fmt::Formatter) -> std::fmt::Result {
+ formatter.write_str("a valid client type")
+ }
+
+ fn visit_str<E: serde::de::Error>(self, value: &str) -> Result<Self::Value, E> {
+ ClientType::from_str_opt(value)
+ .ok_or_else(|| E::unknown_variant(value, &ClientType::NAMES))
+ }
+ }
+
+ deserializer.deserialize_str(ClientTypeVisitor)
+ }
+}
</code_context>
<issue_to_address>
**issue (bug_risk):** ClientType 以数字形式序列化,但仅从字符串反序列化,导致 serde 的往返兼容性被破坏。
`ClientType` 通过 `serializer.serialize_u64(*self as u64)` 来序列化,但反序列化则通过 `deserialize_str`,且只实现了 `visit_str`。因此由 `Serialize` 产生的值在反序列化时会因 "invalid type: integer ..." 错误而失败。
为了恢复往返兼容性并与 `TaskType`/`TouchMode` 保持一致,可以:
- 把 `Serialize` 改为输出字符串(例如 `serializer.serialize_str(self.to_str())`),或者
- 扩展 visitor 以处理数值输入(例如实现 `visit_u64`,并使用 `deserializer.deserialize_any(ClientTypeVisitor)` 或类似的入口,让整数 token 被路由到 `visit_u64`)。
</issue_to_address>
### Comment 2
<location> `crates/maa-types/src/task_type.rs:151` </location>
<code_context>
- }
- }
-
- deserializer.deserialize_str(TaskTypeVisitor)
- }
-}
</code_context>
<issue_to_address>
**issue (bug_risk):** TaskType 的反序列化器声明为 `deserialize_str`,却试图通过 `visit_u64` 处理整数,在当前配置下 serde 永远不会调用它。
由于入口是 `deserializer.deserialize_str(TaskTypeVisitor)`,serde 在处理数值输入时永远不会调用 `visit_u64`;整数 token 会在到达 visitor 之前就报错。因此用于处理整数的分支实际上是死代码,从 `u64` 反序列化也就无法按预期工作。
为了同时支持字符串和整数表示(并与 `Serialize` 实现保持一致),可以把入口改成类似 `deserializer.deserialize_any(TaskTypeVisitor)` 的形式(或者使用一个包装器,依次尝试 `deserialize_u64` 和 `deserialize_str`),这样数值 token 才能真正被路由到 `visit_u64`。
</issue_to_address>
### Comment 3
<location> `crates/maa-types/src/touch_mode.rs:137` </location>
<code_context>
- }
- }
-
- deserializer.deserialize_str(TouchModeVisitor)
- }
-}
</code_context>
<issue_to_address>
**issue (bug_risk):** TouchMode 的反序列化器同样无法真正消费数值 token,尽管实现了 `visit_u64`。
`TouchModeVisitor` 实现了 `visit_u64`,但 `Deserialize` 使用的是 `deserializer.deserialize_str(TouchModeVisitor)`,所以整数 token 永远不会到达 `visit_u64`。这很可能会破坏与 `Serialize` 实现(输出 `u64`)之间的往返行为。
为了启用数值分支并仍然支持字符串,应切换为 `deserializer.deserialize_any(TouchModeVisitor)`(或其他可以接受字符串和整数 token 的入口)。
</issue_to_address>帮我变得更有用!请在每条评论上点 👍 或 👎,我会根据你的反馈改进后续的代码审查。
Original comment in English
Hey - I've found 3 issues, and left some high level feedback:
- The enum helpers (COUNT/NAMES/VARIANTS, from_u8/from_str_opt, unsafe transmute) are now duplicated across ClientType, TaskType, and TouchMode; consider extracting this into a small macro or shared utility to reduce repetition and keep the unsafe pattern centralized.
- ClientType’s serde implementation deserializes only from string but serializes as a u64, which is asymmetric; if not intentional, consider either supporting numeric deserialization as well (like TaskType/TouchMode) or serializing as a string to match the expected input format.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The enum helpers (COUNT/NAMES/VARIANTS, from_u8/from_str_opt, unsafe transmute) are now duplicated across ClientType, TaskType, and TouchMode; consider extracting this into a small macro or shared utility to reduce repetition and keep the unsafe pattern centralized.
- ClientType’s serde implementation deserializes only from string but serializes as a u64, which is asymmetric; if not intentional, consider either supporting numeric deserialization as well (like TaskType/TouchMode) or serializing as a string to match the expected input format.
## Individual Comments
### Comment 1
<location> `crates/maa-types/src/client_type.rs:140-149` </location>
<code_context>
+
+impl std::error::Error for UnknownClientTypeError {}
+
+#[cfg(feature = "serde")]
+impl<'de> serde::Deserialize<'de> for ClientType {
+ fn deserialize<D: serde::Deserializer<'de>>(deserializer: D) -> Result<Self, D::Error> {
+ struct ClientTypeVisitor;
+
+ impl serde::de::Visitor<'_> for ClientTypeVisitor {
+ type Value = ClientType;
+
+ fn expecting(&self, formatter: &mut std::fmt::Formatter) -> std::fmt::Result {
+ formatter.write_str("a valid client type")
+ }
+
+ fn visit_str<E: serde::de::Error>(self, value: &str) -> Result<Self::Value, E> {
+ ClientType::from_str_opt(value)
+ .ok_or_else(|| E::unknown_variant(value, &ClientType::NAMES))
+ }
+ }
+
+ deserializer.deserialize_str(ClientTypeVisitor)
+ }
+}
</code_context>
<issue_to_address>
**issue (bug_risk):** ClientType serializes as a number but only deserializes from a string, breaking round‑trip serde compatibility.
`ClientType` is serialized via `serializer.serialize_u64(*self as u64)` but deserialized via `deserialize_str` with only `visit_str` implemented. A value produced by `Serialize` will therefore fail to deserialize with an "invalid type: integer ..." error.
To restore round‑trip compatibility and match `TaskType`/`TouchMode`, either:
- change `Serialize` to output a string (e.g. `serializer.serialize_str(self.to_str())`), or
- extend the visitor to handle numeric input (e.g. implement `visit_u64` and use `deserializer.deserialize_any(ClientTypeVisitor)` or a similar entry point that routes integers to `visit_u64`).
</issue_to_address>
### Comment 2
<location> `crates/maa-types/src/task_type.rs:151` </location>
<code_context>
- }
- }
-
- deserializer.deserialize_str(TaskTypeVisitor)
- }
-}
</code_context>
<issue_to_address>
**issue (bug_risk):** TaskType deserializer is declared as `deserialize_str` but tries to handle integers via `visit_u64`, which serde will never call in this configuration.
Because the entry point is `deserializer.deserialize_str(TaskTypeVisitor)`, serde will never call `visit_u64` for numeric input; integer tokens will error out before reaching the visitor. So the integer-handling branch is effectively dead and deserialization from `u64` won’t work as intended.
To support both string and integer representations (and match the `Serialize` implementation), change the entry point to something like `deserializer.deserialize_any(TaskTypeVisitor)` (or use a wrapper that tries both `deserialize_u64` and `deserialize_str`) so numeric tokens are actually routed to `visit_u64`.
</issue_to_address>
### Comment 3
<location> `crates/maa-types/src/touch_mode.rs:137` </location>
<code_context>
- }
- }
-
- deserializer.deserialize_str(TouchModeVisitor)
- }
-}
</code_context>
<issue_to_address>
**issue (bug_risk):** TouchMode deserializer similarly cannot actually consume numeric tokens despite implementing `visit_u64`.
`TouchModeVisitor` implements `visit_u64`, but `Deserialize` uses `deserializer.deserialize_str(TouchModeVisitor)`, so integer tokens never reach `visit_u64`. This likely breaks round-tripping with the `Serialize` impl, which emits `u64`.
To use the numeric branch and still support strings, switch to `deserializer.deserialize_any(TouchModeVisitor)` (or another entry point that accepts both string and integer tokens).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
178a73c to
e1f44cc
Compare
e1f44cc to
ea0d04a
Compare
This was referenced Jan 4, 2026
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.
Summary by Sourcery
引入共享的 FFI 字符串工具 crate,将触摸与任务类型重构到独立模块并增加客户端类型支持,同时更新绑定和错误处理以使用新的抽象。
Enhancements:
maa-ffi-stringcrate,用于集中管理安全的CString转换,并在 FFI 边界间复用。TouchMode和TaskType定义重构到单独的maa-types模块中,并通过 crate 根对外暴露,同时添加 schema、serde、clap 和 FFI 集成钩子。ClientType枚举,支持解析、序列化、clap 集成,以及用于包、资源和服务器元数据的辅助方法。maa-sys的 FFI 绑定,复用std::ffi中的c_char/c_void别名,并更新错误类型,以区分无效参数和无效返回值。Build:
maa-ffi-string添加为 workspace crate,并接入maa-sys和maa-types的依赖中,相应提升 crate 版本并启用新的 feature flags。maa-types新增可选的clap、maa-ffi-string、schemars和serdefeature flags,以支持 CLI、FFI 和 schema。Original summary in English
Summary by Sourcery
Introduce a shared FFI string utility crate, refactor touch and task types into dedicated modules with additional client type support, and update bindings and error handling to use the new abstractions.
Enhancements:
Build: