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 #924

Merged
merged 3 commits into from
Jul 6, 2023

Conversation

ygerlach
Copy link
Contributor

@ygerlach ygerlach commented Nov 8, 2022

This removes the need for the setApp() call.

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 kept for now. I have encased the setApplicationIcon() call with an if to protect against unwanted segfaults (There is no need to crash, just because the application icon could not be set).

Why should the VisualizationFrame set the application-icon in the first place?
This seems a bit of. What if i have an app with 2 frames? Maybe an app with many other things and its own application-icon dont want the icon overwritten? But i guess that is a discussion, that should be held elsewhere.

Removing this requirement of having a QApplication* help to ease integration rviz into other (possibly quite big) Qt-Applications.

@ygerlach
Copy link
Contributor Author

ygerlach commented Mar 14, 2023

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

@ygerlach
Copy link
Contributor Author

ygerlach commented Jul 5, 2023

@clalancette ?

Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. This looks good to me, I'll run CI on it next.

@clalancette
Copy link
Contributor

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

Signed-off-by: Chris Lalancette <clalancette@gmail.com>
@clalancette
Copy link
Contributor

New CI after uncrustify fix:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@clalancette clalancette merged commit 439c683 into ros2:rolling Jul 6, 2023
2 checks passed
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.

None yet

3 participants