Adding Qt multimedia component

[atracsys-sbt]

The Qt multimedia wrapper was already implemented but not included in the CMakeLists, neither was its dependency on MultimediaWidgets, so I added those.

[pieper]

This might be an okay workaround for your build, but it make more sense to configure the generator to include the needed code rather than editing the generated_cpp_50 files.

As an aside, we don't build and run the generator during the CTK build process but realistically we should have been doing that all along rather than relying on the pre-generated ones (the generator is pretty quick to build and run).

[atracsys-sbt]

Ok I understand. However, I don't get how the generator creates PythonQt_QtBindinds.cpp. Could you point me to the right direction ?

[pieper]

I've never edited that part of the code myself, but I would think it's very similar to what you've done in the cmakelists to make sure that the multimedia parts are included in the generated code. I guess you would start by building and running the generator and looking to see what configuration options it has.

[atracsys-sbt]

There is something odd: the CMakeLists of the generator doesn't seem to have ever supported Qt5, and yet there are pre-generated wrappers for Qt5 and commits related to Qt5 support... So how were all these "generated_cpp_5x" generated ?

[pieper]

Your new PR looks much better 👍

how were all these "generated_cpp_5x" generated ?

Not sure - probably we should ask @florianlink at the upstream repo

[lassoan]

Yes, please submit the pull request to upstream. We always get changes in upstream repository first and then update our fork.

Before submitting, though, try to get rid of the MultimediaWidgets injections at those few places. I think there is already a mechanism for bundling (module)Widgets with (module). See for example how QuickWidgets are declared as a dependency of Quick:

set(qt5_wrapped_lib_depends_quick QuickWidgets)

So, instead of adding MultimediaWidgets, try to do the same as for QuickWidgets. If it does not work then you can submit the PR to upstream as is, and ask advice from Florian.

[lassoan]

Your new PR looks much better 👍

how were all these "generated_cpp_5x" generated ?

Not sure - probably we should ask @florianlink at the upstream repo

I don't think these generated_cpp_* folders in the build tree are used on Windows and Linux. The 5_11 wrapper is generated in my build tree at c:\D\S4D\CTK-build\PythonQt-build\generated_cpp_511 and /home/perklab/D/Slicer-SuperBuild-Debug/CTK-build/PythonQt-build. When I experimented a bit with the wrapper then this folder was updated with new content, so it was not just a copy from the source tree.

@pieper do you have the freshly generated wrapper in your build tree, on macOS too?

If everything works for our use case without touching the generated_cpp* folders then we may not need to update them in the pull request (and maybe ask Florian in the PR to confirm that this is the right thing to do).

[pieper]

I don't think these generated_cpp_* folders in the build tree are used on Windows and Linux.

That's great - it definitely makes sense to build and run the generator rather, but I had thought we weren't doing that, at least when PythonQt was first integrated.

@pieper do you have the freshly generated wrapper in your build tree, on macOS too?

I don't have access to a mac build at the moment but I would be pretty sure it's the same.

[atracsys-sbt]

Yes, please submit the pull request to upstream.

I'd like to, but their pythonqt doesn't have a cmakelists like in CTK so my modifications are irrelevant then. Rather, there is a batch script to generate a solution from Qt project files. Once I generated and compiled the solution, I didn't get a generated wrapper, but the executable for the generator is there (generator/release/pythonqt_generator.exe). However, when I launch it there is an error:

PS D:\dev-ext\src\pythonqt-mevislab_fork\generator\release> .\pythonqt_generator.exe
Please wait while source files are being generated...
Parsing typesystem file [:/trolltech/generator/build_all.txt]
Fatal error: line=1, column=1, message=unexpected end of file
Fatal error: line=2, column=62, message=Failed to parse: 'typesystem_core.xml'
Cannot parse file: ':/trolltech/generator/build_all.txt'

Moreover, their wrappers don't seem to include a PythonQt_QtBindings.cpp like in CTK.

In any case, I find it really confusing to have pre-generated wrappers if those are not copied in the build. This increases the risk of discrepancy between the pre-generated wrappers and the one built during compilation.

[atracsys-sbt]

Before submitting, though, try to get rid of the MultimediaWidgets injections at those few places. I think there is already a mechanism for bundling (module)Widgets with (module).

Indeed, it's better and it works (see above commit)

[atracsys-sbt]

I had tried generating the wrapper from the upstream repo, but that triggered an error with Qt5.15 as mentioned above.
Since then, I submitted an issue on that repo to signal the problem.
Moreover, I installed Qt5.12 and tried again, with success, but there was no PythonQt_QtBindings.h/cpp included the wrapper like in CTK. Indeed, these files were simply manually added in this commit for generated_cpp_50 to 54, then in this commit for 56 and in this one for 511.
As a result, I don't think there is anything to change upstream, the two modifications required concern the CMakeLists.txt and all the PythonQt_QtBindings.h/cpp, which are part of the CTK/PythonQt repo only.

[atracsys-sbt]

@lassoan Considering the findings detailed in my last post, do you think my PR could be approved and merged ? (along side the complementary one in CTK)

[lassoan]

This looks good to me. Please squash the commits so that we can merge.

[lassoan]

This looks good to me.

[lassoan]

@jcfr could you please review and merge this? I don't have write access and I'm not sure how you want to manage patches. This patch seems to affect the CMake build system, which does not exist upstream, so I guess we can just commit it directly here (don't need to push it upstream and backport).

[atracsys-sbt]

@lassoan @jcfr Kind reminder for merging this

[lassoan]

As I wrote above, it looks good to me, but only @jcfr can merge.

[atracsys-sbt]

As I wrote above, it looks good to me, but only @jcfr can merge.

I understand, I thought maybe someone else could do it if @jcfr is too busy.

[jcfr]

@atracsys-sbt Thanks for the contribution 🙏 and everyone else for the reviews 💯

[jcfr]

@atracsys-sbt Could you submit another pull-request documenting your contribution in the main README ? See https://github.com/commontk/PythonQt#patched-9

[lassoan]

Thank you @jcfr for the review and merge.

@atracsys-sbt thanks again for your contribution and your patience in waiting out all the reviews.

[atracsys-sbt]

@atracsys-sbt Could you submit another pull-request documenting your contribution in the main README ? See https://github.com/commontk/PythonQt#patched-9

@jcfr Done !