feat: Ket2 solid phase support #2230
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
lib/chemotion/sanitizer.rb
Outdated
| @@ -20,6 +20,10 @@ def scrub_svg(fragment, encoding: nil) | |||
| def base_scrub_ml(fragment, type: :xml, encoding: nil) | |||
| result = encoding ? fragment.encode(encoding) : fragment | |||
|
|
|||
| # Allow the <image> tag and all its attributes | |||
| scrubber = Loofah::Scrubber.new do |node| | |||
| node['src'] = node['src'] if node.name == 'image' | |||
There was a problem hiding this comment.
Lint/SelfAssignment: Self-assignment detected.
lib/svg/processor.rb
Outdated
| when /chemdraw/i | ||
| Chemotion::ChemdrawSvgProcessor.new(svg) | ||
| when /ketcher/i | ||
| Ketcherails::SVGProcessor.new(svg) | ||
| else | ||
| Chemotion::OpenBabelSvgProcessor.new(svg) | ||
| end | ||
|
|
||
| editor === 'ketcher2' && is_centered = true |
There was a problem hiding this comment.
Style/CaseEquality: Avoid the use of the case equality operator ===.
lib/svg/processor.rb
Outdated
| when /chemdraw/i | ||
| Chemotion::ChemdrawSvgProcessor.new(svg) | ||
| when /ketcher/i | ||
| Ketcherails::SVGProcessor.new(svg) | ||
| else | ||
| Chemotion::OpenBabelSvgProcessor.new(svg) | ||
| end | ||
|
|
||
| editor === 'ketcher2' && is_centered = true |
There was a problem hiding this comment.
Lint/ShadowedArgument: Argument is_centered was shadowed by a local variable before it was used.
haditariq
force-pushed
the
ketcher-surface-chemistry
branch
3 times, most recently
from
ada46b4 to
68e2891
Compare
adambasha0
requested changes
Dec 13, 2024
There was a problem hiding this comment.
Hi Hadi,
Thanks for the feature. In general, Eslint rules need to be followed, redundant/not used functions need to be removed, better to refactor big files into smaller components and add unit tests for most of the functions to be able to test the functionality
| const { ket2Molfile, svgElement } = await this.ketcher2Ref.current.onSaveFileK2SC(); | ||
| this.setState({ showModal: false, showWarning: this.props.hasChildren || this.props.hasParent }, () => { if (this.props.onSave) { this.props.onSave(ket2Molfile, svgElement, { smiles: '' }, editor.id); } }); | ||
| } else { | ||
| console.error("onSaveFileK2SC is not a function"); |
There was a problem hiding this comment.
suggestion to use:
- try & catch block (as in the next block below)
- avoid nested if/else when possible
- catch: console.error("Error during save operation:", error);
- use destructuring props assignment (eslint)
| } else { | ||
| try { | ||
| const { molfile, info } = structure; | ||
| if (!molfile) throw new Error('No molfile'); | ||
| structure.fetchSVG().then((svg) => { | ||
| console.log({ railssvg: svg }); |
| import DocumentHelper from 'src/utilities/DocumentHelper'; | ||
| import { templateParser } from '../../../utilities/Ketcher2SurfaceChemistryUtils'; |
There was a problem hiding this comment.
import statements should have an absolute patheslint
| @@ -83,11 +83,13 @@ class UserActions { | |||
| }; | |||
| } | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
More than 1 blank line not allowed (eslint)
| def base_scrub_ml(fragment, type: :xml, encoding: nil) | ||
| result = encoding ? fragment.encode(encoding) : fragment | ||
|
|
||
| # Allow the <image> tag and all its attributes | ||
| scrubber = allow_image_tag |
There was a problem hiding this comment.
would be nice to have a spec test for allowing image tag in sanitizer_spec.rb
| const { c_images, molfileData, image_counter } = await adding_polymers_ketcher_format(rails_polymers_list, mols, data, image_used_counter); | ||
| image_used_counter = image_counter; | ||
| molfileData?.root?.nodes.push(...c_images); | ||
| return { collected_images: c_images, molfileData }; |
There was a problem hiding this comment.
Inconsistent properties inside the object?
| }; | ||
|
|
||
| // helper function to test alias list consistency 0,1,2,3,4... | ||
| export const isAliasConsistent = () => { |
There was a problem hiding this comment.
- index_list and uniqueIndices Redundancy:
- Both serve related purposes (tracking indices)
- Use a single data structure (Set or Array) to handle both uniqueness checks and sorting.
|
|
||
| for (let m = 0; m < mols.length; m++) { | ||
| const mol = latestData[mols[m]]; | ||
| for (let a = 0; a < mol?.atoms?.length; a++) { |
There was a problem hiding this comment.
- If mol or mol.atoms is null or undefined, this loop will silently do nothing, potentially masking issues.
- Suggestion: Validate the structure of mol and mol.atoms before entering the loop.
| -> tbr -> flag means this atom has to removed from the list coming from the template | ||
| -> isAliasConsistent before returning -> is a function to make sure all aliases generated are in order 0,1,2,3,4,5,6... | ||
| */ | ||
| const addAtomAliasHelper = async (already_processed) => { |
| @@ -1,45 +1,845 @@ | |||
| /* eslint-disable react/require-default-props */ | |||
There was a problem hiding this comment.
File is too big:
- suggestion: besides KetcherEditor functional component
- break it into:
- KetcherActions
- Process/Manipulate KetcherData
- KetcherValidations
- unit tests are missing for this functional component?
- break it into:
haditariq
force-pushed
the
ketcher-surface-chemistry
branch
from
45c05be to
05e74b2
Compare
haditariq
force-pushed
the
ketcher-surface-chemistry
branch
2 times, most recently
from
643d3bd to
e4ed460
Compare
image drop position captured chore: show alias for atom R# ketcher editor events chore: show alias for atom R# ketcher editor events refac: atom location update with selected image without loop refac: atom location update with selected image without loop refac: atom location update with selected image without loop chore: editor selection atttibute used for identification chore: editor selection atttibute used for identification chore: editor selection atttibute used for identification chore: editor selection atttibute used for identification fix: revert to loop approach for moving a connection feat: alias-atom position on image resize chore: template resize implementation with stack FILO approach feat: atom place with image on correct coordinates done chore: multi template binded movement feat: compatible with other atoms on canvas feat: on atom move stick to template fix: move temaplte to center after atom add feat: mutation observers for cleanup and clear canvas added chore: image delete not possible chore: templates replace with Br -> A(R#)
refactor: mofile import/export compatibility feat: shapes synced with ketcher rails chore: standard size defined for shapes draft: adding polymer images to svg chore: svg svg with editor save fix: loofah image tag allowed for sanitization, shapes display rails-ketcher vice-versa refactor: molfile pre-processors, flags, helper function for handleAddAtom process refactor: templates source refactor: delete template and restore image-counters refactor: DOM > images update flags setter & getters refactor: tags polymer identifiers refactor: event-handler and event-fire for ketcher2 changes refactor: ketcher button events molecule file reset refactor: template list for local storage fix: file with no polyer main cb fix: clean canvas use-cases draft: counter issue with canvas is new fix: addHandleAtom with seen_third_parts, avoiding duplicates for image_used_counter increament chore: template force alias modification removed code clean surface shape size changed && image selection action overide chore: hide extra attributes for user templates list fix: image delete overide, t_02 template size changed draft: removal of linked atom with image on image delete fix: on delete image re-sync counters and delete atom fix: contant image size on layout and on all actions feat: redo and undo fix: multiple move atoms makes add atom disable fix: multi template delete, remove atom with image delete and vice versa update: delete working with multi images, single atom with alias, or single image fix:event: delete image fix: delete with image or delete with selection or delete with atom fix: image movement after delete draft:refactor code clean refac: savemolfile draft: sb/schema.rb rebase refac: scrubber, svf-processor
…KetcherData test: setKetcherdata test: hasKetcherdata, setKetcherdata refactoring: onTemplateMove refactoring+test: movetemplate & placeImageOnAtom Refac+test: on add atom test: on atom add image counter checker refactor+tests: on image delete, mockup files for test test: on delete image delete atom with connected alias; with single or multi images| test+refac: on delete atom clean+refac: add atom logic divied chore: code doc, template-list shape alignment fix: delete atom when a mol is dropped on other mol; delete mol1 and update aliases for mol0 specs fix ketcher Editor refactoring: delete image, read mofile, delete atom(single/group) refactoring+fixes: delete atom by image, delete image by atom, savingMofile fix: delete image when single atom behind is deleted fix: delete atom case when dragged index is smaller then others chore: template names updated bead, surface cleanup + minor fixes test: delete atom to reset aliases, molfile save Refactoring, comments, templates-source parsing
fix: reading molfile for saving fix: polymers index chore: image tag filter applied Chore: Ket2 Surface Chemistry feature docs test: template list processing fix: delete atom with select, behind atom, template, calculate pi event, minor feature-docs changes fix: delete template on click atom fix: utils moved and delete atom minor fix fix: image placement index with refactoring fix: on save molfile fix: resolve ESLint warnings and errors chore: linting, correct template matching while saving clean: Identifying polymers list chore: packs moved, docker configs
haditariq
force-pushed
the
ketcher-surface-chemistry
branch
from
e4ed460 to
4cf8a1c
Compare
…, molfile extra properties adjustments
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR resolves: Issue#166.
Demo:
1- Basic Usage image template
k2SC.image.template.usage.mp4
2- Molfile compatibility with Ketcherrails and ketcher2
k2SC.-.Rails.and.ketcher2.compatibility.mp4
3- Modify image templates with new user templates
k2SC.-.use.template.with.image.templates.1.mp4
4- Redo and Undo features
redo.and.undo.feature.1.mp4
5- Erase Del
20241108-1458-05.8099941.mp4
6- Layout
K2SC.-.demo.vis.1.mp4
rather 1-story 1-commit than sub-atomic commits
commit title is meaningful => git history search
commit description is helpful => helps the reviewer to understand the changes
code is up-to-date with the latest developments of the target branch (rebased to it or whatever) => ⏩-merge for linear history is favoured
added code is linted
tests are passing (at least locally): we still have some random test failure on CI. thinking of asking spec/examples.txt to be commited
in case the changes are visible to the end-user, video or screenshots should be added to the PR => helps with user testing
testing coverage improvement is improved.
CHANGELOG : add a bullet point on top (optional: reference to github issue/PR )
parallele PR for documentation on docusaurus if the feature/fix is tagged for a release