Macro: #3869 - Left-to-right ("Snake-like") layout for RNA #3932

StarlaStarla · 2024-01-18T14:44:36Z

How the feature works? / How did you fix the issue?

(Screenshots, videos, or GIFs, if applicable)

Check list

unit-tests written
e2e-tests written
documentation updated
PR name follows the pattern #1234 – issue name
branch name doesn't contain '#'
PR is linked with the issue
base branch (master or release/xx) is correct
task status changed to "Code review"
reviewers are notified about the pull request

…-to-right-snake-like-layout-for-rna

rrodionov91 · 2024-01-29T17:37:15Z

ketcher-autotests/tests/Macromolecule-editor/Polymer-Bond-Tool/snake-bond-tool.spec.ts

+    const phosphate1 = await page
+      .locator(`//\*[name() = 'g' and ./\*[name()='text' and .='P']]`)
+      .nth(0);
+


I would propose to add it to addRnaPresetOnCanvas method so that it will return object with 3 locators, one for each part of rna preset

rrodionov91 · 2024-01-29T17:40:41Z

ketcher-autotests/tests/Macromolecule-editor/Polymer-Bond-Tool/snake-bond-tool.spec.ts

+    await page.locator('button[title=R2]').nth(0).click();
+    await page.locator('button[title=R1]').nth(1).click();
+    await page.locator('button[title=Connect]').click();


I would propose to move it to bondTwoMonomers. Check if modal window appeared after connecting monomers so choose attachment points (by default R2 - R1, but could be configured by function parameters) and click connect

rrodionov91 · 2024-01-29T17:46:41Z

ketcher-autotests/tests/Macromolecule-editor/Polymer-Bond-Tool/snake-bond-tool.spec.ts

+    const monomer5 = await addMonomerToCanvas(
+      page,
+      'A___Alanine',
+      'A',
+      700,
+      700,
+      4,
+    );
+    const monomer6 = await addMonomerToCanvas(
+      page,
+      'Bal___beta-Alanine',
+      'Bal',
+      600,
+      500,
+      0,
+    );
+    const monomer7 = await addMonomerToCanvas(
+      page,
+      'Bal___beta-Alanine',
+      'Bal',
+      650,
+      500,
+      1,
+    );
+    const monomer8 = await addMonomerToCanvas(
+      page,
+      'Bal___beta-Alanine',
+      'Bal',
+      750,
+      500,
+      2,
+    );


It would be great to have some helper to create monomers like that:

await addMonomersToCanvas([ { name: 'Bal', fullName: 'Bal___beta-Alanine', amount 5, x: 50, y: 100, deltaX: 100, deltaY: 100 } ])

Where deltaX and deltaY will be an increment of position of each monomer from initial x and y.

Also we probably can create addBondedMonomersToCanvas over it. If it is time consuming we can create a separate ticket for that. Please let me know if so.

rrodionov91 · 2024-01-29T18:02:57Z

packages/ketcher-core/src/application/render/renderers/PolymerBondRenderer.ts

+  private isMonomersTypeDifferent() {
+    return (
+      (this.polymerBond.secondMonomer &&
+        [Peptide, Chem].some(
+          (type) => this.polymerBond.firstMonomer instanceof type,
+        ) &&
+        [Sugar, Phosphate].some(
+          (type) => this.polymerBond.secondMonomer instanceof type,
+        )) ||
+      ([Peptide, Chem].some(
+        (type) => this.polymerBond.secondMonomer instanceof type,
+      ) &&
+        [Sugar, Phosphate].some(
+          (type) => this.polymerBond.firstMonomer instanceof type,
+        ))
+    );


I would propose to move part of this logic into concrete monomer classes (Like Peptide, Chem, e t c). Like so for Peptide:

public isMonomerTypeDifferentForChaining(monomerToChain: BaseMonomer) {
return monomerToChain instanceof Sugar || monomerToChain instanceof Phosphate
}

rrodionov91 · 2024-01-29T18:17:12Z

packages/ketcher-core/src/domain/entities/DrawingEntitiesManager.ts

+    const height =
+      (nucleotide.sugar.renderer?.monomerSize.height || 0) +
+      (nucleotide.rnaBase.renderer?.monomerSize.height || 0) +
+      45;


I would propose to move 45 into constant with appropriate name

rrodionov91 · 2024-01-29T18:30:53Z

packages/ketcher-core/src/domain/entities/DrawingEntitiesManager.ts

+        polymerBond?.secondMonomer === rnaBase
+          ? polymerBond?.firstMonomer
+          : polymerBond?.secondMonomer;


there is polymerBond.getAnotherMonomer(rnaBase) method I believe

rrodionov91 · 2024-01-29T18:36:54Z

packages/ketcher-core/src/domain/entities/DrawingEntitiesManager.ts

+      lastPosition.y +
+        (nucleotide.sugar.renderer?.monomerSize?.height ?? 0) / 2 +
+        (nucleotide.rnaBase.renderer?.monomerSize?.height ?? 0) / 2 +
+        45,


Move to constant please.

rrodionov91 · 2024-01-29T18:37:05Z

packages/ketcher-core/src/domain/entities/DrawingEntitiesManager.ts

+        lastPosition.x +
+          (nucleotide.sugar.renderer?.monomerSize?.width ?? 0) / 2 +
+          (nucleotide.phosphate?.renderer?.monomerSize?.width ?? 0) / 2 +
+          45,


…-to-right-snake-like-layout-for-rna

rrodionov91 · 2024-02-01T10:47:38Z

ketcher-autotests/tests/Macromolecule-editor/Polymer-Bond-Tool/snake-bond-tool.spec.ts

    await bondTwoMonomers(page, peptide2, peptide3);
    await bondTwoMonomers(page, peptide3, peptide4);

-    await takeEditorScreenshot(page);
+    // await takeEditorScreenshot(page);


Excess comment I believe

rrodionov91 · 2024-02-01T12:47:13Z

packages/ketcher-core/src/domain/entities/DrawingEntitiesManager.ts


-    return { command, lastPosition };
+  private getRnaBaseSideChainMonomers(


Could you please rename this method? Currently it starts from "get" which means that it should return the value and should not mutate some external state, but it does.

rrodionov91 · 2024-02-01T12:50:08Z

packages/ketcher-core/src/domain/entities/DrawingEntitiesManager.ts

+        DISTANCE_BETWEEN_MONOMERS +
+        HORIZONTAL_DISTANCE_FROM_MONOMER +


What is the difference between these two values?

Actually I just copied the legency code for use. I would assume DISTANCE_BETWEEN_MONOMERS is the length of the corner line that connects two lines of chain, while the HORIZONTAL_DISTANCE_FROM_MONOMER is the distance between monomers.

rrodionov91 · 2024-02-01T13:22:39Z

packages/ketcher-core/src/domain/entities/DrawingEntitiesManager.ts

+  public getPhosphateFromRnaBase(baseMonomer: RNABase) {
+    const r1PolymerBond = baseMonomer.attachmentPointsToBonds.R1;
+    const sugarMonomer = r1PolymerBond?.getAnotherMonomer(baseMonomer);
+    if (sugarMonomer && sugarMonomer instanceof Sugar) {
+      const phosphate = getNextMonomerInChain(sugarMonomer);
+      if (phosphate && phosphate instanceof Phosphate) {
+        return phosphate;
+      }
+    }
+    return undefined;


Looks like this method is not used

rrodionov91

Please fix comments above

#3869 Left-to-right ("Snake-like") layout for RNA

20d7fa7

StarlaStarla linked an issue Jan 18, 2024 that may be closed by this pull request

Macro: Left-to-right ("Snake-like") layout for RNA #3869

Closed

auto-assign bot requested review from AlexeyGirin, chgayane and rrodionov91 January 18, 2024 14:44

auto-assign bot assigned StarlaStarla Jan 18, 2024

StarlaStarla added 2 commits January 19, 2024 16:52

#3869 snake mode for rna chain

4bff65c

#3869 revert changes

916d69e

StarlaStarla force-pushed the 3869-macro-left-to-right-snake-like-layout-for-rna branch from f644475 to 916d69e Compare January 19, 2024 09:13

StarlaStarla added 9 commits January 21, 2024 18:03

#3869 fix snake line issue

a8dbc32

Merge branch 'master' of github.com:epam/ketcher into 3869-macro-left…

c835c2e

…-to-right-snake-like-layout-for-rna

#3869 fix screenshot

f705c8a

#3869 small fix and tests

219fc53

#3869 add rearrange logic for nucleoside

debbbe9

#3869 small fix for code and test

fc7d6c7

#3869 refactor side chain logic

8b455e1

Merge branch 'master' of github.com:epam/ketcher into 3869-macro-left…

53f4793

…-to-right-snake-like-layout-for-rna

#3869 update screenshot

6a1a825

rrodionov91 requested changes Jan 29, 2024

View reviewed changes

StarlaStarla and others added 6 commits January 30, 2024 18:12

#3869 enhancement

187aedb

# 3869 fix issue

639f783

Merge branch 'master' of github.com:epam/ketcher into 3869-macro-left…

f77a9c1

…-to-right-snake-like-layout-for-rna

#3869 fix snake mode issue for same attachment points and autotests

f53ce80

#3869 remove only of test

8019421

- fixed cases with side connections

4cc6baa

rrodionov91 approved these changes Feb 1, 2024

View reviewed changes

rrodionov91 requested changes Feb 1, 2024

View reviewed changes

StarlaStarla added 3 commits February 2, 2024 14:17

#3869 fix single monomer and nucleoside case

00910a8

#3869 fix some comments issues

1946733

#3869 fix autotests

0e25fd6

rrodionov91 approved these changes Feb 2, 2024

View reviewed changes

rrodionov91 merged commit fb33861 into master Feb 2, 2024
5 checks passed

rrodionov91 deleted the 3869-macro-left-to-right-snake-like-layout-for-rna branch February 2, 2024 15:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Macro: #3869 - Left-to-right ("Snake-like") layout for RNA #3932

Macro: #3869 - Left-to-right ("Snake-like") layout for RNA #3932

StarlaStarla commented Jan 18, 2024 •

edited

Loading

rrodionov91 Jan 29, 2024

rrodionov91 Jan 29, 2024

rrodionov91 Jan 29, 2024

rrodionov91 Jan 29, 2024

rrodionov91 Jan 29, 2024

rrodionov91 Jan 29, 2024

rrodionov91 Jan 29, 2024

rrodionov91 Jan 29, 2024

rrodionov91 Feb 1, 2024

rrodionov91 Feb 1, 2024

rrodionov91 Feb 1, 2024

StarlaStarla Feb 2, 2024 •

edited

Loading

rrodionov91 Feb 1, 2024

rrodionov91 left a comment


		return { command, lastPosition };
		private getRnaBaseSideChainMonomers(

		DISTANCE_BETWEEN_MONOMERS +
		HORIZONTAL_DISTANCE_FROM_MONOMER +

Macro: #3869 - Left-to-right ("Snake-like") layout for RNA #3932

Macro: #3869 - Left-to-right ("Snake-like") layout for RNA #3932

Conversation

StarlaStarla commented Jan 18, 2024 • edited Loading

How the feature works? / How did you fix the issue?

Check list

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StarlaStarla Feb 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rrodionov91 left a comment

Choose a reason for hiding this comment

StarlaStarla commented Jan 18, 2024 •

edited

Loading

StarlaStarla Feb 2, 2024 •

edited

Loading