-
Notifications
You must be signed in to change notification settings - Fork 16
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
Some libmei changes #174
Some libmei changes #174
Conversation
I would like to keep the XML import code. It works, and it’s the biggest bit of work that someone would need to do if they wanted to support MEI import. |
It does not work the moment. How about keeping it in the libmei generator code so it is available for future use? |
It relies on functionality of libmei that isn't there any more or has changed in a way that is not compatible any more. For example, it seems that |
Just looking back, the flattened structure was always a dictionary: Line 1243 in d6ffc8e
It's possible that this wasn't changed with the move from v1 to v2.
A lot of time and energy went into writing that bit of code, with the hopes that someone could pick it up. I would be quite sad to see it go. |
Personally, I am a bit unhappy about the code generator and the libmei within sibmei differ... they did at least when started with the MEI 4 support. In an ideal world, we wouldn't mingle with the generated code at all, only with the generator. I understand the point of removing code that doesn't work, but it seems desirable to keep the import code somehow. The risk seems to me quite high, that the code will never come back again after it has been removed. @th-we How much work would it be to fix the import code? |
I'm not terribly happy with libmei at the moment. It was a project that was necessary at the time, but I feel the world has moved on. There were some really great ideas in there (the code generator is pretty cool) and quite a few real stinkers (like re-implementing the DOM API. :-( ) The most up-to-date version of libmei is actually the fork that is being used for Verovio. Now that MEI is at least part of my job description, we should plan on bringing these back together, and seeing what we should keep. |
@ahankinson I am still a fan though, especially because of the code generator. But maybe I am a bit biased after adapting it for .NET 🤪 It makes it easy to have an updatable MEI support for a language... |
Aw, thanks @annplaksin ! Yeah, I think having the updateable support is the 'killer' feature, so we should definitely keep that bit. |
@annplaksin I didn't check how much work it would be to bring the parser back to life. I suggest keeping it "dormant" in the generator code, but not in the generated code. |
@ahankinson If there is a reason to say something nice, it's the best to just say it. But yes, it's upgradable and customisable and for those two reasons quite a natural extension. @th-we You mentioned having it dormant in the generator already and I took that into account. I am not quite happy about this... some sort of a plan on how to bring the generator code and the generated code back together would be even more necessary when doing this. |
"dormant" in the generator code means that it will become "undormant" when we re-generate the library to follow the next MEI release. |
No, not in the way I "put it to sleep". I put the code in a separate variable that will not be output and added some comments on the state of the code to make clear it can not be used in its current state. |
@th-we Slightly stupid question: Is everything that differs in the current libmei already part of a PR? If we're going for putting it to sleep until the next version will be re-generated, we might update the code generator until then... |
@annplaksin Yes, at least I tried properly mirroring the changes here: https://github.com/DDMAL/libmei/pulls/th-we Maybe lets move the discussion there, close this pull request, and after updating the generator code update sibmei's libmei and a new pull request. Sorry for causing somewhat of a chaos here and thanks for all your patient feedback and thorough reviews. |
@th-we Okay, hopefully I'll find some time to check what I have done to the libmei after generating it, because there were already differences. Yeah, after all, it would have been wiser to look deeper into that while doing it... now I know it. |
Should be discussed upstream on libmei. |
A fix for RemoveChild() and removal of unused code.