-
Notifications
You must be signed in to change notification settings - Fork 176
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
#3728 - Add group selection for macromolecules #3879
Conversation
public selectCurrentDrawingEntities(drawingEntity: DrawingEntity) { | ||
const command = new Command(); | ||
|
||
this.allEntities.forEach(([, drawingEntity]) => { | ||
if (drawingEntity.selected) { | ||
const operation = new DrawingEntitySelectOperation(drawingEntity); | ||
command.addOperation(operation); | ||
} | ||
}); | ||
|
||
if (drawingEntity.selected) { | ||
drawingEntity.turnOffSelection(); | ||
const operation = new DrawingEntitySelectOperation(drawingEntity); | ||
command.addOperation(operation); | ||
} else { | ||
drawingEntity.turnOnSelection(); | ||
const operation = new DrawingEntitySelectOperation(drawingEntity); | ||
command.addOperation(operation); | ||
} | ||
|
||
return command; | ||
} | ||
|
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.
I suggest below changes:
private addOperationToCommand(
command: Command,
drawingEntity: DrawingEntity,
) {
const operation = new DrawingEntitySelectOperation(drawingEntity);
command.addOperation(operation);
}
public selectCurrentDrawingEntities(drawingEntity: DrawingEntity) {
const command = new Command();
this.allEntities.forEach(([, drawingEntity]) => {
if (drawingEntity.selected) {
this.addOperationToCommand(command, drawingEntity);
}
});
if (drawingEntity.selected) {
drawingEntity.turnOffSelection();
} else {
drawingEntity.turnOnSelection();
}
this.addOperationToCommand(command, drawingEntity);
return command;
}
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.
I believe that addOperationToCommand is not the best name for this method, because it actually adds concrete DrawingEntitySelectOperation.
However I would propose to not create this method at all. We can do like this in selectCurrentDrawingEntities method:
command.addOperation(
new DrawingEntitySelectOperation(drawingEntity)
);
65ef360
to
f6c3698
Compare
f6c3698
to
b313673
Compare
public selectCurrentDrawingEntities(drawingEntity: DrawingEntity) { | ||
const command = new Command(); | ||
|
||
this.allEntities.forEach(([, drawingEntity]) => { | ||
if (drawingEntity.selected) { | ||
this.addOperationToCommand(command, drawingEntity); | ||
} | ||
}); | ||
|
||
if (drawingEntity.selected) { | ||
drawingEntity.turnOffSelection(); | ||
} else { | ||
drawingEntity.turnOnSelection(); | ||
} | ||
this.addOperationToCommand(command, drawingEntity); | ||
|
||
return command; | ||
} |
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.
It looks like a bit overhead to create operations for already selected entities. Can we rework and rename this method to add only one drawing entity to selection?
How the feature works? / How did you fix the issue?
(Screenshots, videos, or GIFs, if applicable)
Check list
#1234 – issue name