[RFC]feat(editor): add DragCanvas interaction with customizable trigger keys #191
Conversation
… in drag-element.ts
Summary of ChangesHello @xxMudCloudxx, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 此拉取请求主要引入了一个新的编辑器功能，即通过按住空格键来拖拽画布，显著提升了用户体验。为了确保此功能的稳定性和避免与其他交互冲突，代码中包含了精细的激活条件判断。同时，还解决了一个现有拖拽元素选择逻辑中的竞态条件问题，并对状态管理机制进行了优化，以更好地支持这种高性能的交互模式，同时保持命令历史的整洁。 Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
这个 PR 引入了通过空格键拖拽画布的功能，实现得非常出色，特别是对各种边界情况（如输入框保护、焦点范围限制）的考虑很周全。相关的单元测试也很详尽，覆盖了大部分场景。
我对 DragElement 中为修复 bug 而将选中逻辑后移的做法表示赞同，这解决了互斥锁的竞争问题。
关于讨论中提到的 State 更新策略和类型调整，我进行了一些深入分析，并在代码中发现了一些潜在的风险，主要集中在 UpdateOptionsCommand 的实现上。将类型调整为 Partial 是正确的方向，但这需要对 apply 和 undo 的逻辑进行相应调整，以确保命令的正确性和类型安全。我在具体的 review comment 中给出了详细的解释和建议。
总的来说，这是一个高质量的 PR，在解决我提出的几个关键问题后，代码会更加健壮和清晰。
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #191 +/- ##
==========================================
+ Coverage 44.89% 45.22% +0.33%
==========================================
Files 332 334 +2
Lines 26473 26635 +162
Branches 2081 2115 +34
==========================================
+ Hits 11884 12046 +162
Misses 14579 14579
Partials 10 10
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
建议是交互改名为 |
|
ok我晚点改一下 |
xxMudCloudxx
force-pushed
the
feat/spacebar-drag
branch
from
dbe3aac to
e0f4cf8
Compare
xxMudCloudxx
changed the title
|
/gemini review |
There was a problem hiding this comment.
Code Review
本次 PR 新增了 DragCanvas 交互，允许用户通过可配置的触发键（默认为空格键）来平移画布。该功能的实现很可靠，对事件监听器、焦点和排他性交互锁的处理都非常谨慎。附带的测试也很全面，覆盖了各种激活、拖拽流程和边缘场景。
此外，PR 还包含了一个针对 DragElement 交互的重要 Bug 修复，将选择逻辑推迟到获取排他锁之后执行，这提高了元素拖拽的稳定性和可预见性。
相关的 UpdateOptionsCommand 和 StateManager.updateOptions 的重构，以支持局部更新，也是一个显著的改进。我注意到 StateManager.updateOptions 在处理嵌套的 undefined 值以删除属性时存在一个潜在问题，如果能解决此问题，实现将更加健壮。
总体而言，这是一次出色的功能添加，并对现有代码库进行了深思熟虑的改进。
更改说明(Changes)在后续的commit中我做了以下修改： 缺陷修复 (Bug Fix)修复 功能重构与增强 (Refactoring)
总结
待确认项 (Items for Review)需要维护者确认的： 目前的逻辑流是：在
if ((event.ctrlKey || event.metaKey) && event.shiftKey) {
const command = new UpdateOptionsCommand({
viewBox: undefined,
});
void this.commander.execute(command);
return;
}
updateOptions(options: UpdatableInfographicOptions) {
mergeOptions(this.options, options);
if (this.options.viewBox) {
this.editor.getDocument().setAttribute('viewBox', this.options.viewBox);
} else {
this.editor.getDocument().removeAttribute('viewBox');
setSVGPadding(
this.editor.getDocument(),
parsePadding(this.options.padding),
);
}
// xxx
} |
3 participants
功能
新增
DragCanvas交互，用于自由拖拽画布。默认通过按住 空格键 触发，并支持通过trigger参数自定义触发按键（如ShiftLeft等）。自定义触发按键示例:
为了防止在非预期情况下拦截触发键，代码在
handleKeyDown中做了多重校验：this.isHovering判断鼠标是否在编辑器范围内。只有鼠标悬停在画布上时，按下触发键才会有反应，避免了意外干扰（例如防止用户想滚动网页时却意外触发了画布拖拽）。Bug Fix
关于 DragElement 选择框逻辑的优化：在测试
spacebarDrag时发现：即便spacebarDrag已持有互斥锁，在拖动的过程中仍会触发选中框。经排查，原因是drag-element在尝试竞争锁之前就先执行了select逻辑。因此，我把选中逻辑移到了竞争锁的回调函数中。讨论
均已解决
1. 关于 State 直接更新与 Command 模式的权衡现状：目前的 MVC 架构中，如果
dragCanvas的每一次移动都经过 Command 系统，会导致 Undo/Redo 栈被大量中间状态污染。实现：我采用了“过程直接更新 State，结果合并提交 Command”的策略：
在拖拽过程中直接更新 State 中的
viewBox以确保性能且不留存历史。仅在拖拽完全结束时，对比起止状态并发射一次 Command。
评估：这种处理方式在当前架构中是否可接受？
2. 关于 State 类型调整 (Partial)实现：配合上述修改，为了支持仅更新部分属性（如只更新
viewBox而非全量配置），我将 State 中相关接口的参数类型调整为了Partial<ParsedInfographicOptions>。评估：这一改动是否会破坏架构的设计预期？
改动：
// src/editor/commands/UpdateOptions.ts export class UpdateOptionsCommand implements ICommand { constructor( private options: Partial<ParsedInfographicOptions>, - private original?: ParsedInfographicOptions, + private original?: Partial<ParsedInfographicOptions>, ) {} //... } // src/editor/managers/state.ts - updateOptions(options: ParsedInfographicOptions){} + updateOptions(options: Partial<ParsedInfographicOptions>){} // src/editor/types/state.ts - updateOptions(options: ParsedInfographicOptions): void; + updateOptions(options: Partial<ParsedInfographicOptions>): void;3. 关于 Space 键激活机制的 UX 探讨this.interaction.isActive()才启用 spacebarDrag，但是用户在未点击聚焦的情况下按下 Space 键，预期是拖拽，实际却触发了页面滚动。