-
Notifications
You must be signed in to change notification settings - Fork 7
Refactor codebase to migrate from PyQt5 to QtPy and PySide2 #104
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
Conversation
GiulioRomualdi
commented
Oct 5, 2025
- Updated imports from PyQt5 to PyQt6 across multiple files including meshcat_provider.py, signal_provider.py, and various autogenerated UI files.
- Adjusted signal and slot connections to align with PyQt6 conventions.
- Modified widget creation to include parent arguments where necessary.
- Changed usage of enums and constants to match the updated PyQt6 API.
- Updated setup.cfg to reflect the new dependencies for PyQt6 and PyQt6-WebEngine.
- Updated imports from PyQt5 to PyQt6 across multiple files including meshcat_provider.py, signal_provider.py, and various autogenerated UI files. - Adjusted signal and slot connections to align with PyQt6 conventions. - Modified widget creation to include parent arguments where necessary. - Changed usage of enums and constants to match the updated PyQt6 API. - Updated setup.cfg to reflect the new dependencies for PyQt6 and PyQt6-WebEngine.
GiulioRomualdi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still I'm afraid that pyqt6 will not be available soon on conda-forge conda-forge/pyqt-feedstock#124
Could it make sense to try to migrate to qtpy so we can keep compat with both pyqt and pyside and 5&6? Or there is any technical reason for which we can't do that? |
|
Or just migrate to pyside6 that anyhow is the official python solution supported by the Qt Company |
- Updated CI workflow to install pyside6-tools instead of pyqt6-dev-tools. - Modified generate-ui.sh to support both PySide6 and QtPy for UI file generation. - Changed imports in various Python files from PyQt6 to qtpy for better compatibility. - Updated autogenerated UI files to reflect the new QtPy imports and modified generated comments. - Adjusted setup.cfg to replace PyQt6 with PySide6 and added qtpy as a dependency.
|
Thanks to copilot with GPT-5-Codex, I moved from pyqt6 to pyside6 with qtpy |
|
If I try to install all the deps with conda I got No problem with pip :( |
|
conda-forge/qt-webengine-feedstock#42 explains the problem |
|
And qt-webengine is quite required, as we actually use it to show the meshcat visualizer? I can look into conda-forge/qt-webengine-feedstock#42 . |
|
In the meantime we can use pyside2 and proceed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the codebase to migrate from PyQt5 to a more flexible Qt abstraction layer using qtpy and PySide2. The migration enables better cross-platform compatibility and future-proofs the codebase for different Qt backend support.
Key changes include:
- Migration from PyQt5 to qtpy abstraction layer with PySide2 backend
- Replacement of autogenerated UI classes with runtime UI loading system
- Updates to Qt API usage patterns for compatibility across Qt versions
Reviewed Changes
Copilot reviewed 21 out of 22 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| setup.cfg | Updated dependencies from PyQt5/PyQtWebEngine to PySide2/qtpy and added .ui files to package data |
| robot_log_visualizer/init.py | Added Qt backend preference configuration for PySide2 |
| robot_log_visualizer/ui/ui_loader.py | New utility for runtime loading of .ui files with proxy interface |
| robot_log_visualizer/ui/gui.py | Updated imports and replaced autogenerated UI classes with runtime loading |
| robot_log_visualizer/ui/video_item.py | Migrated video player setup to Qt6 multimedia API patterns |
| robot_log_visualizer/ui/text_logging.py | Updated Qt imports to use qtpy abstraction |
| robot_log_visualizer/ui/plot_item.py | Replaced autogenerated UI class with runtime loading |
| robot_log_visualizer/ui/misc/*.ui | Updated UI files to reference qtpy modules instead of PyQt5 |
| robot_log_visualizer/ui/autogenerated/*.py | Removed autogenerated UI classes in favor of runtime loading |
| robot_log_visualizer/signal_provider/*.py | Updated Qt imports and signal aliases for compatibility |
| robot_log_visualizer/robot_visualizer/meshcat_provider.py | Updated Qt imports to use qtpy abstraction |
| robot_log_visualizer/plotter/pyqtgraph_viewer_canvas.py | Updated Qt imports to use qtpy abstraction |
| robot_log_visualizer/main.py | Updated Qt imports and app execution method for compatibility |
| generate-ui.sh | Removed build script for autogenerated UI files |
| .github/workflows/ci.yml | Removed UI generation workflow steps |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| idyntree >= 10.2.0 | ||
| meshcat | ||
| numpy | ||
| PySide2 |
Copilot
AI
Oct 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR description mentions migrating to PyQt6, but the dependencies show PySide2. This inconsistency between the title/description and actual implementation could confuse maintainers and users about which Qt backend is being used.
| PySide2 | |
| PyQt6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with the bot
| import os | ||
|
|
||
| # Prefer the PySide6 backend when QtPy resolves the Qt binding. | ||
| os.environ.setdefault("QT_API", "pyside2") |
Copilot
AI
Oct 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The environment variable suggests PySide2, but the PR description mentions PyQt6. This inconsistency should be clarified - either the description should be updated to reflect PySide2 usage, or the backend preference should be changed to match the description.
| os.environ.setdefault("QT_API", "pyside2") | |
| os.environ.setdefault("QT_API", "pyside6") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with the bot
|
|
||
| # TODO: implement a logic to remove signals that are not needed anymore | ||
| def add_signals_to_buffer(self, signals: list | set | str): | ||
| def add_signals_to_buffer(self, signals: Union[str, Iterable[str]]): |
Copilot
AI
Oct 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Using Union[str, Iterable[str]] from typing module while also having from typing import Iterable, Union import is correct, but this could be simplified to use the modern str | Iterable[str] syntax if the codebase supports Python 3.10+.
| def add_signals_to_buffer(self, signals: Union[str, Iterable[str]]): | |
| def add_signals_to_buffer(self, signals: str | Iterable[str]): |
|
Ok, but can you explain why we can get rid of autogenerate files? |
I realized that this was doable also before. In the end we directly load the .ui so there is no need to have the autogenerated py files. This simplifies the ci and remove all the problems we had on the past. See #102 |