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

Tool refers to unmounted canvas instance when new file is opened #1057

Closed
karen-sarkisyan opened this issue Dec 3, 2021 · 0 comments · Fixed by #1064
Closed

Tool refers to unmounted canvas instance when new file is opened #1057

karen-sarkisyan opened this issue Dec 3, 2021 · 0 comments · Fixed by #1064
Assignees
@karen-sarkisyan @ViktoriiaFedotkina
Labels
bug
Milestone
Release 2.5.0

Comments

@karen-sarkisyan
Copy link
Contributor

karen-sarkisyan commented Dec 3, 2021 •
edited
Loading

All Editor tools should be checked to make sure they work correctly when a file is opened.

Example issue is #1032 , which can be reproduced for eraser, bond and other tools. For example:

  • Open Ketcher (version 2.4 and later) and select Eraser tool.
  • Open file with expanded FG ketcher (21).zip
  • Hover over one atom of a functional group and try to erase it

Expected behavior
Erase action fails. Modal is shown with suggestion to remove FG abbreviation before changing it.

Actual behavior
Atom is erased

Same happens with other tools (whichever tool is active when file opens) except for Select tool, which was fixed in PR #1051

Suggested fix and refactoring for other tools is:

  • Make sure tool's constructor function does not refer to editor.render.ctab and its nested objects, because it creates a reference to an older state of canvas when new canvas is created on file open.
  • Use ES6 class syntax
  • Use TypeScript
@AndreiMazol AndreiMazol added this to the Release 2.4.0 milestone Dec 6, 2021
@karen-sarkisyan karen-sarkisyan self-assigned this Dec 7, 2021
@karen-sarkisyan karen-sarkisyan mentioned this issue Dec 7, 2021
Merged
@karen-sarkisyan karen-sarkisyan linked a pull request Dec 7, 2021 that will close this issue
#1057 tools that modify FGs refer canvas correctly #1064
Merged
AndreiMazol pushed a commit that referenced this issue Dec 9, 2021
@karen-sarkisyan @ViktoriiaFedotkina
#1057 tools that modify FGs refer canvas correctly (#1064)
8e9e05e 
* Renamed tools for refactoring to .ts extension

* Refactor Eraser tool

* Refactor Bond tool

* Refactor Chain tool

* Refactor Charge tool

* Refactor Rotate tool

* Refactor Atom tool

* Refactor APoint tool

* Refactored template tool into class with correct reference to canvas

* Refactored sgroup tool into class with correct reference to canvas

* Refactored RGroupFragment tool into class with correct reference to canvas

* Refactored RGroupAtom tool into class with correct reference to canvas

* Refactored Paste tool into class with correct reference to canvas

* Refactored Attach tool into class with correct reference to canvas

Co-authored-by: Viktoriia Fedotkina <[email protected]>
AndreiMazol added a commit that referenced this issue Dec 16, 2021
@AndreiMazol
Revert "#1057 tools that modify FGs refer canvas correctly (#1064)"
498e816 
This reverts commit 8e9e05e.
AndreiMazol added a commit that referenced this issue Dec 16, 2021
@AndreiMazol
Revert "#1057 tools that modify FGs refer canvas correctly (#1064)" (#…
6bd89a6 
…1115)

This reverts commit 8e9e05e.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@karen-sarkisyan karen-sarkisyan

@ViktoriiaFedotkina ViktoriiaFedotkina

Labels
bug
Projects
None yet
Milestone
Release 2.5.0
Development

Successfully merging a pull request may close this issue.

#1057 tools that modify FGs refer canvas correctly epam/ketcher
3 participants
@karen-sarkisyan @AndreiMazol @ViktoriiaFedotkina