rism-digital / verovio

🎵 Music notation engraving library for MEI with MusicXML and Humdrum support and various toolkits (JavaScript, Python)
https://www.verovio.org
GNU Lesser General Public License v3.0
687 stars 185 forks source link

Add an option to specify selected attributes to be added to the SVG #1991

Closed wergo closed 3 years ago

wergo commented 3 years ago

It would be useful for faster DOM traversing with jQuery to have the @n attribute for staff and layer written into the SVG output. Currently, this information has to be found in the MEI directly which is less convenient (and probably slower).

So a staff element would then look like: <g id="staff-0001234" class="staff" n="2">

(Feature request)

ahankinson commented 3 years ago

If you are concerned about performance and are hitting bottlenecks, native JS lookups are much faster than using jQuery. If you need jQuery compatibility you can do DOM traversal in native JS, and then convert your results to jQuery objects when you find them.

lpugin commented 3 years ago

@wergo this would unfortunately not be valid SVG. One simple and generic way around it is to put in the MEI @type values you need to have in the SVG. Verovio will pass them as @class in the SVG. So if your MEI is <staff n="1" type="n1"> you will get <g id="staff-0001234" class="staff n1">. Can this work for you?

wergo commented 3 years ago

Thank you for your quick replies!

@ahankinson, could you give an example on how you would use "native JS lookups" for xml encodings (directly on the text?)? (Sorry, I am not quite native in JS myself.)

@lpugin This would require the @type attribute in the MEI code? I would rather prefer a solution that runs with the "normal" MEI encodings.

lpugin commented 3 years ago

This would require the @type attribute in the MEI code?

Yes. You can add this easily with an XSLT.

I would rather prefer a solution that runs with the "normal" MEI encodings.

I understand. But the problem is that this type of need is fairly project-specific. So you need @n, but somebody else will need @pname, or @place, or midi attributes, and so on. (We already had requests for all of these.) And we will end up replicating the entire MEI into the SVG ;-) I am happy to hear about other ideas, though.

ahankinson commented 3 years ago

If you have the XML parsed into a DOM structure, you can use document.querySelectorAll:

https://developer.mozilla.org/en-US/docs/Web/API/Document/querySelectorAll

I'm not suggesting that you drop jQuery automatically if that's what you're comfortable with, but if you are hitting bottlenecks in the DOM traversal and lookups you may want to look at optimizing those with native calls.

wergo commented 3 years ago

I understand. But the problem is that this type of need is fairly project-specific. So you need @n, but somebody else will need @pname, or @place, or midi attributes, and so on. (We already had requests for all of these.) And we will end up replicating the entire MEI into the SVG ;-) I am happy to hear about other ideas, though.

@lpugin I understand and I see the problem. A pitty that a pure @n would not generate valid SVG. So, I will find another solution. Thanks a lot for your help.

@ahankinson Thanks a lot for pointing me to this. I will have a thorough look before asking more questions.

musicog commented 3 years ago

@lpugin

But the problem is that this type of need is fairly project-specific. So you need @n, but somebody else will need @pname, or @place, or midi attributes, and so on. (We already had requests for all of these.) And we will end up replicating the entire MEI into the SVG ;-) I am happy to hear about other ideas, though.

Reopening with a humble proposal: provide an optional "attributesToRender" argument on the toolkit that takes a list of additional attributes to render. Then, carry these over into the SVG as data-* attributes (in these example, data-n, data-pname, data-place).

Advantages:

What do you think?

lpugin commented 3 years ago

Thanks for the proposal. Making it generic and flexible is key, and your idea works well on that point. The use of data- attributes will work only with SVG 2.0, if this ever comes out. But we already use them with the --svg-html5 option. See #1357 and #1428. I am fine with this and we do not need to discuss in again in my opinion as long as nobody complains that the SVG is not valid ;-)

The content for the new option - I would suggest svgAdditionalAttributes or something like this - will probably have to be JSON object. I would expect something like:

{
   "note": ["pname", "oct"],
   "staff": ["n", "follows"]
}

Is it what you have in mind?

(There is a side issue here we need to take into account (see https://github.com/rism-ch/verovio/issues/1706#issuecomment-701492474), namely that we would have the option --svg-additional-attributes-file for the command-line or python versions, and that one would not be available in the JS version. With the JS it will be JSON within JSON. Is this possible?)

The best would be to pass the option to the SVGDeviceContext object. We want to avoid having to resolve string element and attribute names dynamically into classes, which is not straightforward at all in C++. I think we can avoid it by relying on the Object::GetAttributes method. The steps would be:

lpugin commented 3 years ago

Another idea if we want to avoid the JSON option complication is to have a repeatable option (--svg-additional-attribute) that take values such as note@pname, note@oct, staff@n, staff@follows

wergo commented 3 years ago

Thanks @lpugin for these suggestions:

Another idea if we want to avoid the JSON option complication is to have a repeatable option (--svg-additional-attribute) that take values such as note@pname, note@oct, staff@n, staff@follows

I have implemented them now in #2361.