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

Add service load_config_discard_changes #1710

Merged

Conversation

FSund
Copy link
Contributor

@FSund FSund commented Feb 6, 2022

Description

Adds a service load_config_discard_changes that avoids the "There are unsaved changes" dialogue that appears when calling the regular load_config service.

I am very open to alternative names for the service, and alternative implementations.

Implements suggestions in #1709

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.

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.

Generally looks good. Given that this targets an already released distro, I agree with your choice of using another service name. Otherwise, the existing message should have been augmented by an extra flag.
Please address the two minor comments below.

src/rviz/visualization_frame.cpp Outdated Show resolved Hide resolved
src/rviz/visualizer_app.cpp Outdated Show resolved Hide resolved
@FSund
Copy link
Contributor Author

FSund commented Feb 6, 2022

Thanks for the speedy reply and review. Your comments have been addressed in the latest commits.

Do I need to do anything about the failing CI jobs?

@FSund FSund requested a review from rhaschke February 6, 2022 16:10
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.

Some more nitpicks.

src/rviz/visualization_frame.h Outdated Show resolved Hide resolved
src/rviz/visualizer_app.cpp Outdated Show resolved Hide resolved
FSund and others added 3 commits February 6, 2022 18:38
Co-authored-by: Robert Haschke <rhaschke@users.noreply.github.com>
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.

Thanks a lot. I'm happy if CI is...

@rhaschke rhaschke merged commit 34d4aea into ros-visualization:noetic-devel Feb 6, 2022
@FSund FSund deleted the load-config-discard-changes branch February 6, 2022 20:55
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.

Option to call load_config service without triggering "There are unsaved changes" dialogue?
2 participants