-
Notifications
You must be signed in to change notification settings - Fork 187
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
#1954 Draw selected structure at mouse cursor after closing Templates window #2079
#1954 Draw selected structure at mouse cursor after closing Templates window #2079
Conversation
/** | ||
* Why deprecated? | ||
* | ||
* We want to make this tool consistent with `PasteTool`, | ||
* @see https://github.com/epam/ketcher/issues/1954 | ||
* | ||
* We know, there will be some features being lost, such as rotation. | ||
* There also will be some bugs, such as multiple attachment atoms. | ||
* | ||
* So, next steps, in `PasteTool`, lost features will be added and bugs will be fixed. | ||
* Keep this file here in case it's helpful to future work. | ||
*/ |
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.
Indicated why.
Please note - there is a bit of an issue w/ Paste tool, if you want to select another tool with your mouse while hovering - it will move the scroll position. I dont think this is a particularly big issue, something to note though: rec-paste-2.mov |
Another edge-case issue w/ Paste tool (I dont think its that important, but good to know) - if you press Ctrl+S (save) on your keyboard - the structure that is being hovered will get into the saved molecule. rec-paste.mov |
btw, I cant reproduce this on master |
looks like #2087 resolves this |
(This bug has been fixed by commit 829fbf2 in this PR) Bug: when pasting a salt or solvent next to another structure, it shouldn't be mergedSteps to Reproduce
Actual behavior Expected behavior |
(This bug has been fixed by commit 829fbf2 and 3afa70e in this PR) Bug: Green hovered circle showing when moving salts and solvents close to other structures by select toolSteps to Reproduce
Actual behavior Expected behavior |
This bug occurs after the two bugs above have been fixed. I guess It's a bit complicated to resolve. Consider creating a new ticket or maybe there are other tickets that are working on it. Bug: Non-expanded functional group doesn't show the green merging circle because the circle is hidden in the functional groupSteps to Reproduce
Actual behavior Expected behavior |
if (mergeMap.atoms.size === 0 && mergeMap.bonds.size === 0) return null | ||
|
||
return mergeMap | ||
} | ||
|
||
function omitAtomsAndBondsOfSaltsAndSolventsFromMergeMap( |
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.
In general, LGTM.
But the only thing I want to clarify – can't mergeMap
include 'salt or solvents'?
From this function, I see it doesn't add salt or solvent to a map, but can't they be added before this function gets called? In lines 85-88, there new Map created from atoms and bonds of closestItem:
let mergeMap: MergeMap = {
atoms: new Map(closestMap.atoms),
bonds: new Map(closestMap.bonds)
}
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.
Really good suggestion. Have changed to call this function a bit earlier. Thank you!
@Nitvex I suppose this one is complicated. I have created a new issue for it, see #2110 |
This one is redirected to #2109 |
} | ||
}) | ||
|
||
return mergeMap |
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.
Maybe we shouldn't separate testing for atoms and bonds involved in merge and cancel merge if there is any sign of salt or solvent at all.
But I'm not sure about all possible consequences, so it's just an assumption for now.
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.
We might want to wait for #2110 and get back to re-implementing this
…th-another-functional-group-on-clickdrag
…up on click&drag
…oup-does-not-connect-with-another-functional-group-on-clickdrag
…th-another-functional-group-on-clickdrag
bbcd21b
to
a2d0c0b
Compare
…th-another-functional-group-on-clickdrag
a2d0c0b
to
fb73c1f
Compare
…-structure-at-mouse-cursor-as-pasting-does
…sor-as-pasting-does
One more note - it seems to lag a bit when devtools are open and a benzene ring is following cursor around the canvas. |
…raw-selected-structure-at-mouse-cursor-as-pasting-does
Resolves #1954
To make the template tool consistent with the paste tool, I marked the previous implementation as deprecated instead of deleting it directly, in case the previous code is helpful to future work.
Bug collections
Showcase