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

Toolkit methods getHumdrum and getHumdrumFile is not correctly named #3827

Open
ahankinson opened this issue Oct 14, 2024 · 7 comments
Open
Assignees

Comments

@ahankinson
Copy link
Contributor

The get* methods tend to return a value from the internal state of the toolkit, while the render* methods will render the toolkit data to a file. So getHumdrumFile(filename: str) -> bool should probably be renamed to renderToHumdrumFile(filename: str) -> bool to match.

Additionally, getHumdrum() -> str should probably be renamed renderToHumdrum() -> str

@craigsapp
Copy link
Contributor

I am thinking about this: The output of getHumdrum() is more than renderToHumdrum(). Usually it is used to capture an intermediate state when loading Humdrum data on its way to conversion into MEI (rather than conversion from MEI to Humdrum). I am traveling to Europe tomorrow and will have jetlag and conferences after that so remind me in a few weeks if I forget too look into this.

@ahankinson
Copy link
Contributor Author

Usually it is used to capture an intermediate state when loading Humdrum data on its way to conversion into MEI (rather than conversion from MEI to Humdrum).

OK; what is getHumdrumBuffer for? That seems like it's also a place to get humdrum data, possibly not as a conversion?

@craigsapp
Copy link
Contributor

Yes, that is something to check related to names of output functions (these were all added 8 years ago, so I will have to read the code to see what they do and how to normalized their names).

@lpugin
Copy link
Contributor

lpugin commented Oct 14, 2024

GetHumdrumBuffer is the equivalent of GetCString (and SetCString). I am not sure why Humdrum has a dedicated function. @craigsapp wouldn't be possible to use Set/GetCString instead and to drop them?

@craigsapp
Copy link
Contributor

Here is a schematic of import of Humdrum into verovio (names of variable/functions may be approximate):

Screenshot 2024-10-14 at 04 42 50

I need access to the intermediate Humdrum data after it has been transformed and before it is converted to MEI. In this case renderToHumdrum is misleading. Command line: verovio file.krn -t humdrum gives the intermediate version of the Humdrum data before being processed by iohumdrum.cpp.

For conversion of MEI to Humdrum, the schematic is (approximately, but likely):

Screenshot 2024-10-14 at 04 48 33

This is the case where it can be renamed appropriately to renderToHumdrum.

Here is the mei2hum converter: https://github.com/craigsapp/humlib/blob/master/src/tool-mei2hum.cpp

@lpugin
Copy link
Contributor

lpugin commented Oct 14, 2024

OK. My understanding would be that we can:

  • make SetHumdrumBuffer private or protected
  • rename GetHumdrum and GetHumdrumFile to RenderToHumdrum and RenderToHumdrumFile respectively
  • make GetHumdrum in the C wrapper use SetCString and GetCString
  • drop GetHumdrumBuffer completely

Does it sound right?

@craigsapp
Copy link
Contributor

Yes that sounds about right. SetHumdrumBuffer should only be needed internally in verovio (in Toolkit class, I would have to check how InputHumdrum class interacts with it). The old names should be kept for a while with deprecation warnings (in Javascript version of verovio).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants