-
Notifications
You must be signed in to change notification settings - Fork 17
refactor(ui): 在Core中实现了Hint功能, 并修改相关签名 #126
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
Conversation
ae84f39 to
9fa1841
Compare
whitecat346
left a comment
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.
包含了不在该PR作者指定的范围(实现Hint功能)内的更改,超出职权范围
我改改指定范围(可以吧)实在不想重开新PR和分支 |
Pigeon0v0
left a comment
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.
对 LobbyService.cs 的相关修改单开一个 PR,这个 PR 越权了
DotnetInstall
left a comment
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.
首先,直接改名枚举并修改成员(Normal -> Info、Success -> Finish、Error -> Critical)是一个破坏性变更:如果项目中任何地方把这个枚举当做外部 API、序列化字段或被插件/扩展引用,都会导致运行时或编译期错误;即便仓库内你已更新了部分调用点,必须做一次全库全名搜索并写出迁移指南,或者先保留旧枚举作为 Obsolete 的兼容层再做逐步替换,而不是直接替换所有定义。其次,HintWrapper 的委托签名从 HintTheme 改为 HintType 也会导致所有订阅 OnShow 的代码必须同时改动,任何遗漏的订阅点都会造成编译失败或逻辑缺失;你没有在 PR 中提供任何测试或静态分析结果来证明订阅方都已更新。第三,UI 显示逻辑被直接撒在低层逻辑(例如 LobbyController、NatayarkProfileManager、FolderManager、LaunchArgBuilder)中,这违反了关注点分离:控制器/服务层应该抛出或返回错误状态,由上层 UI 负责决定是否提示用户并以何种语气提示;在底层直接调用 HintWrapper.Show 会把 UI 依赖强耦合进了业务逻辑,难以测试且降低了可复用性。第四,异常处理不一致且含糊:你在一些特定的 ArgumentException 情形下同时记录日志并展示提示,这是可以的,但有的 catch 只是记录未提示,有的则提示但没有附带用户可执行的建议或错误码;比如 LobbyController 在找不到 hostname 时仅显示“大厅创建者的用户名为空”,没有给出下一步该如何修复或是否应重试、回退,这会给用户带来困惑。
啊这。。。我还不如重开分支和PR |
|
我不干了,别人写吧awa |
Link: PCL-Community/PCL2-CE#1856
实现了Hint,并更改为与CE部分一致的签名