Add executables list to plugin API

[JonathanFeenstra]

This adds the IExecutable and IExecutablesList interfaces and the IOrganizer::executablesList method to expand the plugin API.

[Holt59]

I'm okay with the idea but I don't really like the list interface:

• The original code in the main repository does not use shared pointers, so creates new object every time. The getByXXX should probably returns simple references.
• I know titleExists is the name in the original code but that's a pretty bad name... Maybe hasExecutableWithTitle or simply contains (since this is an executable list).
• I don't like that executables() since you will have to copy everything... Maybe try returning a range of IExecutable& that one can iterate over? I'm pretty sure there is something similar somewhere in uibase that can then be bound in Python.

A bit more complicated but I really don't like that arguments() returns a simple string... I know it's the case in the main repository, so for now it will have to do but maybe add some warning that this could be changed in the future (to return a QStringList)?

[JonathanFeenstra]

I don't like that executables() since you will have to copy everything... Maybe try returning a range of IExecutable& that one can iterate over? I'm pretty sure there is something similar somewhere in uibase that can then be bound in Python.

Do you mean like how IFileTree has the __iter__ method in Python by implementing iterators? The ExecutablesList already does something similar, but I don't know how to make it work with IExecutable&. Or alternatively, would it be possible to return a std::generator?

A bit more complicated but I really don't like that arguments() returns a simple string... I know it's the case in the main repository, so for now it will have to do but maybe add some warning that this could be changed in the future (to return a QStringList)?

I think we could change Executable::arguments to return a QStringList and just update all calls to use join(" ") on that list.

[Holt59]

I don't like that executables() since you will have to copy everything... Maybe try returning a range of IExecutable& that one can iterate over? I'm pretty sure there is something similar somewhere in uibase that can then be bound in Python.

Do you mean like how IFileTree has the __iter__ method in Python by implementing iterators? The ExecutablesList already does something similar, but I don't know how to make it work with IExecutable&. Or alternatively, would it be possible to return a std::generator?

I think the best way is to return a std::generator<const IExecutable&>, this should be doable. std::generator are already used for IFileTree so this should be workable with Python without to much issue.

A bit more complicated but I really don't like that arguments() returns a simple string... I know it's the case in the main repository, so for now it will have to do but maybe add some warning that this could be changed in the future (to return a QStringList)?

I think we could change Executable::arguments to return a QStringList and just update all calls to use join(" ") on that list.

You would have to handle quoted arguments, etc., so that's not that simple. I think for now we can keep it as a single QString.

[JonathanFeenstra]

You would have to handle quoted arguments, etc., so that's not that simple.

That would be a problem if it was the other way around (splitting on spaces), but ExecutableInfo::arguments already returns a QStringList, so I think calling join(" ") on that should work fine, right? The Executable constructor already does it.

[Holt59]

You would have to handle quoted arguments, etc., so that's not that simple.

That would be a problem if it was the other way around (splitting on spaces), but ExecutableInfo::arguments already returns a QStringList, so I think calling join(" ") on that should work fine, right? The Executable constructor already does it.

I think at some point I tried to make that change and stumble upon something annoying but maybe I'm misremembering... Feel free to try to modify it this way.

[JonathanFeenstra]

I think I just found what you're probably referring to: Executable has another method to set the arguments using a QString&. That method is used for deserialization, so there you would have to go the other way around and parse the string into a list. I'll just keep it a QString& for now then. Should I add a warning in the documentation that the return type might change in the future?

[JonathanFeenstra]

Unfortunately, I can't use std::generator without breaking installer_fomod_csharp which is still on CXX20. The other places in uibase that use std::generator use QDLLEXPORT (see this PR: #173), but I can't do that with a virtual function.

I could do something similar to what I did with profiles and just return a QStringList with the titles of the executables so you can retrieve them using getByTitle, but that would be less efficient if you are looking for an executable by for example the binary name.

[Holt59]

Unfortunately, I can't use std::generator without breaking installer_fomod_csharp which is still on CXX20. The other places in uibase that use std::generator use QDLLEXPORT (see this PR: #173), but I can't do that with a virtual function.

It's not about QDLLEXPORT, it's about including the file or not. I don't think the FOMOD C# installer needs access to the executable, the issue is that you are including #include "iexecutableslist.h" in moinfo.h. An easy fix is to forward declare IExecutablesList in moinfo.h without including iexecutablelist.h.

