jperon / lyluatex

Alternative à lilypond-book pour lualatex
MIT License
58 stars 11 forks source link

lyluatex-options #243

Closed uliska closed 5 years ago

uliska commented 5 years ago

I think it is a good idea to factor out an additional module lyluatex-options. With that separation third-party code (including my own, I'm talking of a public package like lyluatexmp and private packages for custom project code) can either include either the option handling infrastructure or only the generic toolkit.

Probably it would also be a good idea to move those two Lua modules lyluatex-lib and lyluatex-options to a new separate LuaLaTeX package so it could be distributed and used independently from lyluatex altogether. But I'm not so sure how that would have to be done and how a pure-Lua package would actually be integrated in something like TeXLive (maybe it would simply be installed and not be used with \usepackage at all).

What should be done in any case is writing package author's documentation on how to make use of the option handling system. I think this really was a great way to make lyluatex configurable, and I think this could be generally useful for creating new Lua-based packages.

uliska commented 5 years ago

(PS: the code in this PR successfully passes make clean && make manual)

uliska commented 5 years ago

I've now hit a wall because I'm a bit confused about the exact handling of the Score object. What remains to be done is also moving the storage of the "current" options (those set by set_property) to lyluatex-options. I've managed (in a WIP branch) to store them in a second entry in the OPTIONS table and to move/update ly.set_property() accordingly.

However, where I'm stuck is to figure out how the Score object actually accesses these properties.

@jperon how exactly relate the local Score table and the self inside Score:XY() functions? And when in the process are the global, current and local properties actually merged?

uliska commented 5 years ago

Would it be correct to say that Score is the metatable, so assigning a key/value to that is alive throughout the rest of the document whereas Score:new() produces an instance that is then stored in ly.score?

That would mean that Score.XY behaves like a class variable while ly.score.XY like an instance variable.

In other words:

-- given that at some point this was written
Score.some_option = 'foo'
-- this returns 'foo':
ly.score.foo

while

-- after
ly.score.another_option = 'bar'
-- this will return nil:
Score.another_option

?

jperon commented 5 years ago

Would it be correct to say that Score is the metatable, so assigning a key/value to that is alive throughout the rest of the document whereas Score:new() produces an instance that is then stored in ly.score?

Nearly: the single fact that Score is the metatable isn't sufficient, but Score is the __index of the metatable. I could have written (l. 365 - 366):

setmetatable(o, {__index = self})

Using Score itself is just a slight optimization (avoiding the creation of a new table).

That would mean that Score.XY behaves like a class variable while ly.score.XY like an instance variable.

Yes.

In other words:

-- given that at some point this was written
Score.some_option = 'foo'
-- this returns 'foo':
ly.score.foo

while

-- after
ly.score.another_option = 'bar'
-- this will return nil:
Score.another_option

?

Yes.

jperon commented 5 years ago

I think it is a good idea to factor out an additional module lyluatex-options. With that separation third-party code (including my own, I'm talking of a public package like lyluatexmp and private packages for custom project code) can either include either the option handling infrastructure or only the generic toolkit.

Totally agree.

Probably it would also be a good idea to move those two Lua modules lyluatex-lib and lyluatex-options to a new separate LuaLaTeX package so it could be distributed and used independently from lyluatex altogether. But I'm not so sure how that would have to be done and how a pure-Lua package would actually be integrated in something like TeXLive (maybe it would simply be installed and not be used with \usepackage at all).

What should be done in any case is writing package author's documentation on how to make use of the option handling system. I think this really was a great way to make lyluatex configurable, and I think this could be generally useful for creating new Lua-based packages.

I agree with that too, but IMHO we should delay this part of changes after final 1.0 release.

uliska commented 5 years ago

I agree with that too, but IMHO we should delay this part of changes after final 1.0 release.

Ok, just to be sure there are no misunderstandings: I would now continue factoring out these two modules (there is some more work to be done to untangle the lyluatex-specific functions from generic ones), but for now we'll simply leave them as .lua files in the directory, without making any noise about it.

After a proper 1.0 release we can think of moving modules to separate LaTeX packages with arbitrary amounts of documentation, right?

In lyluatex.lua, l. 27: local OPTIONS = {} should be removed, as it isn't used any more in that file.

Yes, I've already noticed that and will go forward that way.

jperon commented 5 years ago

@uliska Right.

uliska commented 5 years ago

I renamed the branch to indicate that I'll do some more work on this.

uliska commented 5 years ago

@jperon I think this is now ready for review and hopefully merging.

NOTE: This function needs special consideration since it introduces some convolution that wouldn't be necessary if it were for lyluatex alone. I wanted to store both the option declarations as well as the actual entries in lyluatex-options and interact with them through a unified interface. However, this is made complicated through the fact that lyluatex uses the Score metatable to store the actual option values. My solution is:

I didn't see any side-effects, and make clean && make manual compiles correctly (great to have this as "unit tests" BTW). However, the approach is somewhat convoluted and I'm not sure I fully understand it.

uliska commented 5 years ago

If this is going to be merged I think I'm through with major updates for now ;-)

uliska commented 5 years ago

@jperon if you want you may have a look at the beginning stub of lyluatexmp for how I'm going to make use of the new code structure (the test file should compile on your computer too, just note that it depends on this unmerged branch in lyluatex).

uliska commented 5 years ago

I noticed an issue, but no solution yet: it seems that includes the at not processed at all!

uliska commented 5 years ago

Oops! It seems package options are not respected at all :-( Obviously there is something fishy with that operation around the double use of the Score object.

uliska commented 5 years ago

Some more tests seem to indicate that the last commit solved the issue.

jperon commented 5 years ago

@uliska While reviewing this, I'm in the process of rewriting in a more object-oriented style, so there will be quite substantial changes ; please don't add other changes before I push this work.

jperon commented 5 years ago

@uliska If you agree with my last commit, please feel free to merge the PR.