feat: sync webcomponent props with attributes

[tpoisseau]

Refs: #229
Refs: #231

New PR, without copy-paste events handlings

Reminder: sync webcomponent properties with attributes, and propagate vaadin events (need more discussions)

[tpoisseau]

The only modification I kept from original PR with copy-paste handling is to prevent to propagate events ctrl+c / ctrl+v to java event manager.

It will avoid to display errors in console (copy is not working outside a valid user-interaction in browser, at least for firefox).
And paste can't be implemented in sync.

[targos]

@artaius do you think it will work for you?

[artaius]

Thanks for the changes. As far as I can say this looks good to me.

The only missing thing I can see is, that the molecule coordinates are not yet considered on changes nor are they accessible by properties/attributes. If this is supported as well, we can start to deploy a first prototype to our users. The coords are crucial as people wont accept if the display of the molecules change upon pasting.

Otherwise I found the solutions to

• set custom event names in Vaadin (41fecf9)
• handle the clipboard directly though Vaadin's Element API.

[targos]

I think we can use the "idcode idcoordinates" convention for molecules, but what about reactions? it seems that the space character is already used to separate reactants from products?

[thsa]

For encoding reactions including mapping, coordinates, etc, you use the ReactionEncoder

[artaius]

I think we can use the "idcode idcoordinates" convention for molecules, but what about reactions? it seems that the space character is already used to separate reactants from products?

Fully agree, lets keep this concatination of "idcode coords" for molecules. This way we also get change events on coord changes, what makes sense.

For reactions I think we can use the similar thing (as @thsa suggested). I think the following two methods should do. @thsa Can you please confirm that this makes sense for the editor (especially the mode param)?

// get reaction idcode
String reactionIdCode = ReactionEncoder.encode(new Reaction(), true, ReactionEncoder.INCLUDE_MAPPING | ReactionEncoder.INCLUDE_COORDS | ReactionEncoder.INCLUDE_DRAWING_OBJECTS | ReactionEncoder.INCLUDE_CATALYSTS | ReactionEncoder.RETAIN_REACTANT_AND_PRODUCT_ORDER);

// parse reaction idcode
Reaction decodedReaction = ReactionEncoder.decode(reactionIdCode, true);

[thsa]

It depends a little on the purpose. A typical case would be an encoding of a reaction with atom coords, mapping and original order of the reactants:
String reactionIdCode = ReactionEncoder.encode(new Reaction(), true, ReactionEncoder.INCLUDE_MAPPING | ReactionEncoder.INCLUDE_COORDS | ReactionEncoder.RETAIN_REACTANT_AND_PRODUCT_ORDER);

If catalysts structures are needed, the a typical UI would probably not allow drawing them within the reaction editor, but do it in a separate step.

If you need a canonical reaction representation, e.g. to remove duplicates from a database, then you would not include coords, mapping, catalysts. And you would allow the encoder to put reactants into a canonical order.

[cloudflare-workers-and-pages]

Deploying openchemlib-js with    Cloudflare Pages

Latest commit:	75f7ab3
Status:	✅  Deploy successful!
Preview URL:	https://3fa2d904.openchemlib-js.pages.dev
Branch Preview URL:	https://229-improve-web-component-wi.openchemlib-js.pages.dev

View logs

[artaius]

@thsa Thanks for the comments.

I would then go with @thsa proposal, i.e.

// get reaction idcode
String reactionIdCode = ReactionEncoder.encode(new Reaction(), true, ReactionEncoder.INCLUDE_MAPPING | ReactionEncoder.INCLUDE_COORDS | ReactionEncoder.RETAIN_REACTANT_AND_PRODUCT_ORDER);

// parse reaction idcode
Reaction decodedReaction = ReactionEncoder.decode(reactionIdCode, true);

[tpoisseau]

I can use ReactionEncoder.encode(this.getReaction(), true, ReactionEncoder.INCLUDE_MAPPING | ReactionEncoder.INCLUDE_COORDS | ReactionEncoder.RETAIN_REACTANT_AND_PRODUCT_ORDER); / ReactionEncoder.decode(reactionIdCode, true) for reaction.

And ${molecule.getIDCode()} ${molecule.getIDCoordinates()} / const [idcode, coordinates] = this.idcode.split(' '); Molecule.fromIDCode(idcode, coordinates).

ReactionEncoder store some constants about separator it could use. Do we have constants to use for molecule separator ?

[thsa]

For testing and comparison to the Java version you could download openmolecules.org/idcodeexplorer.jar. It is an editor that has molecule and reaction modes, both for molecules and substructures. On the fly it creates idcodes/rxncodes with every editor change. You can also paste an idcode or rxncode and it converts it to the respective structures/reactions...

[artaius]

@tpoisseau Looks like if not all overloads of ReactionEncoder methods are present in the current OCL-JS.

• ReactionEncoder.decode(String) exists but ReactionEncoder.decode(String, boolean) doesn't.
• ReactionEncoder.encode(Reaction) exists but ReactionEncoder.encode(Reaction, boolean, int) doesn't.

[tpoisseau]

@artaius Where could I find ID-Code encoding options used by idcodeexplorer.jar ?

Give me

gOp@DjWkjj`@ gOp@DjWkjj`@!gOp@DjWkjj`@##!BbOvw?_x@?g?~_{_} !Bb@Jw@oy??`C~@K\B !Bb@K~_{\BbOvH?_y?##

webcomponent transform it to

gOp@DjWkjj`@ gOp@DjWkjj`@!gOp@DjWkjj`@##!RbGwW_Wx@_c}~O}]}GThB !RbOsW?Gx?_`A~@M_|GYkB !R|Gq~_{]||Gwp_Wy?GT[|##

Which look similar.

[artaius]

This is just a minor change of the coordinates. Not sure where this comes from. @thsa might this come from some floating point inaccuracy, or because of the different drawing area sizes.

@thsa Could you please give a hint about where to find the source code of the idcodeexplorer?
However I don't think this is a problem.

[artaius]

@tpoisseau Do you have any plans to release a (beta) version of the current code to npm? This would be great as I could then already adapt the Vaadin project to the new package and test it as well.

[tpoisseau]

As soon this PR is merged, we can release I guess, I'm not sure a beta is needed.

Changes concern mainly the webcomponent, and add options to ReactionEncoder encode-decode.

@artaius can you check the defaults params for ReactionEncoder to confirm the transition will be smooth please ?
https://github.com/cheminfo/openchemlib-js/pull/234/files#diff-a150fb53afee945f23f3fe7c176007111f3ba81bb0d188af4077d4a0fc4caa37

[thsa]

the source code of idcodeexplorer is not public, because it is meant as a test application for myself.

[thsa]

the encoding part of the idcodeexplorer source is:

			if (mComboBoxMode.getSelectedIndex() == MODE_REACTION) {
				Reaction rxn = mDrawPanel.getDrawArea().getReaction();
				mTextFieldIDCode.setText(ReactionEncoder.encode(rxn, false,
						ReactionEncoder.INCLUDE_DEFAULT | ReactionEncoder.RETAIN_REACTANT_AND_PRODUCT_ORDER));
				mTextFieldSmiles.setText(!rxn.isFragment() ?
					  IsomericSmilesCreator.createReactionSmiles(rxn)
					: IsomericSmilesCreator.createReactionSmarts(rxn));
				}
			else {
				StereoMolecule mol = new StereoMolecule(mMolecule);
				MoleculeStandardizer.standardize(mol,0);
				Canonizer canonizer = new Canonizer(mol, Canonizer.ENCODE_ATOM_CUSTOM_LABELS);
				mTextFieldIDCode.setText(canonizer.getIDCode() + " " + canonizer.getEncodedCoordinates());
				mTextFieldSmiles.setText(mMolecule.isFragment() ?
						IsomericSmilesCreator.createSmarts(mMolecule)
					  : IsomericSmilesCreator.createSmiles(mMolecule));
				}
			}

[tpoisseau]

ok, so you encode without absolute coordinates, So I guess it's normal import idcode from idcodeexplorer do not places the reactants exactly to same place when decode with ensureCoordinates to true.

[artaius]

As soon this PR is merged, we can release I guess, I'm not sure a beta is needed.

Changes concern mainly the webcomponent, and add options to ReactionEncoder encode-decode.

@artaius can you check the defaults params for ReactionEncoder to confirm the transition will be smooth please ? https://github.com/cheminfo/openchemlib-js/pull/234/files#diff-a150fb53afee945f23f3fe7c176007111f3ba81bb0d188af4077d4a0fc4caa37

@tpoisseau As far as I can say this looks good to me... 👍 And I agree, we don't need a beta.

[targos]

@tpoisseau Can you prepare a squash commit message and I'll merge?

[tpoisseau]

feat: sync webcomponent props with attributes

* feat: add options to `ReactionEncoder.decode` and `ReactionEncoder.decode`

Refs: https://github.com/cheminfo/openchemlib-js/issues/229

[targos]

I just changed it to this for release-please:

feat: sync webcomponent props with attributes

Refs: https://github.com/cheminfo/openchemlib-js/issues/229

feat: add options to `ReactionEncoder.decode` and `ReactionEncoder.decode`

See https://github.com/googleapis/release-please#what-if-my-pr-contains-multiple-fixes-or-features