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(extension): 【dynamic-group】DynamicGroup 使用 moveNode 进行移动时子节点没有跟随移动 #1963

Closed
wants to merge 4 commits into from

Conversation

ZivvW
Copy link
Contributor

@ZivvW ZivvW commented Nov 15, 2024

#1858 中,分组内节点跟随移动的逻辑被放在了 node:mousemove 事件监听中,导致只有拖拽才会触发该逻辑。对分组使用 Api 如 moveNode 时,不会触发这个事件,子节点不跟随。
现将跟随移到逻辑改到 getMoveDistance 中,拖拽和使用方法进行移动时均会执行该方法。

同时,selection-select 插件中有问题,在选择到分组节点时,会将子节点也选中,导致在做了上面的修改后,子节点会移动 n 次。
selection-select 的代码里有对分组情况的处理:

        // 如果节点属于分组,则不选中节点,此处兼容旧版 Group 插件
        if (!group || !group.getNodeGroup(element.id)) {
          this.lf.selectElementById(element.id, true)
        }
        // 如果节点属于动态分组,则不不选中节点
        if (!dynamicGroup || !dynamicGroup.getGroupByNodeId(element.id)) {
          this.lf.selectElementById(element.id, true)
        }

这样的判断在单独使用分组插件时,会有另一个 if 触发,选中节点。现在修复了该逻辑,不会影响到分组的移动

效果:
chrome-capture-2024-11-15

这样处理后也能修复 #1949 ,这种场景下,鼠标没有移动,所以不触发事件
效果:
chrome-capture-2024-11-15 (1)

wzw added 3 commits November 13, 2024 16:41
之前的修改中,跟随移动的逻辑被放在了 node:mousemove 事件监听中,导致只有拖拽才会触发,现改到 getMoveDistance 中,拖拽和使用方法进行移动均会执行
DynamicGroup 的子元素跟随移动逻辑中,获取了所有子元素,导致多次触发移动逻辑
…移动逻辑重复

之前的框选组件中,不选中分组节点的 if 逻辑有问题,在使用单个分组插件时,if (!group) 这样的判断会一直触发,导致会选中所有节点
Copy link

changeset-bot bot commented Nov 15, 2024

⚠️ No Changeset found

Latest commit: 6cdf0a2

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@wbccb
Copy link
Contributor

wbccb commented Nov 20, 2024

#1911 (comment)所说,所以你这个pr是如何解决moveNodes()的实际移动dx和dy,和mousemove事件emit出去dx和dy的不一致问题的?

以及在这个#1858 中,我使用node:mousemove 事件代替addNodeMoveRules(),是因为我觉得addNodeMoveRules()这个方法字面上的意思就是一个规则的判断,不应该移动节点,目前使用getMoveDistance()addNodeMoveRules()本质也是很难理解(不应该移动节点) + 它们都是同样链路获取dxdy,为什么要采用新的模式getMoveDistance()而不是恢复1.0版本就使用的addNodeMoveRules()逻辑呢?

@wbccb
Copy link
Contributor

wbccb commented Nov 20, 2024

而对于moveNode这个API,是否需要同时让子节点跟随这个我是存在疑问的,因为这个API的字面意思就是移动节点,为什么还要让内部的子节点跟随?是不是基于moveNode增加一些参数,比如moveNode(dx, dy, needChildFollow)这样更为恰当点?

@ZivvW
Copy link
Contributor Author

ZivvW commented Nov 20, 2024

#1911 (comment)所说,所以你这个pr是如何解决moveNodes()的实际移动dx和dy,和mousemove事件emit出去dx和dy的不一致问题的?

改成这样的话就和 mousemove事件 没关系了
移动流程如下:moveNodes 中调用 DynamicGroupgetMoveDistance 从而移动,而 getMoveDistance 里拿到的就是实际移动的 dx dy,再用这个 dx dy让子节点进行移动

以及在这个#1858 中,我使用node:mousemove 事件代替addNodeMoveRules(),是因为我觉得addNodeMoveRules()这个方法字面上的意思就是一个规则的判断,不应该移动节点,目前使用getMoveDistance()addNodeMoveRules()本质也是很难理解(不应该移动节点) + 它们都是同样链路获取dxdy,为什么要采用新的模式getMoveDistance()而不是恢复1.0版本就使用的addNodeMoveRules()逻辑呢?

getMoveDistance() 虽然翻译过来是获取移动距离,但在实际使用中,它是用来修改节点自身的位置的。

    if (isAllowMoveX && deltaX) {
      this.x = this.x + deltaX
      this.text && this.moveText(deltaX, 0)
      moveX = deltaX
    }
    if (isAllowMoveY && deltaY) {
      this.y = this.y + deltaY
      this.text && this.moveText(0, deltaY)
      moveY = deltaY
    }

可以看到,节点的移动是通过,在这里面进行 x y的修改进行的。
这个方法的命名可能有点问题,如果按它实际的用法来看,在这里面执行子节点的移动是比较符合逻辑的

@wbccb
Copy link
Contributor

wbccb commented Nov 20, 2024

你没理解我的问题,我的意思是,一个节点A,你实际moveNode()dx=1dy=1,但是你mousemove事件出去告诉其他开发者,我这个节点的mousemove的是dx=2dy=2,其它开发者如果使用你这个事件去做一些事情,得到的就是错误的dxdy

@ZivvW
Copy link
Contributor Author

ZivvW commented Nov 20, 2024

你没理解我的问题,我的意思是,一个节点A,你实际moveNode()dx=1dy=1,但是你mousemove事件出去告诉其他开发者,我这个节点的mousemove的是dx=2dy=2,其它开发者如果使用你这个事件去做一些事情,得到的就是错误的dxdy

嗯,事件这里确实有问题。针对 #1911 的情况,这个 pr 相当于是绕开了这个问题

@ZivvW
Copy link
Contributor Author

ZivvW commented Nov 20, 2024

而对于moveNode这个API,是否需要同时让子节点跟随这个我是存在疑问的,因为这个API的字面意思就是移动节点,为什么还要让内部的子节点跟随?是不是基于moveNode增加一些参数,比如moveNode(dx, dy, needChildFollow)这样更为恰当点?

我是觉得 moveNode api 和拖拽的行为保持一致比较好。在之前的版本中,moveNode 是会让子节点进行跟随的。

添加参数的话,moveNode 作为主系统的 api,而分组节点作为一个插件,如果在moveNode上增加专门为插件定制的参数的话,不知道是否合适。

@wbccb
Copy link
Contributor

wbccb commented Nov 28, 2024

@boyongjiong
👀有空给人家review一下啊,我只是提了一嘴

@boyongjiong
Copy link
Collaborator

@boyongjiong 👀有空给人家review一下啊,我只是提了一嘴

嗯嗯,我正在想怎么解决你提出的问题,我考虑一下,在看了😁

@boyongjiong
Copy link
Collaborator

我小丑了。。我用 github 的工具处理了一下冲突,结果它把 master 分支 merge 到你的代码分支了。

  1. moveNodes 这个 API,我认为 「同时让子节点跟随」 是比较合理的操作。如果移动分组,但分组内的元素保持不动,感觉像是 bug

  2. getMoveDistance 这个 api 中进行了节点移动,这个操作是否合理?像 @wbccb 说的,看名字感觉不太合适,但我看了下代码,感觉又没有什么毛病,没想出什么更好的方法 0.o (我太弱了-。-)

  3. moveNode 实际移动的 dx、dy,与 mousemove 事件 emit 出去 dx 和 dy 的不一致问题,我再看看新开一个 pr 处理吧,这个 PR 我看不存在这样的问题。

再次道歉,拖了这么久。最近一直在忙文章和述职的事情,脑子不够使了,拖了好久。感谢大佬的贡献 @ZivvW

因为一会儿着急发版本,我用你的代码重新提了个 pr -> #2007 ,你的产出还是在 commit 里哈

@boyongjiong
Copy link
Collaborator

代码已合并哈

@ZivvW
Copy link
Contributor Author

ZivvW commented Dec 12, 2024

好的

@ZivvW ZivvW deleted the fix/group-all-move-by-api branch December 14, 2024 09:01
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