[JonathanFeenstra]

You're right, that fixed it. However, I have another issue in plugin_python. If I bind the IExecutablesList::executables method like this:

py::class_<IExecutablesList>(m, "IExecutablesList")
    .def("executables",
         [](IExecutablesList* executablesList) {
             return make_generator(executablesList->executables());
         })

I get build errors: 'MOBase::IExecutable': cannot instantiate abstract class. How should I deal with that?

[Holt59]

You're right, that fixed it. However, I have another issue in plugin_python. If I bind the IExecutablesList::executables method like this:

py::class_<IExecutablesList>(m, "IExecutablesList")
    .def("executables",
         [](IExecutablesList* executablesList) {
             return make_generator(executablesList->executables());
         })

I get build errors: 'MOBase::IExecutable': cannot instantiate abstract class. How should I deal with that?

Did you include the iexecutableslist.h in the file where you wrote the binding?

[JonathanFeenstra]

Yes, in basic_classes.cpp, but the error points to this line: https://github.com/ModOrganizer2/modorganizer-plugin_python/blob/b81646562c5429c733b59b3e6fa96515c83210bb/src/pybind11-utils/include/pybind11_utils/generator.h#L39

[Holt59]

Yes, in basic_classes.cpp, but the error points to this line: https://github.com/ModOrganizer2/modorganizer-plugin_python/blob/b81646562c5429c733b59b3e6fa96515c83210bb/src/pybind11-utils/include/pybind11_utils/generator.h#L39

What type did you use with std::generator? IExecutable? Or std::shared_ptr<IExecutable>? IExecutable&?

[JonathanFeenstra]

std::generator<const IExecutable&>

[Holt59]

You can try to replace the lambda with:

.def("__next__", [](state& s) {
    if (s.it != s.g.end()) -> decltype(auto) {
        decltype(auto) v = *s.it;
        s.it++;
        return v;
    }
    else {
        throw py::stop_iteration();
    }
});

...but that would need some testing.

[JonathanFeenstra]

This gives me error code C2440 'return': cannot convert from '_Rty' to '_Rty &&' at the return line

[Holt59]

This gives me error code C2440 'return': cannot convert from '_Rty' to '_Rty &&' at the return line

Can you push the uibase and plugin python changes to Github? I'll try to have a look when I can.

[JonathanFeenstra]

Alright, thanks

[Holt59]

Alright, thanks

Replace the -> decltype(auto) with -> T, it should compile, I'll let you check if it runs properly.

[JonathanFeenstra]

It works if I add py::return_value_policy::reference, otherwise I get this:

I'm not sure if that would also work for the other functions that use make_generator though. I'll check, maybe we need to have two variations of make_generator if the others require copy instead of reference?

[Holt59]

I'm not sure if that would also work for the other functions that use make_generator though. I'll check, maybe we need to have two variations of make_generator if the others require copy instead of reference?

I don't think the other use needs that because they return std::shared_ptr, but you can pass the args for __next__ to make_generator, e.g.

// create a Python generator from a C++ generator
//
template <typename T, typename... Args>
auto make_generator(std::generator<T> g, Args&&... args)
{
    using state = detail::generator_state<T>;

    namespace py = pybind11;
    if (!py::detail::get_type_info(typeid(state), false)) {
        py::class_<state>(py::handle(), "iterator", pybind11::module_local())
            .def("__iter__",
                    [](state& s) -> state& {
                        return s;
                    })
            .def(
                "__next__",
                [](state& s) -> T {
                    if (s.it != s.g.end()) {
                        decltype(auto) v = *s.it;
                        s.it++;
                        return v;
                    }
                    else {
                        throw py::stop_iteration();
                    }
                },
                std::forward<Args>(args)...);
    }

    return py::cast(state{std::move(g)});
}

And then you pass py::return_value_policy::reference to make_generator when needed.

[JonathanFeenstra]

The changes in make_generator caused IFileTree.walk() to yield an extra None after the last item. Do you have an idea what the cause of that might be? It doesn't do that for the executables method.

[Holt59]

The changes in make_generator caused IFileTree.walk() to yield an extra None after the last item. Do you have an idea what the cause of that might be? It doesn't do that for the executables method.

Maybe try replacing the decltype(auto) v = *s.it; by T v = *s.it;?