Skip to content
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

Gui: PythonWrapper::fromQAction #12695

Closed
wants to merge 5 commits into from
Closed

Conversation

wwmayer
Copy link
Contributor

@wwmayer wwmayer commented Mar 2, 2024

  • Gui: Implement PythonWrapper::fromQAction
  • Gui: PythonWrapper: Raise exception on qt_wrapInstance failure
  • Gui: PythonWrapper: Use getPyTypeObjectForTypeName consistently
  • Gui: PythonWrapper: Fix qt_wrapInstance
  • Gui: PythonWrapper: Consolidate typeName handling

Wrapping QAction through QObject does not work as QAction class
is a part of QtGui not QtCore, so Py::Object with internal
pointer being null is returned causing a crash later.
Therefore implement fromQAction conversion method.
When qt_wrapInstance fails it returns Py::Object with internal
pointer set to null. Make PythonWrapper::from* methods raise
exception when this happens to be consistent with PySide code path.
@github-actions github-actions bot added the Core Issue or PR touches core sections (App, Gui, Base) of FreeCAD label Mar 2, 2024
@wwmayer
Copy link
Contributor Author

wwmayer commented Mar 2, 2024

Replaces PR #12577

@3x380V
Copy link
Contributor

3x380V commented Mar 2, 2024

Replaces PR #12577

Added relevant changes back into original PR #12577 addressing my comments there.

wwmayer and others added 2 commits March 2, 2024 18:30
* Fix possible crash in qt_wrapInstance
* Make qt_getCppType mor robust in case shiboken's Python API changes
* Fix PythonWrapper::fromQAction for Qt5
  Unlike in Qt6 the class QAction is part of the module QtWidgets in Qt5
Use typeName consistently for both PySide and Python interface code path
@@ -642,7 +642,11 @@ Py::Object PythonWrapper::fromQDir(const QDir& dir)
return Py::asObject(pyobj);
}
#else
Q_UNUSED(dir)
// Access shiboken/PySide via Python
Py::Object obj = qt_wrapInstance<const QIcon*>(icon, "QDir", "QtCore");
Copy link
Contributor

@3x380V 3x380V Mar 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo? Anyway, original PR updated.

Copy link
Contributor Author

@wwmayer wwmayer Mar 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops! Copy & paste error.

@@ -470,6 +470,9 @@ qttype* qt_getCppType(PyObject* pyobj)
// https://github.com/PySide/Shiboken/blob/master/shibokenmodule/typesystem_shiboken.xml
Py::Module mainmod(importShiboken(), true);
Py::Callable func = mainmod.getDict().getItem("getCppPointer");
if (func.isNull()) {
throw Py::RuntimeError("Failed to get C++ pointer");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put a note to myself here: I was temped to change this into return nullptr; to be consistent with Shiboken C++ version, but this is indeed runtime error. Perhaps these two functions (getCppPointer and wrapInstance) could be cached, so they are not called over and over again, but for the time being it is not worth doing. It is on the slow code path and chances are I break something other.

@wwmayer, I put this change as a separate commit in #12577 (one commit per change works better for me) so these two are almost identical. Decision yours, of course.

@wwmayer
Copy link
Contributor Author

wwmayer commented Mar 3, 2024

Closing this one as the original PR got some updates.

@wwmayer wwmayer closed this Mar 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core Issue or PR touches core sections (App, Gui, Base) of FreeCAD
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants