Gui: PythonWrapper::fromQAction

[3x380V]

No description provided.

[3x380V]

This is an attempt to implement #12542 (comment)
Just a draft for now, all that from* methods can be perhaps consolidated a bit. It would be helpfull to know why is fromQPrinter so special that it deserves extra Shiboken::Conversions::getPythonTypeObject("QPrinter"); call.

[wwmayer]

It would be helpfull to know why is fromQPrinter so special

It's special for the same reason as fromQAction because QPrinter is part of the module QtPrintSupport. However, there is a bug in fromQPrinter where it does return qt_wrapInstance<QPrinter*>(printer, "QPrinter", "QtCore").
It should be return qt_wrapInstance<QPrinter*>(printer, "QPrinter", "QtPrintSupport") instead.

Currently, the application also crashes inside qt_wrapInstance for the exact same reason as explained in #12542

Furthermore, QPrinter does not inherit from QObject so that fromQObject isn't an option. And if FreeCAD is built by linking the C++ API of PySide we must have a special converter function, too.

[3x380V]

Thanks for explanation, I'll fix that along the way. However, main concern was following code:

        // XXX: Why is QPrinter special?
#if defined (HAVE_SHIBOKEN2)
        type = reinterpret_cast<SbkObjectType*>(Shiboken::Conversions::getPythonTypeObject("QPrinter"));
#else
        type = Shiboken::Conversions::getPythonTypeObject("QPrinter");
#endif

No other wrapper has this special handling, so other could be probably coalesced using template.

Let's implement only 1. from #12542 (comment) and leave 2. for separate PR.

[xtemp09]

std::string typeName = action->metaObject()->className();
PyObject* pyobj = Shiboken::Object::newObject(type, action, false, false, typeName.c_str());

className() returns const char *.¹ PyObject* newObject requires const char *typeName. ²

Why is there double conversion via std::string?

Footnotes

1. https://doc.qt.io/qt-6/qmetaobject.html#className ↩

2. https://github.com/pyside/Shiboken/blob/09e1ffb01597b6fe4ed1ba1ae362449e3780d7ed/libshiboken/basewrapper.h#L247 ↩

[wwmayer]

className() returns const char . PyObject newObject requires const char *typeName

It's probably even better to directly write "QAction" because if there is a sub-class in FreeCAD the method will fail to create a wrapper.

[3x380V]

Indeed, I'll clean that.

[3x380V]

Contrary to the previous statement, #12542 (comment) is implemented here as well, with other minor changes. These might be easily dropped if they are causing troubles or are just plain wrong.

[chennes]

@wwmayer this seems fine to me now, but I leave it to your final review.

[wwmayer]

The first three commits look OK but as explained in the review the last commit is problematic. And what I still miss is the fix of qt_wrapInstance.

I am going to create a new PR based on this one.

[3x380V]

Do you mean qt_wrapInstance fix described at 2. of #12542 (comment) ? The one that it should return Py::None()? Previously we agreed that we throw an exception, so behavior is the same for both code paths and we are doing so now.

[wwmayer]

Do you mean qt_wrapInstance fix described at 2. of #12542 (comment)

Yes.

The one that it should return Py::None()?

As it was it still caused a segmentation fault because in Qt5 the class QAction is not part of QtGui but QtWidgets. But as we agreed to throw an exception I changed it to return immediately and let the calling instance handle the exception.

See 28d9392

[3x380V]

Ah! I didn't test Qt5 without PySide, sorry. Now it makes sense, thank you. However, I'd rather see another something like constexpr *char QActionModule along SbkPySide2_QtCoreTypes and friends than another ifdef in the code. It is ifdef hell enough already.

[3x380V]

...as shown in this update. Btw, qt_getCppType would deserve similar fix after "getCppPointer".

[wwmayer]

..as shown in this update

What has been updated? To me it looks like you simply rebased the branch and added my additional commit but left your problematic last commit.

Btw, qt_getCppType would deserve similar fix after "getCppPointer"

Yes, a similar check can be added there, too.

[3x380V]

What has been updated? To me it looks like you simply rebased the branch and added my additional commit but left your problematic last commit.

I'm sorry, what exactly is the problem with 663c9ba ? At least I do not remember it was mentioned previously.

In case you are talking about first commit, 45e9205 I squashed and modified your #ifdef part from your additional commit inside it.

[wwmayer]

I'm sorry, what exactly is the problem with 663c9ba ? At least I do not remember it was mentioned previously.

Because it's a new problem added with this commit. I wonder whether you couldn't see the review comment I have added.

Anyway, I explain it again.
The implementation of fromQIcon() may look like this:

Py::Object PythonWrapper::fromQIcon(const QIcon* icon)
{
...
const char* typeName = typeid(*const_cast<QIcon*>(icon)).name();
Py::Object obj = qt_wrapInstance<const QIcon*>(icon, typeName, "QtGui");
...
}

The problem is that the output of typeid().name() is not standardized and thus not portable. For different compilers the output string can be different. Some compilers will return "QIcon" but some other compilers will return "class QIcon".

But for the qt_wrapInstance() method it is essential to pass the exact class name because it's used to search for the attribute of the given module. E.g. the QtGui module has an attribute QIcon but it doesn't have an attribute class QIcon.

Remark:
For the C++ interface of the shiboken API it doesn't matter to use typeid() because it generates code to register the same converter function with several strings like "QIcon", typeid(QIcon).name(), "QIcon&", "QIcon*", ...

[3x380V]

Because it's a new problem added with this commit. I wonder whether you couldn't see the review comment I have added.

No, I haven't see any review comment even after being now notified about it. Knowing about it, I would of course fix that.
Please let me fix all that issues here and review them.

[wwmayer]

Now I get some build failures. If for Qt5 HAVE_PYSIDE2 is not defined then it says: use of undeclared identifier 'qtModWithQAction'. The same happens for Qt6 if HAVE_PYSIDE6 is not defined.
This is because qtModWithQAction is declared at the wrong position. It should be moved below constexpr const char* ModuleShiboken = "shiboken2"; or constexpr const char* ModuleShiboken = "shiboken6";, respectively.

Then I get a further failure with Qt6 which says that the class declaration of QAction cannot be found. Adding #include <QAction> fixes it.

[3x380V]

Now I get some build failures. If for Qt5 HAVE_PYSIDE2 is not defined then it says: use of undeclared identifier 'qtModWithQAction'. The same happens for Qt6 if HAVE_PYSIDE6 is not defined. This is because qtModWithQAction is declared at the wrong position. It should be moved below constexpr const char* ModuleShiboken = "shiboken2"; or constexpr const char* ModuleShiboken = "shiboken6";, respectively.

That would introduce another build failure as code which needs it is #if not defined (HAVE_SHIBOKEN) || not defined(HAVE_PYSIDE). Just to clarify: all combinations are valid and expected to work? (seems so)

1. !HAVE_SHIBOKEN, !HAVE_PYSIDE
2. HAVE_SHIBOKEN, !HAVE_PYSIDE
3. !HAVE_SHIBOKEN, HAVE_PYSIDE
4. HAVE_SHIBOKEN, HAVE_PYSIDE

I'll just return back your original solution assuming it was tested with all combinations, so I do not block this PR from merging.

[wwmayer]

I'll just return back your original solution assuming it was tested with all combinations, so I do not block this PR from merging

OK.

In a subsequent PR the ifdef hell could be simplified a bit in two steps.
In the first step the ModuleShiboken, ModulePySide and qtModWithQAction can be declared completely independent of the HAVE_SHIBOKEN or HAVE_PYSIDE but only dependent on the Qt version.

For some combinations of HAVE_SHIBOKEN and HAVE_PYSIDE this will generate warnings that an identifier is declared but not used. This can be fixed in the second step by using Q_UNUSED() in the corresponding functions.

Edit:
It's even easier It can be simply done as

#if QT_VERSION < QT_VERSION_CHECK(6,0,0)
[[maybe_unused]]constexpr const char* ModuleShiboken            = "shiboken2";
[[maybe_unused]]constexpr const char* ModulePySide              = "PySide2";
[[maybe_unused]]constexpr const char* qtModWithQAction          = "QtWidgets";
#else
[[maybe_unused]]constexpr const char* ModuleShiboken            = "shiboken6";
[[maybe_unused]]constexpr const char* ModulePySide              = "PySide6";
[[maybe_unused]]constexpr const char* qtModWithQAction          = "QtGui";
#endif

[xtemp09]

When writing adaptation for different extensions, people write something like that:

void func (void)
{
	#ifdef SSE
		func_SSE();
	#endif

	#ifdef SSE2
		func_SSE2();
	#endif

	#ifdef AVX
		func_AVX();
	#endif

	#ifdef AVX2
		func_AVX2();
	#endif
}

Perhaps, it would be better to recompartmentalize in this fashion.

---

I would also add that these implementations can be put into different files, like file_PySide.h, file_Shiboken.h, file_PySide_Shiboken.h. They can be safely added into cmake files, since they have #ifdef guard, therefore, if there is no PySide, it's just an empty file.

[wwmayer]

I would also add that these implementations can be put into different files, like file_PySide.h, file_Shiboken.h, file_PySide_Shiboken.h

shiboken or PySide stuff shouldn't be visible in any header file to avoid to make other source files depending on it. That's why all this stuff is in a single source file.

Theoretically, we could have different source files for shiboken2/PySide2 and shiboken6/PySide6 but then we will end up in a lot of code duplication. The real differences between these two versions are less than 20 lines of code.