Skip to content

Conversation

@JonathanFeenstra
Copy link
Contributor

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

@Holt59
Copy link
Member

Holt59 commented Jan 18, 2026

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
Copy link
Contributor Author

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
Copy link
Member

Holt59 commented Jan 18, 2026

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
Copy link
Contributor Author

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
Copy link
Member

Holt59 commented Jan 19, 2026

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
Copy link
Contributor Author

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants