Use mixins for transposition dialogs

[rpatters1]

This pull request changes transpose_chromatic.lua and transpose_by_step.lua to use mixin.FXCCustomLuaWindow. All state management is handled by the mixin library now.

The mixins required a bug fix and a minor enhancement. @ThistleSifter may wish to take a look as well.

[Nick-Mazuk]

Wow, that simplifies the code a lot!

Though note at this time if we were to merge this PR, anyone downloading the script from the website will run into errors. The bundler needs to be updated before this is merged.

Feel free to try and figure out how to update the bundler on your own. If not, I'll try to get to it as soon as I can.

[rpatters1]

I reckon just dumping the contents of the mixin directory into the combined script (when library.mixin is required) is the easy solution.

[ThistleSifter]

Looking good! It makes quite a difference.

I do have a couple of comments to make. In general, I'd put as much as possible on the dialog object itself, since that was the whole point of making them mutable. It simplifies setup code, makes the dialog the single source of truth, and it will reduce the chance of any issues occurring if we end up needing to destroy/recreate new window objects in the background.

FCMCtrlPopup.AddStrings
I had intentionally not allowed tables because it plays havoc with argument validation, or more specifically, reporting of argument errors. I would rather this was left as is and table.unpack was used instead, ie AddStrings(table.unpack(interval_names)).

transpose_by_step.lua - Window Handlers
All window handlers (Init, Close, etc) are passed a reference to the window object in the mixins, so that you never have to reach around into the surrounding scope. Eg

dialog:AddHandleOkButtonPressed(function(self)
self:GetControl("num_steps"):GetInteger()
end)

(I tend to prefer anonymous functions for handlers because the less that is global the better, and in this case I don't think it needs to be. But more on that in the next point. Actually, with mixins, nothing needs to be global at all except for the dialog itself.)


transpose_by_step.lua - Replacing on_ok with a dialog method
If I was writing this, I would make do_transpose_by_step a method of the dialog object, again to eliminate the need to reach into the surrounding scope. But at the very least, I would suggest the following alternative:

functioncreate_dialog_box()
...functiondialog:do_transpose()
do_transpose_by_step(self:GetControl("num_steps"):GetInteger())
end-- Utilising the handler being passed self as an argumentdialog:RegisterHandleOkButtonPressed(dialog.do_transpose)
returndialogend...functiontranspose_by_step()
...ifmodifier_keys_on_invokeandglobal_dialogthenglobal_dialog:do_transpose()
returnend...end

transpose_chromatic.lua
Pretty much the same stuff as before. Eg:

functioncreate_dialog_box()
localdialog=mixin.FCXCustomLuaWindow():SetTitle("Transpose Chromatic")
-- No need to perform extra check for finenv.RetainLuaStatedialog.interval_names= {
"Perfect Unison",
...
}
...dialog:CreatePopup(x_increment, current_y, "interval_choice"):AddStrings(table.unpack(dialog.interval_names)):SetWidth(140):SetSelectedItem(0)
...-- Make get_values a methodfunctiondialog:get_values()
localdirection=self:GetControl("direction_choice"):GetSelectedItem() >0and1or-1localinterval_choice=1+self:GetControl("interval_choice"):GetSelectedItem()
localdo_simplify= (0~=self:GetControl("do_simplify"):GetCheck())
localplus_octaves=self:GetControl("plus_octaves"):GetInteger()
localdo_preserve= (0~=self:GetControl("do_preserve"):GetCheck())
returndirection, interval_choice, do_simplify, plus_octaves, do_preserveendfunctiondialog:do_tranpose()
do_transpose_chromatic(self:get_values())
end...dialog:RegisterHandleOkButtonPressed(dialog.do_tranpose)
returndialogend...functiontranspose_chromatic()
...ifmodifier_keys_on_invokeandglobal_dialogthenglobal_dialog:do_transpose()
returnend...end

[rpatters1]

About the table-as-argument issue, it seems like the difference between allowing a table or using table.unpack is non-existent. Either way, the element-level type checking in AddString is going result in the same behavior. Why require the clutter in the caller?

[rpatters1]

About self being passed in to the handler, I implemented your suggested code and self is nil in the callback. (I can change self to global_dialog and it works.

I tracked the call stack to line 115 in FCMCustomLuaWindow.lua:

localfunctioncb()
localhandlers=private[self][f]
ifhandlers.Registeredthenhandlers.Registered() -- <== line 115endfor_, vinipairs(handlers.Added) dov(self)
endend

I'm wondering if self should be passed as an argument here. That seems to fix the issue.

[ThistleSifter]

localt= {"a", "b", "c", "d", true}
my_popup:AddStrings(table.unpack(t))
-- Error: path/to/my_plugin:3: bad argument #5 to 'AddStrings' etc, etcmy_popup:AddStrings(t)
-- Error: path/to/mixin/FCMCtrlPopup:159: bad argument #2 to 'AddString' etc, etc

Eh, it's your call.


Ah, yes that was an oversight. It should be handlers.Registered(self). Up to you if you want to fix it, otherwise I can get it tomorrow along with some other changes I wanted to submit.

[rpatters1]

Okay, I fixed the handler call and decided you have a point about the error reporting. Also considering that nowhere else in the mixin libraries is a table a valid input parmater for a strings. Calling table.unpack is a small issue.

[rpatters1]

With the help of @ThistleSifter I believe we have all the dialog handling down to a consistent and reusable package. To run a modeless dialog simply requires a create_dialog function and the following two lines of code:

dialog=dialogorcreate_dialog()
dialog:RunModeless()

The RunModeless function encapsulates all the behavior I described in #192. All this PR is waiting for now is a way to deploy it stand-alone.

[rpatters1]

I'm beginning to have a backlog waiting on this pull request. The hold-up is that it will not deploy correctly as a stand-alone script due to needing the entire mixin library to be dumped into the stand-alone script. The easy solution is going to be: if the mixin library is required, dump the entire mixin subdirectory as well.

[rpatters1]

I'm using this to piggy-back some comments in tie.lua. Awaiting the okay from @Nick-Mazuk (and merge if you wish).

[Nick-Mazuk]

Looks good to me. Merging.