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.

Sign up for GitHub

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

Jump to bottom

Incorrect merging of Functional Groups #2110

Closed
yuleicul opened this issue Jan 19, 2023 · 1 comment · Fixed by #2179
Closed

Incorrect merging of Functional Groups #2110

yuleicul opened this issue Jan 19, 2023 · 1 comment · Fixed by #2179
Assignees
@St-Permiakov
Labels
bug Priority: High
Milestone
Ketcher 2.9.0-rc.1

Comments

@yuleicul
Copy link
Collaborator

yuleicul commented Jan 19, 2023 •
edited by Zhirnoff
Loading

Steps to Reproduce

  1. Use the select tool to select a functional group or paste a functional group from clipboard
  2. Drag it close to other structure (such as a benzene)
  3. Click on the canvas to insert the functional group

Actual behavior
After step 2, there may be multiple highlighted green circles that indicate atoms to be merged, and the circles correspond to the atoms inside the expanded functional group.
After step 3, there may be multiple functional groups being merged.
Additionaly, the group may get merged with other structures that are in close proximity.

Expected behavior
Contracted functional group should ignore all atoms apart form attachment point when dragging (and/or pasting)

Additional info
It looks like a contracted functional group is being treated as an expanded one while it is being dragged along the canvas, hence it is showing multiple connection points wherever atoms of an expanded group are located.
Based on the reproduction steps:
After step 2, there will be only one highlighted green circle that indicates the unique attachment atom of the functional group.
After step 3, there will be only one functional group being merged and it will be merged only to the structure that is highlighted

Example of correct merge of functional group:
image

Salts or solvents are special case of this behaviour and they are not supposed to be merged at all. Example of adding salt or solvent to canvas over benzene ring:
image

Screenshots
ketcher-paste-bug-06

Ketcher version
v2.7.0 (remote rc version)

Additional context
Two related bugs:

The correct behavior is described in the ticket #2394

#184100466
@yuleicul yuleicul assigned Nitvex and unassigned Nitvex Jan 19, 2023
@yuleicul yuleicul mentioned this issue Jan 19, 2023
Merged
@yuleicul yuleicul added this to the Release Candidate 2.8.0-rc.1 milestone Jan 20, 2023
St-Permiakov pushed a commit that referenced this issue Jan 27, 2023
#2110 new template/paste/merge logic for groups WiP
1269547
St-Permiakov pushed a commit that referenced this issue Feb 1, 2023
#2110 new template/paste/merge logic for groups WiP
ce6db8b
St-Permiakov pushed a commit that referenced this issue Feb 1, 2023
#2110 - updated template tool
0edb50e
St-Permiakov pushed a commit that referenced this issue Feb 1, 2023
#2110 - create new insert tool based on template tool
2fbe07c
St-Permiakov pushed a commit that referenced this issue Feb 1, 2023
#2110 - Insert tool WiP
1b80b18
St-Permiakov pushed a commit that referenced this issue Feb 1, 2023
#2110 - Insert tool WiP
3f5e50d
St-Permiakov pushed a commit that referenced this issue Feb 2, 2023
#2110 new template/paste/merge logic for groups WiP
ee71d0e
St-Permiakov pushed a commit that referenced this issue Feb 2, 2023
#2110 - updated template tool
4b3705d
St-Permiakov pushed a commit that referenced this issue Feb 2, 2023
#2110 - create new insert tool based on template tool
e7cd267
St-Permiakov pushed a commit that referenced this issue Feb 2, 2023
#2110 - Insert tool WiP
3f90a94
St-Permiakov pushed a commit that referenced this issue Feb 6, 2023
#2110 new template/paste/merge logic for groups WiP
99e7453
St-Permiakov pushed a commit that referenced this issue Feb 6, 2023
#2110 - updated template tool
a2d414a
St-Permiakov pushed a commit that referenced this issue Feb 6, 2023
#2110 - create new insert tool based on template tool
3f6115c
St-Permiakov pushed a commit that referenced this issue Feb 6, 2023
#2110 - Insert tool WiP
a1e7722
St-Permiakov pushed a commit that referenced this issue Feb 6, 2023
#2110 - Insert tool WiP
c34eca5
St-Permiakov pushed a commit that referenced this issue Feb 6, 2023
#2110 - Drag-n-drop fix for group interaction
49a854e
@St-Permiakov St-Permiakov mentioned this issue Feb 6, 2023
Merged
St-Permiakov pushed a commit that referenced this issue Feb 6, 2023
#2110 - Drag-n-drop fix for group interaction
e07976e
St-Permiakov pushed a commit that referenced this issue Feb 7, 2023
#2110 - sync select and paste behavior for groups
7dfa2bb
St-Permiakov pushed a commit that referenced this issue Feb 7, 2023
#2110 - Unification and extraction of Select and Paste tools logic - …
582618d 
…{WiP}
St-Permiakov pushed a commit that referenced this issue Feb 7, 2023
#2110 - Unification and extraction of Select and Paste tools logic - …
6d48ee2 
…{WiP}
St-Permiakov pushed a commit that referenced this issue Feb 8, 2023
#2110 - some select mouseup refactor
c0dd5fd
St-Permiakov pushed a commit that referenced this issue Feb 8, 2023
#2110 - refactor and separate logic of select and paste mousemove
cc906c1
St-Permiakov pushed a commit that referenced this issue Feb 8, 2023
#2110 - refactor and separate logic of select and paste mousemove
10ed5d6
St-Permiakov pushed a commit that referenced this issue Feb 8, 2023
#2110 - shared utils hotfix
ed9210c
St-Permiakov pushed a commit that referenced this issue Feb 10, 2023
#2110 - improvements to code transparency
e4ab22c
St-Permiakov pushed a commit that referenced this issue Feb 10, 2023
Merge remote-tracking branch 'origin/master' into #2110-incorrect-mer…
0fd12e1 
…ging-of-functional-groups
St-Permiakov pushed a commit that referenced this issue Feb 13, 2023
Merge remote-tracking branch 'origin/master' into #2110-incorrect-mer…
55ed7f6 
…ging-of-functional-groups
St-Permiakov pushed a commit that referenced this issue Feb 14, 2023
#2110 - fix getnongroupitems method
f6db8b2
St-Permiakov added a commit that referenced this issue Feb 14, 2023
@St-Permiakov
#2110: Incorrect merging of Functional Groups (#2179)
30df9a0 
* #2110 - Drag-n-drop fix for group interaction

* #2110 - sync select and paste behavior for groups

* #2110 - Unification and extraction of Select and Paste tools logic - {WiP}

* #2110 - some select mouseup refactor

* #2110 - refactor and separate logic of select and paste mousemove

* #2110 - shared utils hotfix

* #2110 - improvements to code transparency

* #2110 - fix getnongroupitems method

---------

Co-authored-by: Stanislav Permiakov <Stanislav.Permiakov@primark.onmicrosoft.com>
ansivgit pushed a commit that referenced this issue Feb 22, 2023
@St-Permiakov @ansivgit
#2110: Incorrect merging of Functional Groups (#2179)
f512740 
* #2110 - Drag-n-drop fix for group interaction

* #2110 - sync select and paste behavior for groups

* #2110 - Unification and extraction of Select and Paste tools logic - {WiP}

* #2110 - some select mouseup refactor

* #2110 - refactor and separate logic of select and paste mousemove

* #2110 - shared utils hotfix

* #2110 - improvements to code transparency

* #2110 - fix getnongroupitems method

---------

Co-authored-by: Stanislav Permiakov <Stanislav.Permiakov@primark.onmicrosoft.com>
@Zhirnoff
Copy link
Collaborator

At the moment, the ticket is supposedly not working correctly due to a bug #2326
The correct behavior is described in the ticket #2394

@kaluginserg kaluginserg mentioned this issue May 19, 2023
Closed
@yuleicul yuleicul mentioned this issue May 23, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@St-Permiakov St-Permiakov

Labels
bug Priority: High
Projects
None yet
Milestone
Ketcher 2.9.0-rc.1
Development

Successfully merging a pull request may close this issue.

#2110: Incorrect merging of Functional Groups epam/ketcher
5 participants
@St-Permiakov @yuleicul @Nitvex @Zhirnoff @KonstantinEpam23