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

Fixups with Qt6 enums #13611

Merged
merged 3 commits into from
May 6, 2024
Merged

Conversation

YakoYakoYokuYoku
Copy link
Contributor

@YakoYakoYokuYoku YakoYakoYokuYoku commented Apr 23, 2024

Because PySide6 treats enums and QFlags as IntEnums and IntFlags respectively I've added converters for Python PySide enums to C++, and due to this I've modified the requirements of task panel's getStandardButtons to return standard buttons without casting them to int. Fixes #13605 and part of #13303. Related to #6992.

@github-actions github-actions bot added Core Issue or PR touches core sections (App, Gui, Base) of FreeCAD WB FEM Related to the FEM Workbench WB BIM Related to the BIM/Arch Workbench WB Draft Related to the Draft Workbench WB CAM Related to the CAM/Path Workbench WB Part Design Related to the Part Design Workbench WB Part Related to the Part Workbench WB OpenSCAD Related to the OpenSCAD Workbench labels Apr 23, 2024
@luzpaz luzpaz added the 3rd party: Qt 6 Issue related to Qt 6 label Apr 24, 2024
@adrianinsaval
Copy link
Member

and due to this I've modified the requirements of task panel's getStandardButtons to return standard buttons without casting them to int.

can we avoid this? because this sounds like any addon using this will have to be updated to work which is not good

@YakoYakoYokuYoku
Copy link
Contributor Author

We can't due to both IntEnum and IntFlag from enum neither inherit int nor implement __int__, that's an issue with the standard Python library itself. But is easy enough to update, simply remove the int conversion, like int(QtWidgets.QDialogButtonBox.Cancel) with QtWidgets.QDialogButtonBox.Cancel. This also retains compatibility with PySide2 because the conversion is done at Gui::PythonWrapper::toEnum.

@chennes chennes self-requested a review April 29, 2024 16:02
@adrianinsaval
Copy link
Member

alternative solution was proposed in a different PR: #13337 (comment)

try:
    # pyside2
    return int(QtWidgets.QDialogButtonBox.Close)
except TypeError:
    # pyside6
    return QtWidgets.QDialogButtonBox.Close.value

while this still means that addons will need to update for qt6 compatibility I think it might be better as they will have more time for migration as they'll retain compatibility with the qt5 releases of FreeCAD

@YakoYakoYokuYoku
Copy link
Contributor Author

Let me explain for a bit. Those int conversions aren't really necessary, here's why. The PySide2.QtWidgets.QDialogButtonBox.StandardButton flag supports bitwise operations without casting to an int (you can see examples in WB code prior to my PR).

# This
    return int(QtGui.QDialogButtonBox.Ok) | int(QtGui.QDialogButtonBox.Cancel)
# Would be the same as
    return int(QtGui.QDialogButtonBox.Ok | QtGui.QDialogButtonBox.Cancel)

Before my PR, the return value of getStandardButtons regardless of its type is converted to an int through the Py::Int constructor as you can see at line #737 from src/Gui/TaskView/TaskDialogPython.cpp. I took extra care to make sure that this, the C++-side Py::Int cast, remains being done by PythonWrapper when Shiboken is not linked with FreeCAD.

QDialogButtonBox::StandardButtons TaskDialogPython::getStandardButtons() const
{
Base::PyGILStateLocker lock;
try {
if (dlg.hasAttr(std::string("getStandardButtons"))) {
Py::Callable method(dlg.getAttr(std::string("getStandardButtons")));
Py::Tuple args;
Py::Int ret(method.apply(args));
int value = (int)ret;
return {value};
}
}
catch (Py::Exception&) {
Base::PyException e; // extract the Python error text
e.ReportException();
}
return TaskDialog::getStandardButtons();
}

Therefore the following works too.

# Before
    return int(QtGui.QDialogButtonBox.Ok) | int(QtGui.QDialogButtonBox.Cancel)
# After
    return QtGui.QDialogButtonBox.Ok | QtGui.QDialogButtonBox.Cancel

As you can see int conversions aren't necessary in Python code thus they can be removed even if you're using PySide2. So, you can make code compatible with both versions of PySide by discarding those conversions. I hope I was clear enough.

@adrianinsaval
Copy link
Member

my question is rather if old workbenches will remain compatible with freecad after this change

@YakoYakoYokuYoku
Copy link
Contributor Author

Ahh, gotcha. In that case this change retains compatibility between old workbenches and FreeCAD using PySide2, but it won't be the case if PySide6 is used as those may also have other incompatibilities too (like Maker Workbench for example). Though, getting rid of int casts from getStandardButtons in workbenches will be OK for both PySide2 and PySide6.

@adrianinsaval
Copy link
Member

adrianinsaval commented May 1, 2024

In that case this change retains compatibility between old workbenches and FreeCAD using PySide2, but it won't be the case if PySide6 is used

you mean they'll still work after this PR or with the proposal I posted above? PySide6 incompatibility for addons is ok and expected, nothing we can do there they'll have to port.

@YakoYakoYokuYoku
Copy link
Contributor Author

They'll work with both my PR and the proposal. In case a regression happens I'll take responsability on it.

@chennes
Copy link
Member

chennes commented May 1, 2024

Just noting that we provide a basic compatibility layer between PySide2 and PySide6 so that some addons do not have to change anything. It may be worth investigating if this can be expanded. See the code generation here:

file(MAKE_DIRECTORY ${CMAKE_BINARY_DIR}/Ext/PySide)

@YakoYakoYokuYoku
Copy link
Contributor Author

I've already sent PRs to three workbenches which had been merged (Zolko-123/FreeCAD_Assembly4#489, shaise/FreeCAD_SheetMetal#333, shaise/FreeCAD_FastenersWB#366), so far I haven't seen any regressions provoked by them. Those were very simple changes so adding them to your workbench isn't that hard and remains compatible with PySide2.

@yorikvanhavre yorikvanhavre merged commit 9759da8 into FreeCAD:main May 6, 2024
8 of 9 checks passed
@adrianinsaval
Copy link
Member

argggh I had tested this on an addon that had already been ported. I tested now on cfdOF and the task dialog was missing the close button. I made a PR that fixes this but not all addons will get these updates in a timely manner

@adrianinsaval
Copy link
Member

since the python changes seem to be compatible with both qt5 and qt6 without any more effort, what's the point of the c++ changes here?

@YakoYakoYokuYoku
Copy link
Contributor Author

We can't cast enums to int (using Py::Int before the PR) in Qt6, so you've to use the proper functions, either Shiboken::Enum::getValue or reading the enum value field, that's why the C++ changes.

@adrianinsaval
Copy link
Member

Could we make these changes only apply when compiling with pyside6?

@3x380V
Copy link
Contributor

3x380V commented May 7, 2024

@YakoYakoYokuYoku, damage follows in #13851

@wwmayer
Copy link
Contributor

wwmayer commented May 8, 2024

This fixes the problem: #13902

@adrianinsaval adrianinsaval added the Backport todo PR that should be backported to stable branch label May 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3rd party: Qt 6 Issue related to Qt 6 Backport todo PR that should be backported to stable branch Core Issue or PR touches core sections (App, Gui, Base) of FreeCAD WB BIM Related to the BIM/Arch Workbench WB CAM Related to the CAM/Path Workbench WB Draft Related to the Draft Workbench WB FEM Related to the FEM Workbench WB OpenSCAD Related to the OpenSCAD Workbench WB Part Design Related to the Part Design Workbench WB Part Related to the Part Workbench
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Problem] Unkown command(s), affected Draft functions
7 participants