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.

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

#5403 - Introduce hydrogen bonds in macromolecules mode #5843

Merged

Conversation

rrodionov91
Copy link
Collaborator

@rrodionov91 rrodionov91 commented Oct 22, 2024

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

@rrodionov91 rrodionov91 linked an issue Oct 22, 2024 that may be closed by this pull request
@rrodionov91 rrodionov91 force-pushed the 5403-Introduce_hydrogen_bonds_in_macromolecules_mode branch 3 times, most recently from 909fca2 to 3eb7308 Compare October 25, 2024 12:19
@@ -756,7 +781,10 @@ export class DrawingEntitiesManager {
return _polymerBond;
}

const polymerBond = new PolymerBond(firstMonomer);
const isHydrogenBond = bondType === 'hydrogen';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use MACROMOLECULES_BOND_TYPES here?

this.editor = editor;
this.history = new EditorHistory(this.editor);
this.bondType =
options.toolName === 'bond-single'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ToolName enum can be used here


constructor(private editor: CoreEditor) {
constructor(private editor: CoreEditor, options: { toolName: string }) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And passed as a type here

@@ -126,6 +130,7 @@ export abstract class BaseMode {
entity.secondMonomer,
firstAttachmentPoint,
secondAttachmentPoint,
undefined,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we shouldn't do anything here as it will lead to massive changes, although I don't really like that we need to pass undefined explicitly in the middle of the arguments list, I'd prefer to have some optional argument in the end of the list or pass arguments as an object with optional properties

@@ -59,6 +61,7 @@ export class DrawingEntityMoveOperation implements Operation {
// we need to redraw them to apply the correct drawing mode.
if (
this.drawingEntity instanceof PolymerBond ||
this.drawingEntity instanceof HydrogenBond ||
this.drawingEntity instanceof MonomerToAtomBond
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe some isBond helper method might come in handy here to improve readability

@@ -220,9 +222,13 @@ export abstract class BaseMonomer extends DrawingEntity {

public setBond(
attachmentPointName: AttachmentPointName,
bond: PolymerBond | MonomerToAtomBond,
bond: PolymerBond | MonomerToAtomBond | HydrogenBond,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we just use BaseBond type here?

polymerBond,
) as AttachmentPointName,
),
(_polymerBond?: PolymerBond) =>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit confusing, polymerBond type is specified as a union in the deletePolymerBond yet here it's just PolymerBond and I didn't see any type narrowing?

@@ -747,6 +771,7 @@ export class DrawingEntitiesManager {
secondMonomer: BaseMonomer,
firstMonomerAttachmentPoint: AttachmentPointName,
secondMonomerAttachmentPoint: AttachmentPointName,
bondType?: string,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps MACROMOLECULES_BOND_TYPES can be used here too?

@@ -818,17 +849,20 @@ export class DrawingEntitiesManager {
secondMonomer: BaseMonomer,
firstMonomerAttachmentPoint: AttachmentPointName,
secondMonomerAttachmentPoint: AttachmentPointName,
bondType = 'polymer',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And here too

@@ -33,7 +33,7 @@ export function LeftMenuComponent() {
const activeMenuItems = [activeTool];

const menuItemChanged = (name) => {
editor.events.selectTool.dispatch(name);
editor.events.selectTool.dispatch(name, { toolName: name });
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit confusing why we pass the name of the tool twice, is it due to the nested menu items? Can we avoid it somehow? We still have unique tool names in the end

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's because options are passed directly to active tool and then tool can use them. So toolName option is used in Bond tool to create appropriate bond type

@rrodionov91 rrodionov91 force-pushed the 5403-Introduce_hydrogen_bonds_in_macromolecules_mode branch from 3eb7308 to 84b85d4 Compare October 28, 2024 14:00
@rrodionov91 rrodionov91 merged commit c55cc66 into master Oct 28, 2024
5 checks passed
@rrodionov91 rrodionov91 deleted the 5403-Introduce_hydrogen_bonds_in_macromolecules_mode branch October 28, 2024 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introduce hydrogen bonds in macromolecules mode
3 participants