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: Consolidate PythonWrapper #12088

Merged
merged 7 commits into from Feb 19, 2024
Merged

Gui: Consolidate PythonWrapper #12088

merged 7 commits into from Feb 19, 2024

Conversation

3x380V
Copy link
Contributor

@3x380V 3x380V commented Jan 23, 2024

This is just a draft as I'm unable to verify all configurations.
I'm still unhappy with PythonWrapper, there are way too many ifdeffed paths, making it error prone. An attempt to clean it up a bit is made. Disclaimer: a week ago I had zero knowledge about Shiboken and it does improve very slowly over time.

Issues:

  • build system is unable to find pip installed QtWebEngineWidgets
  • python claims value returned by toSecsSinceEpoch (into repo.updated_timestamp) is a float, therefore typecast.
  • only Python loaded modules tested, not linked against shiboken.

Comments very welcome and appreciated.

@github-actions github-actions bot added Core Issue or PR touches core sections (App, Gui, Base) of FreeCAD Addon Manager labels Jan 23, 2024
@chennes
Copy link
Member

chennes commented Jan 23, 2024

I'm about to tear out QtWebEngineWidgets, so don't worry too much about that one 😄.

@3x380V
Copy link
Contributor Author

3x380V commented Jan 23, 2024

@chennes, that was really fast, so I'm not worried any more. I would appreciate is someone could look at Addon Manager part. I works now, it is a bit ungly.

@chennes
Copy link
Member

chennes commented Jan 24, 2024

These AddonManager changes should be fine, all of those methods and constants existed in Qt 5.12.

@3x380V 3x380V force-pushed the qt6_fixes branch 4 times, most recently from 25582fc to d21e753 Compare January 24, 2024 08:41
@3x380V
Copy link
Contributor Author

3x380V commented Jan 24, 2024

The problem with Addon Manager is that I do not know where that float comes from. It ends this way:

TypeError: 'float' object cannot be interpreted as an integer
/home/ladis/src/FreeCAD-build/Mod/AddonManager/package_details.py:137: RuntimeWarning: libshiboken: Overflow: Value 1695849953.710445 exceeds limits of type  [signed] "x" (8bytes).
  QtCore.QDateTime.fromSecsSinceEpoch(repo.updated_timestamp)
Traceback (most recent call last):
  File "/home/ladis/src/FreeCAD-build/Mod/AddonManager/AddonManager.py", line 748, in table_row_activated
    self.packageDetails.show_repo(selected_repo)
  File "/home/ladis/src/FreeCAD-build/Mod/AddonManager/package_details.py", line 124, in show_repo
    self.display_repo_status(self.repo.update_status)
  File "/home/ladis/src/FreeCAD-build/Mod/AddonManager/package_details.py", line 137, in display_repo_status
    QtCore.QDateTime.fromSecsSinceEpoch(repo.updated_timestamp)
OverflowError
TypeError: 'float' object cannot be interpreted as an integer
Traceback (most recent call last):
  File "/home/ladis/src/FreeCAD-build/Mod/AddonManager/package_details.py", line 137, in display_repo_status
    QtCore.QDateTime.fromSecsSinceEpoch(repo.updated_timestamp)
OverflowError

So that typecasing to int is needed, alhough documentation says it should be u64. And I do not like pushing things I do not understand.
I will create separate PR for Addon Manager once you merge #12097

@wwmayer, could you elaborate why is QPrinter special? (see XXX in the code)

@3x380V
Copy link
Contributor Author

3x380V commented Jan 24, 2024

Ah, repo.updated_timestamp is updated also with os.path.getmtime() which returns a floating-point value of class ‘float’ that represents the time (in seconds) of last modification of the specified path.

@3x380V 3x380V changed the title More Qt6 fixes Gui: Consolidate PythonWrapper Jan 24, 2024
@3x380V
Copy link
Contributor Author

3x380V commented Jan 24, 2024

Addon Manager has its PR #12100 now. However I do not know how to remove label.

@chennes
Copy link
Member

chennes commented Jan 27, 2024

This is looking good now -- is it ready for review?

@3x380V
Copy link
Contributor Author

3x380V commented Jan 27, 2024

Yes, however only shiboken/PySide access via C++ is working while via Python it still does not. This is the same as before. Of course, I would appreciate help with debugging. Just uncomment block mentioned in code, to see the problem

// Uncomment this block to remove PySide C++ support and switch to its Python interface

@3x380V
Copy link
Contributor Author

3x380V commented Jan 27, 2024

So here it is:
PyObject* CommandPy::getAction(PyObject *args) is calling wrap.fromQObject(a) (there are some places where result of wrap.loadWidgetsModule() is not tested at all, btw) and because const char* className is set to nullptr, later in Py::Object PythonWrapper::fromQObject(QObject* object, const char* className) construction of std::string from nullptr fails.

I bet someone more familiar with codebase would find it way sooner (or perhaps I should try setting up a decent debugger)

@3x380V 3x380V marked this pull request as ready for review January 28, 2024 08:41
@3x380V 3x380V force-pushed the qt6_fixes branch 2 times, most recently from 508db86 to 51e40a1 Compare January 28, 2024 10:43
src/Gui/PythonWrapper.cpp Outdated Show resolved Hide resolved
SetupShibokenAndPyside macro checks module include path and eventually
decides to disable respective module in case its include directory is
missing. Make this process more straightforward by testing directory
existence; "Location: " string is 10 not 9 characters long and leading
whitespace makes testing for directory name fail.
While there, rename variables to respect that find_pip_package returns
only single include and library path.
@chennes chennes merged commit 925be2a into FreeCAD:main Feb 19, 2024
9 checks passed
@3x380V 3x380V deleted the qt6_fixes branch February 19, 2024 16:48
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