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

additionally support Qt5 #359

Merged
merged 15 commits into from Apr 27, 2016

Conversation

Projects
None yet
3 participants
@dirk-thomas
Copy link
Member

commented Mar 30, 2016

With these patches the same code base can satisfy both of the following use cases (with only minimal try / except logic:

The version dependency on python_qt_binding ensures that the packages which use QtWidgets are getting the changes from ros-visualization/python_qt_binding#31.

This is required for a release into Kinetic.

@dirk-thomas dirk-thomas force-pushed the qt5 branch 2 times, most recently from 2344a1a to 8341193 Mar 30, 2016

@dirk-thomas

This comment has been minimized.

Copy link
Member Author

commented Apr 1, 2016

As a follow up of this PR the package rqt_plot will be moved into a separate repository since it requires a Qt version specific dependency on Qwt. Since I don't want to branch this repo for Kinetic rqt_plot is extracted since it requires branching.

@trainman419

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2016

rqt_bag depends on rqt_plot, so that may need to move as well.

@dirk-thomas

This comment has been minimized.

Copy link
Member Author

commented Apr 1, 2016

rqt_bag doesn't need to move. Only rqt_plot needs a different dependency in its manifest. rqt_bag can continue to use it even if it is in a different repo.

@trainman419

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2016

The dependency chain now is rqt_bag_plugins -> rqt_plot -> rqt_py_common. Moving the middle item in that dependency chain out of the repository seems like it will cause confusion for people building from source, and I'm not convinced that tools like bloom or devel jobs are designed for that situation.

@dirk-thomas

This comment has been minimized.

Copy link
Member Author

commented Apr 1, 2016

That is true. A circular dependency on the repository level is not fun and should be avoided. I might create a "relay" package then which just chooses the Qwt version. Then that package can be branches and rqt_plot and friends can stay in this repo (without a need to branch).

@dirk-thomas

This comment has been minimized.

Copy link
Member Author

commented Apr 11, 2016

I created a "dependency only" package called qwt_dependency which lives outside of this repo and is now being used as a dependency: 3ac086c

That way we can use Qwt 5 for Jade and older and Qwt 6.1 for Kinetic (once it becomes available as a system package or someone creates a custom package which we import into our apt repo).

@dirk-thomas

This comment has been minimized.

Copy link
Member Author

commented Apr 20, 2016

With PySide 2 there are no working backends for plotting at the moment. There are either no packages available upstream or the package is too old (in the case of matplotlib it supports PySide 2 only as of the upcoming release 2.1). We would need to create our own package of one of the backend libraries to support it in Kinetic.

@dirk-thomas dirk-thomas referenced this pull request Apr 21, 2016

Closed

Kinetic Release #364

@dirk-thomas

This comment has been minimized.

Copy link
Member Author

commented Apr 21, 2016

I posted on the SIG and am looking forward for feedback on these changes: https://groups.google.com/forum/#!topic/ros-sig-rqt/1zcA4xlQ3E0

@dirk-thomas dirk-thomas merged commit 1226f14 into master Apr 27, 2016

@ojura

This comment has been minimized.

Copy link
Contributor

commented on c7cfadb May 6, 2016

Shouldn't these imports be also provided for Qt4 by catching import exceptions? I believe this fails on Qt4.
e.g.
from python_qt_binding.QtGui import QWidget -> from python_qt_binding.QtWidgets import QWidget

This comment has been minimized.

Copy link
Member Author

replied May 6, 2016

python_qt_binding provides QtWidgets as an alias to QtGui in Qt4 (ros-visualization/python_qt_binding#31) for easy forward compatibility.

This comment has been minimized.

Copy link
Contributor

replied May 6, 2016

Thanks, that's true. I somehow ended in a situation where Ubuntu's python_qt_binding, which is missing that alias, got loaded instead of the ROS Kinetic one, alongside with Qt4, and then it failed.

lucasw added a commit to lucasw/rqt_pcl_visualizer that referenced this pull request Feb 8, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.