jatinchowdhury18 / ChowDSP-VCV

ChowDSP modules for VCV Rack
GNU General Public License v3.0
81 stars 8 forks source link

RFC: Fix cardinal dialogs #31

Closed falkTX closed 2 years ago

falkTX commented 2 years ago

I tested with this and module works, file is saved as expected. Please have a look, does this seem ok to you?

I am not sure.. why do we need std::bind again?

jatinchowdhury18 commented 2 years ago

Originally, I had written the argument to use a lambda with a capture initializer, but unfortunately that was not strictly compatible with C++11. The purpose of the std::bind was to do the same thing in a way that is C++11-compatible.

The motivation for using a move instead of a copy is that (in my experience) the move operation has less chance of introducing a lifetime issue when the function is eventually called on a thread different from the one it was created on. Since I suppose you're the one writing the code that calls the action, and it works fine when tested on your end, I think the way you've written it should be fine.

Typically when I write code designed to execute an action defined elsewhere, I prefer to use a templated argument rather than a std::function, for example:

template<typename ActionType>
void async_dialog_filebrowser(other_args..., ActionType&& action) {
    action(args...);
}

That way the argument could be a std::function, std::bind, lambda, etc.