Skip to content
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

Closed
wants to merge 2 commits into from

Conversation

th-we
Copy link
Member

@th-we th-we commented May 16, 2021

A fix for RemoveChild() and removal of unused code.

@th-we th-we requested a review from annplaksin May 16, 2021 12:43
@ahankinson
Copy link
Member

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.

@th-we
Copy link
Member Author

th-we commented May 16, 2021

It does not work the moment. How about keeping it in the libmei generator code so it is available for future use?

DDMAL/libmei#124

@th-we
Copy link
Member Author

th-we commented May 16, 2021

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 MEIFlattened was a SparseArray when you wrote the XML parser, but now it's a Dictionary. The createEntry() function also does not exist any more. That's at least the two things that I noticed.

@ahankinson
Copy link
Member

ahankinson commented May 17, 2021

Just looking back, the flattened structure was always a dictionary:

flat_doc = CreateDictionary();

It's possible that this wasn't changed with the move from v1 to v2.

createEntry(name) was renamed CreateElement(name, null);

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.

@annplaksin
Copy link
Member

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?

@ahankinson
Copy link
Member

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.

@annplaksin
Copy link
Member

@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...
It would be very cool to bring it back together.

@ahankinson
Copy link
Member

Aw, thanks @annplaksin ! Yeah, I think having the updateable support is the 'killer' feature, so we should definitely keep that bit.

@th-we
Copy link
Member Author

th-we commented May 17, 2021

@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.

DDMAL/libmei#124

@annplaksin
Copy link
Member

@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.

@ahankinson
Copy link
Member

"dormant" in the generator code means that it will become "undormant" when we re-generate the library to follow the next MEI release.

@th-we
Copy link
Member Author

th-we commented May 18, 2021

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.

DDMAL/libmei#124

@annplaksin
Copy link
Member

@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...
@ahankinson If it's okay for you, to keep it dormant, I won't object

@th-we
Copy link
Member Author

th-we commented May 18, 2021

@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.

@annplaksin
Copy link
Member

@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.

@th-we
Copy link
Member Author

th-we commented Aug 23, 2021

Should be discussed upstream on libmei.

@th-we th-we closed this Aug 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants