search

glycoinfo / GlycanBuilder2

7 stars 5 forks source link

Generated SVG files no longer have semantic ID attributes... #21

Closed edwardsnj closed 3 years ago

edwardsnj commented 3 years ago

It seems that in the battle to get SVG generation working again and the related issues with GroupingSVGGraphics2D, GroupingSVGGraphics2D is no longer putting semantic ID attributes on SVG element groups (representing monosaccharides and their links). These IDs are crucially important for Will's glycoTree sandbox.

See also #12 and #13.

e15d5605 commented 3 years ago

The SVG of glycans output by GlycanBuilder does not support Will's glycoTree sandbox.

edwardsnj commented 3 years ago

See my pull request #22 for a fix

MasaakiMatsubara commented 3 years ago

@edwardsnj

Hi Nathan,

Thank you for taking up this issue and for putting a proposal of implementation for that. However, we do not plan to support SVG export for Will's glycoTree sandbox in GlycanBuilder, because the GroupingSVGGraphcs2D is not supposed to output semantic ID attributes originally.

We instead developed the following web APIs for the SVG export:

WURCSToWURCSJSON-api https://api.glycosmos.org/glycanformatconverter/#2.5.2-wurcs-to-wurcsjson

WURCSJSONToSVG-api https://api.test.glycosmos.org/#wurcs-json-wurcs-json-to-svg-api-post

WURCSToWURCSJSON-api converts WURCS sequence to a JSON style format, and WURCSJSONTOSVG-api exports the SVG from the JSON. These APIs are developed for Will's glycoTree sandbox as a main purpose and the basic features are already implemented. The reason why we developed the systems as APIs is that we originally planed to use these in our glycan editing web tool, SugarDrawer.

edwardsnj commented 3 years ago

All that not withstanding, Will does not appear aware of this development to support his tools, but regardless, y'all are removing functionality and choosing not to add it back in for free, for no great reason. I've added it back because it is useful to us, y'all are free to ignore it in the output. Please merge so I don't have to maintain a separate fork.

yamadaissaku commented 3 years ago

After taking your comment, we investigated our changes again and then noticed that SVGGlycanRenderer originally has the fanctionality which you pointed out and it is accidentally removed. We are sorry about that. We will accept your pull-request. Thank you for your cooperation. I will be careful so that this kind of thing doesn't happen in the future.

In addition, I suggest that we invite you as a owner of github.com/glycoinfo repositories.

edwardsnj commented 3 years ago

In addition, I suggest that we invite you as a owner of github.com/glycoinfo repositories.

I've no problem with having existing developers review/gatekeep (and push-back and/or approve) pull requests. Most projects require a degree of dictatorship with respect to direction and quality control. Still, it would be nice to have the interests of those outside the core group be acknowledged and considered, and for projects like this to be run in a way that serves the community rather than the core group.

Thanks for merging the pull request!

Cheers!

edwardsnj commented 3 years ago

Actually, I realize now that my solution is incomplete and will not be correct for structures with undetermined linkage. I will provide another pull request in the same style as this one to fix this issue.