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

Mixin request for FCNoteEntryLayers #326

Open
rpatters1 opened this issue Aug 7, 2022 · 11 comments
Open

Mixin request for FCNoteEntryLayers #326

rpatters1 opened this issue Aug 7, 2022 · 11 comments
Assignees
Labels
enhancement New feature or request

Comments

@rpatters1
Copy link
Collaborator

rpatters1 commented Aug 7, 2022

Is your feature request related to a problem? Please describe.
This issue arose due to a common and justifiable misunderstanding about the intent of the built-in eachentry and eachentrysaved functions. It is important for a Finale-Lua programmer to understand how these work, and for this reason they are open source. They are written in Lua, so you don't have to know C++ to understand them.

The basic problem is that the eachentry... functions create local instances of variables from which they feed entry instances back to the loop. If the loop saves those instances out to a table or other variable external to the loop, the underlying C++ structures will go out of scope when Lua garbage collection runs. If your code then accesses one of these instances, Finale crashes. The eachentry... functions facilitate one-by-one entry processing but should never be used for aggregate entry processing or analysis.

What's needed is an easy way to traverse entries, allowing them to be cherry picked and processed after the loop finishes.

Describe the solution you'd like
I propose adding an FCX mixin for FCNoteEntryLayers (or FCNoteEntryLayer, see below) that would allow it to have its own eachentry iterator function. Then you could do something like:

local entries = mixin.FCXNoteEntryLayers()
entries:LoadForRegion(finenv.Region())
for entry in entries:eachentry() do
   -- cherry-pick entries for later processing
end
-- process cherry-picked entries.
entries:SaveAll() -- if you made changes to the entries

Describe alternatives you've considered
The only other alternative I can think of is standard library functions, but a mixin seems like a better DX.

Additional context
The biggest issue is that FCNoteEntryLayers is not exported into Lua at the moment. So until that could be added to RGP Lua (and/or if we want to support this with JW Lua), we might need to make this a mixin on FCNoteEntryLayer and create a "fake" collection in Lua.

Another issue to be aware of is that this loop would traverse layers with the layer number as the top loop variable. Programmers would have to get used to a different sequence of entries than for the standard eachentry... functions, unless we wanted to write a lot of Lua code in the mixin to make it the same.

@rpatters1 rpatters1 added the enhancement New feature or request label Aug 7, 2022
@rpatters1
Copy link
Collaborator Author

I've already thought of an alternative that might be better:

Wrap a table of FCNoteEntryCell instances to look like a collection, perhaps FCXNoteEntryCells, but I'm not sure if it's okay to create mixins on nonexistent Finale classes. (It might not be a big deal to add FCNoteEntryCells to the PDK Framework, but depending on that would require updating RGP Lua.)

Then our eachentry function could be an almost literal copy of the built-in eachentry function, and it would feed entries in the same order.

@ThistleSifter
Copy link
Member

At the moment it's only possible to add a mixin to a Finale class that exists, and even though you can add a mixin to __FCCollection, for example, you can't instantiate it so that doesn't solve the issue here.

That could be changed, I guess, to use a dummy object like an FCString, and then completely ignore the underlying metatable altogether (what would you call it? FCP* perhaps for polyfill?). I don't know whether it's worth it to do that (the mixin library itself would need some modifications to enable that to happen) or whether we're better off just doing standard Lua OOP with just a regular table.

@rpatters1
Copy link
Collaborator Author

I'm thinking a simple table wrapped as an object is the way to go. Using a fake class like FCString seems really unwise to me.

@Nick-Mazuk
Copy link
Member

I'm not familiar enough with how mixins work to comment on that, but +1 to the idea in general.

@rpatters1
Copy link
Collaborator Author

rpatters1 commented Aug 11, 2022

Here is another idea that requires no work in this repo. (But does require an RGP Lua update.)

  1. Add finale.FCNoteEntryCells collection class to the PDK Framework.
  2. Enhance the built-in eachentry iterator function to accept an optional third parameter that is an FCNoteEntryCells instance. If the parameter is supplied, eachentry adds every instance of FCNoteEntryCell that it creates to this collection. Upon exit, the collection is holding them all, so Lua garbage collection won't touch them as long as the collection is in scope.

With this you could write the following:

local entries = finale.FCNoteEntryCells()
for entry in eachentry(finenv.Region(), nil, entries) do
    -- cherry pick entries into a table
end
-- process cherry picked entries
entries:SaveAll() -- if any changes needed to be saved.

I think this can be implemented fairly easily. The devil is always in the details.

@ThistleSifter
Copy link
Member

Would a pure Lua solution be more preferable? That way any author who uses it doesn't need to bump the script's MinJWLuaVersion.

@ThistleSifter
Copy link
Member

@rpatters1 Any further thoughts on this? If you still want a Lua solution, I can start working on it.

@rpatters1
Copy link
Collaborator Author

rpatters1 commented Aug 25, 2022

I think a Lua solution makes sense. My original idea of using FCNoteEntryLayers may not be the best, though, because I think the new iterator should follow the same order and precedence as eachentry. So probably the pure Lua solution will be a flavor of the actual eachentry function that collects its FCNoteEntryCell instances into a table.

But those are just ideas. Feel free to be creative.

@ThistleSifter
Copy link
Member

Do you need a similar thing done for eachentrysaved, eachcell and eachstaff?

@rpatters1
Copy link
Collaborator Author

The only class that has the referencing issue is FCNoteEntry, so there is no problem with cherry-picking cells or staves. (Anyway, I believe eachstaff just feeds a staff number.)

As for eachentrysaved, I think rather than duplicate that function, the preferred pattern would be:

  • cherry pick with mixin.eachentry
  • save cherry picked items as needed with entry:GetParent():Save()

The only issue I see is that the underlying FCNoteEntryCell item might be saved multiple times, but I'm not aware of any reason it won't work.

@rpatters1
Copy link
Collaborator Author

rpatters1 commented Sep 5, 2022

I should add that FCNote has the same issue as FCNoteEntry. (because FCNoteEntry is a collection FCNote instances.) A mixin.eachnote function based on mixin.eachentry might be handy some day. But I'd suggest waiting till it is a requirement to implement it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants