Skip to content
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

Fix: Reset rendering after deleting all nodes #6855

Open
wants to merge 8 commits into
base: v5
Choose a base branch
from

Conversation

ouy-o3o
Copy link

@ouy-o3o ouy-o3o commented Mar 11, 2025

No description provided.

Copy link

Walkthrough

This pull request introduces a fix for deleting the last node in a graph. It includes changes to the graph rendering logic to reset the rendering state when all nodes are destroyed. Additionally, it adds a test case to verify the deletion functionality and updates the demo and unit test files accordingly.

Changes

File Summary
packages/g6/tests/demos/element-delete-last-node.ts Added a new test case for deleting the last node in a graph.
packages/g6/tests/demos/index.ts Exported the new test case from the demos index.
packages/g6/tests/snapshots/behaviors/delete-last-node/deleteLastNode.svg Added a snapshot for the delete last node behavior.
packages/g6/tests/unit/behaviors/delete-last-node.spec.ts Added a unit test for the delete last node functionality.
packages/g6/src/runtime/element.ts Modified to reset rendering state when all nodes are destroyed.
packages/g6/src/runtime/graph.ts Added a method to reset the rendering engine status.
🪧 Tips For further assistance, please describe your question in the comments and @petercat-assistant to start a conversation with me.

panel
.add(
{
Remove: () => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function name 'Remove' is misleading here as it actually adds a node. Consider renaming it to 'Add' for clarity.

@ouy-o3o ouy-o3o changed the title Fixdelete last node Fix: Reset rendering after deleting all nodes Mar 11, 2025
@zhongyunWan zhongyunWan requested a review from Aarebecca March 11, 2025 08:01
@ouy-o3o ouy-o3o closed this Mar 11, 2025
@ouy-o3o ouy-o3o reopened this Mar 11, 2025
// <zh/> 如果所有节点都销毁了,重置渲染状态
// <en/> If all nodes are destroyed, reset the rendering state
if (this.getNodes().length === 0) {
this.context.graph.reSetRenderingState();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

建议把相关逻辑写到 destroyElementafter 中,每销毁一个节点后,就清除该节点相关的数据

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

建议把相关逻辑写到 destroyElementafter 中,每销毁一个节点后,就清除该节点相关的数据

已完成 帮忙看看呢

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ouy-o3o 没有看到相关变更呢

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Aarebecca 6d4c3dc 这个呢

@ouy-o3o ouy-o3o requested a review from Aarebecca March 12, 2025 07:20
* <zh/> 重置渲染引擎状态
* <en/> Reset the rendering engine status
*/
public reSetRenderingState(): void {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

名字就用 resetXxx 吧,reset 事一个单词。

Copy link
Author

@ouy-o3o ouy-o3o Mar 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

名字就用 resetXxx 吧,reset 事一个单词。

好的 已修改~

* <zh/> 重置渲染引擎状态
* <en/> Reset the rendering engine status
*/
public reSetRenderingState(): void {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

可以不用添加这个 API

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get! 直接在内部完成了 感谢指点

@ouy-o3o ouy-o3o requested review from hustcc and Aarebecca March 18, 2025 02:31
@Aarebecca
Copy link
Contributor

@ouy-o3o 我刚确认了下,after 中 clearElement 方法会执行相关清理操作,现在额外调用的 clear 是为了解决什么问题吗

@ouy-o3o
Copy link
Author

ouy-o3o commented Mar 19, 2025

Aarebecca

@Aarebecca 经过测试发现 clear这一步操作确实是无效的 已经删除

@Aarebecca
Copy link
Contributor

@ouy-o3o 那剩下调用 this.context.graph.draw(); 的目的是?

@ouy-o3o
Copy link
Author

ouy-o3o commented Mar 24, 2025

@Aarebecca 我今天尝试复现具体问题 这个pr是为了修复 #6828 却发现本地无法复现这个问题了 半个月前还可以稳定复现 直到我找到这个pr #6702 看起来是同一个问题 却不知道为什么半个月前可以复现..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants