Technical proposal of MergeManager for entities in the Ketcher

[kaluginserg]

Problem

1. tool classes do something not related with directly tool logic - handle merge items for various entities: Atom, SGroup, Bond, RGroup.., so it's wrong abstraction and for developer it's hard to grasp the meaning of the changes
2. bloated code in tool classes: Atom - 320 LOC, Bond - 380 LOC, Template - 640 LOC, Select - 680 LOC, Select - 680 LOC, Paste - 280 LOC + helper functions ~ 1000 LOC. (Overall > 3000 LOC)
3. Bad code design and code smell.
4. Bugs in merge logic for lot of situations: ... Paste tool with dragging is not forming a new bond #2716, Click & drag a Functional Group on FG, Salts and Solvents, FG connected with Atom makes replace #2278, Click & drag a 'Salts and Solvents' on atom forms a bond #2280, Functional Group doesn't replace another FG or Salt when it's selected and moved over it #2274, Salt doesn't replace atoms, functional groups and other salts when selected and moved over them #2275, atom-atom structure doesn't replace functional groups and salts when selected and moved over them #2277, Functional Group connected to terminal atoms structure doesn't replace other structures when selected and moved over them #2279, ... etc.

Proposal

Create MergeManager abstraction for handle all merge logic.

export interface MergeOptions {
    mergeStrategy: 'direct' | 'opposite'
    mergeBase?: ActiveSubStruct
}

export interface MergeManager {
    highlightHypothetical(from: ActiveSubStruct, options: MergeOptions): void
    merge(from: ActiveSubStruct, options: MergeOptions): void
}

export type ConflictResolver = (resolveData: unknown) => void

abstract class MergeRule<TFrom, TTo>{
    abstract readonly from: 'atom' | 'bond' | 'sgroupContracted' | 'rgroupLabel'
    abstract readonly to: 'atom' | 'bond' | 'sgroupContracted' | 'rgroupLabel'
    abstract isMergeable(params: { from: TFrom, to: TTo }): boolean | ConflictResolver
    abstract handle(params: { from: TFrom, to: TTo }): void
}

// case 1: select and move the atom
activeSubStruct.addElement(atom)
const action = mergeManager.highlightHypothetical(activeSubStruct, {mergeStrategy: "direct"})

// case 2: create new bond from one atom to another
activeSubStruct.addElement(bondEndAtom)
const action = mergeManager.merge(activeSubStruct, {mergeStrategy: "opposite"})

// case 3: create new sgroup (template tool) to some atom or another sgroup
activeSubStruct.addElement(templateSGroup)
const action = mergeManager.merge(activeSubStruct, {mergeStrategy: "direct"})

Expected pros:

1. clean tool logic without handling merge and various elements
2. one structured place for the merge logic

Conserns:

1. Refactor all tools
2. Not clear how to implement "Atom on Atom with dragging -> form new Bond"

Next steps:

• have agreement and alignment
• prepare POC for Paste tool, Atom tool, Select tool
• demo POC
• integrate MergeManager to
  • AtomTool
  • BondTool
  • ChainTool
  • TemplateTool
  • SelectTool
  • PasteTool

[kaluginserg]

Instant feedback from discussion:

• @AnastasiiaPlyako - it's require to have big test coverage with e2e tests
• @Nitvex - confusion with "'direct' | 'opposite'" - maybe not needed
• @Nitvex - confusion with naming "highlightHypothetical"

[kaluginserg]

@AnastasiiaPlyako @yuleicul JFYI ^^