-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Plugin skeleton generator #117
base: master
Are you sure you want to change the base?
Conversation
Hi Tym, Great start!! Some small remarks:
== Thunder Development Tools == Choose one of the following tools: (To quit, type q)
You have picked:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a great tool indeed, well done! I added some remarks, and if something is not clear, feel free to reach out. Small side note: I reviewed the python code, and the generated code in examples, since it was easier than reviewing templates.
Also, it would be great if you could simply react to or comment each of my remarks after you take them into consideration, way easier to track what's changed 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- set execution bits for py files
- remove examples in the end
- start from ThunderDevTTools:
Error running the script: python3: can't open file '/home/mfransen/src/ThunderWIP/PluginSkeletonGenerator/ThunderTools/PluginSkeletonGenerator/PluginSkeletonGeneratror.py': - make adding config conditional (see below as well)
- in plugin c'tor also call PluginHost::IPlugin, JSONRPC etc. constructors (like we now do for the other interfaces, classes)
- config, also call constructor for member(s) in initializer list
- for a lot of the methods there is only a declaation, not a definition
- let's indent the entries in the interface map as wel always do
- eh think the Jsonrpc without comrpc do not make much sense, or am I missing something
- out of process should have a mode setting in the conf.in file
-
propose to do (OutProcess& parent) (no assert needed and clear it must be a valid pointer) and make member then also a &
explicit ConnectionNotification(OutProcess* parent) : _parent(*parent) {
- out of process -> the Deactivated notification handlinging in the parent is missing
- OOP without interface is hardly valid use case. If we want to support it the interface in root should be IPlugin I guess)
- OOP implementatiion is missing the move deletes
- plugin.josn status": "production", -> make this development (ask Sebastian for possible options)
- OOP missing init for the interface members in ctor and queryinterfacing them and asserts and assigning etc for multiple interfaces e.g.g always refered to as _implementation in cpp file)
- OOP implementation no Exchange:: namespce before interfaces
- OOP configure needs more attention. I propose to indeed ask support and when OOP ask for in or out of process configuration (and use IConfiguration for the latter)
- OOP Deinitialize: if (_service != nullptr) { -> sorry somebody added that almiost in every plugin, that should not have hapenend :) It should be an assert I think: ASSERT(_service == service);
- Think it should be okay now but if you change for the above comments taken into account Deinitaize will be called ahen Initialize is failed so should to correct cleanup for every failure in Initialize
- Also for OOP does not make sense to have jsonrpc without comrpc interface, also now realize how do you connect them together. Sorry only realize this now we should not ask for the jsonrpc inteface we can deduct from the comrpc. Hmm only problem could be that they are in one header file (like IDisplayInfo) but we can't cater for all situations, they have to change that manualy
if we parse the interfaces we can just check the @JSON tag - move the J interfaces to the cpp file?
Plugin skeleton generator 1.0: