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

use static QCoreApplication::processEvents() function without a QApplication instance. #1772

Merged

Conversation

ygerlach
Copy link
Contributor

This removes the need for the setApp() call.

Description

As seen in the Qt documentation processEvents is a static function so no member of QCoreApplication is required. Removing this takes away the need for setApp() in VisualizationFrame and VisualizerApp which i have keept for now, but should be marked as deprecated and get removed in the future.
Removing this requirement of QApplication help to ease the step of integrating rviz into other possibly quite big Qt-Applications, if there is no need to pass around a pointer to the main Application.

Checklist

  • If you are addressing rendering issues, please provide:
    • Images of both, broken and fixed renderings.
    • Source code to reproduce the issue, e.g. a YAML or rosbag file with a MarkerArray msg.
  • If you are changing GUI, please include screenshots showing how things looked before and after.
  • Choose the proper target branch: latest release branch, for non-ABI-breaking changes, future release branch otherwise.
    Due to the lack of active maintainers, we cannot provide support for older release branches anymore.
  • Did you change how RViz works? Added new functionality? Do not forget to update the tutorials and/or documentation on the ROS wiki
  • While waiting for someone to review your request, please consider reviewing another open pull request to support the maintainers of RViz. Refer to the RViz Wiki for reviewing guidelines.

…ication instance. This removes the need for the setApp() call.
Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

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

Looks good. A small nitpick though:

src/rviz/visualization_frame.h Outdated Show resolved Hide resolved
src/rviz/visualizer_app.h Outdated Show resolved Hide resolved
Co-authored-by: Robert Haschke <rhaschke@users.noreply.github.com>
@ygerlach
Copy link
Contributor Author

ygerlach commented Oct 28, 2022

thanks, i wasn't sure how deprecated methods are annotated in this project.

@ygerlach
Copy link
Contributor Author

ygerlach commented Mar 14, 2023

What needs to happen, that this PR can be merged?

@rhaschke
Copy link
Contributor

Sorry, this got out of my sight. Closing and reopening to trigger CI again...

@rhaschke rhaschke closed this Mar 20, 2023
@rhaschke rhaschke reopened this Mar 20, 2023
@rhaschke rhaschke merged commit 15e148d into ros-visualization:noetic-devel Mar 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants