Add instanceName and profiles methods to plugin API

[JonathanFeenstra]

See ModOrganizer2/modorganizer#2321

[Holt59]

I don't understand why you made a wrapper for IProfile? Wrappers are used to inherit class from Python, but there is no reason to inherit IProfile from Python.

[JonathanFeenstra]

I had issues with my original profiles function when calling it from Python, an exception occurred on the C++ side while trying to delete the shared_ptr. I noticed that in other parts of the plugin API where shared_ptr to MOBase interfaces are returned, these wrappers were used. Based on the comments (custom type_caster<> for shared_ptr<> to keep the Python object alive), I thought this was the normal solution to that issue.

[Holt59]

I had issues with my original profiles function when calling it from Python, an exception occurred on the C++ side while trying to delete the shared_ptr. I noticed that in other parts of the plugin API where shared_ptr to MOBase interfaces are returned, these wrappers were used. Based on the comments (custom type_caster<> for shared_ptr<> to keep the Python object alive), I thought this was the normal solution to that issue.

I think you simply need to add a line here for IProfile: https://github.com/ModOrganizer2/modorganizer-plugin_python/blob/master/src/mobase/pybind11_all.h#L142-L144

[JonathanFeenstra]

That doesn't seem to be enough, I get the same error when I remove IProfile from the wrappers and add it back to basic_classes.cpp and add the MO2_PYBIND11_SHARED_CPP_HOLDER(MOBase::IProfile) line.

[Holt59]

That doesn't seem to be enough, I get the same error when I remove IProfile from the wrappers and add it back to basic_classes.cpp and add the MO2_PYBIND11_SHARED_CPP_HOLDER(MOBase::IProfile) line.

I think the issue is that profile() returns a raw pointer, so there is two different types of C++ managed object in the interface, which the Python interface will not really like... Best option would probably be to update the interface so that profile returns a shared_ptr<IProfile> (and change the unique_ptr in OrganizerCore to a shared_ptr), but that will probably require small changes in many places.

[JonathanFeenstra]

I tried changing profile to return a shared_ptr<IProfile> in uibase and updating modorganizer and all C++ plugins that call that function to reflect the change, but I'm still getting the same error:

[AnyOldName3]

Are you constructing a shared_ptr from an existing shared_ptr, or are you constructing it from an existing instance that isn't managed by shared_ptr? If the latter, that's illegal. IIRC, that's why we have the proxy classes in the main repo - we can have a proxy with shared ownership even if the thing it's proxying has a different ownership model.

[Holt59]

I tried changing profile to return a shared_ptr<IProfile> in uibase and updating modorganizer and all C++ plugins that call that function to reflect the change, but I'm still getting the same error:

I think you also need to update the simple binding for IProfile to indicates the underlying storage here https://github.com/ModOrganizer2/modorganizer-plugin_python/blob/master/src/mobase/wrappers/basic_classes.cpp#L883:

py::class_<IProfile, std::shared_ptr<IProfile>>(m, "IProfile")

This should be similar to FileTreeEntry (

modorganizer-plugin_python/src/mobase/wrappers/pyfiletree.cpp

Lines 87 to 89 in 648db4d

	auto fileTreeEntryClass =
	py::class_<FileTreeEntry, std::shared_ptr<FileTreeEntry>>(m,
	"FileTreeEntry");

).

[JonathanFeenstra]

Are you constructing a shared_ptr from an existing shared_ptr, or are you constructing it from an existing instance that isn't managed by shared_ptr? If the latter, that's illegal. IIRC, that's why we have the proxy classes in the main repo - we can have a proxy with shared ownership even if the thing it's proxying has a different ownership model.

I changed m_CurrentProfile in organizercore.h to be a shared_ptr, that's what's being returned.

I tried changing profile to return a shared_ptr<IProfile> in uibase and updating modorganizer and all C++ plugins that call that function to reflect the change, but I'm still getting the same error:

I think you also need to update the simple binding for IProfile to indicates the underlying storage here https://github.com/ModOrganizer2/modorganizer-plugin_python/blob/master/src/mobase/wrappers/basic_classes.cpp#L883:

py::class_<IProfile, std::shared_ptr<IProfile>>(m, "IProfile")

This should be similar to FileTreeEntry (

modorganizer-plugin_python/src/mobase/wrappers/pyfiletree.cpp

Lines 87 to 89 in 648db4d

	auto fileTreeEntryClass =
	py::class_<FileTreeEntry, std::shared_ptr<FileTreeEntry>>(m,
	"FileTreeEntry");

).

That seems to have fixed it. Should I keep the change of the profile return type and create PRs in all repos that require it, or should I just apply the fix in the binding?

[Holt59]

That seems to have fixed it. Should I keep the change of the profile return type and create PRs in all repos that require it, or should I just apply the fix in the binding?

I think you need to keep the change to profile otherwise this will cause issue in the Python bindings.

[JonathanFeenstra]

I think the build is failing because it's not using my version of uibase from: ModOrganizer2/modorganizer-uibase#178