Rolling namespace in title #1074

maxbader · 2023-10-09T18:59:12Z

Hi

I like to request a merge, with the proposed changes RVIZ shows the currently used namespace in the title. This comes very handy when using multiple robots with namespaces.
See #1073

ahcorde

linters https://build.ros2.org/job/Rpr__rviz__ubuntu_jammy_amd64/301/

maxbader · 2023-10-11T15:36:43Z

Sorry, I fixed the linters error

clalancette

Can you please remove all of the unrelated changes? It makes it hard to review what is actually changed here.

maxbader · 2023-10-12T07:57:21Z

Sorry, I used the ament_clang_format --reformat and ament_uncrustify --reformat on the files I changed.

clalancette · 2023-10-12T12:37:41Z

Sorry, I used the ament_clang_format --reformat and ament_uncrustify --reformat on the files I changed.

Hm, it is interesting that those made all the changes they did, since we already run them (without --reformat) on every pull request. But it's a mystery for another time.

maxbader · 2023-10-13T07:27:18Z

I also put some efforts to get it working for humble. It would be nice to merge that pull #1073 request as well.
(it was some effort because humble rviz is not using C++17 and I had to use QString ....)

clalancette · 2023-10-13T11:54:12Z

rviz_common/src/rviz_common/visualization_frame.cpp

+    /// Adding a namespace prefix on the window frame title
+    std::string node_namespace;
+    node_namespace = rviz_ros_node_.lock()->get_raw_node()->get_namespace();
+    if (node_namespace.compare("/") != 0) {
+      title = node_namespace + " - " + title;
+    }


I don't think we should add this by default. I think that the title is already too long. Let's remove this for now, since the rest of the PR already adds in the ability for the user to set it to what they want.

rviz_common/src/rviz_common/visualization_frame.cpp

…e_in_title

clalancette

This is getting closer! A few more changes that I think will clean this up.

rviz_common/include/rviz_common/visualization_frame.hpp

rviz_common/src/rviz_common/visualization_frame.cpp

…e_in_title

… {CONFIG_PATH} and {CONFIG_FILENAME}

rviz_common/include/rviz_common/visualization_frame.hpp

rviz_common/src/rviz_common/visualization_frame.cpp

…e_in_title

Co-authored-by: Chris Lalancette <clalancette@gmail.com> Signed-off-by: Markus Bader <markus.bader@tirol.com>

…rviz into rolling_namespace_in_title

clalancette

One more minor thing to fix, then we can run CI on this.

rviz_common/include/rviz_common/visualization_frame.hpp

maxbader

Missing " added

clalancette · 2023-11-05T21:10:45Z

Missing " added

Hm, I don't see it for some reason? Did you maybe forget to push?

maxbader · 2023-11-06T13:43:54Z

Missing " added

Hm, I don't see it for some reason? Did you maybe forget to push?

Yes, sorry

clalancette

Thanks for all of the iteration here! I'll run full CI on this next to see where it is.

clalancette · 2023-11-06T13:45:27Z

CI:

Linux
Linux-aarch64
Windows

clalancette · 2023-11-06T16:04:24Z

So it looks like Windows is unhappy with this change:

01:48:52.854 C:\ci\ws\src\ros2\rviz\rviz_common\src\rviz_common\visualization_frame.cpp(773,81): error C2664: 'void rviz_common::VisualizationFrame::setDisplayConfigFile::<lambda_1cd49f53e1d41e008ea030c2db9d75cb>::operator ()(std::string &,const std::string &,const std::string &) const': cannot convert argument 3 from 'std::filesystem::path' to 'const std::string &' [C:\ci\ws\build\rviz_common\rviz_common.vcxproj]

01:48:52.854 C:\ci\ws\src\ros2\rviz\rviz_common\src\rviz_common\visualization_frame.cpp(773,81): message : Reason: cannot convert from 'std::filesystem::path' to 'const std::string' [C:\ci\ws\build\rviz_common\rviz_common.vcxproj]

…e_in_title

maxbader · 2023-11-07T08:15:06Z

Sorry, I am not working with Windows. But I think I have a solution. It seems that the convention of path to string works with under Linux but not under Windows. I just added now c_str() to the path arguments, this is the suggested way.

clalancette · 2023-11-07T13:10:42Z

Sorry, I am not working with Windows. But I think I have a solution. It seems that the convention of path to string works with under Linux but not under Windows. I just added now c_str() to the path arguments, this is the suggested way.

Sounds good, thanks. I'll run CI on it again.

Windows

rviz_common/src/rviz_common/visualization_frame.cpp

maxbader

Hi, I hope that was the issue, I do not have any Windows machine around for testing.

Co-authored-by: Chris Lalancette <clalancette@gmail.com> Signed-off-by: Markus Bader <markus.bader@tirol.com>

clalancette · 2023-11-08T15:37:35Z

Here's another try on Windows:

Windows

clalancette

I think if you apply the latest changes, it will now work. At least, I verified locally that it compiles on Windows for me. I also noticed that "relative_path" isn't what we want; we actually want "parent_path". With these things fixed, we can give CI another go.

rviz_common/src/rviz_common/visualization_frame.cpp

Co-authored-by: Chris Lalancette <clalancette@gmail.com> Signed-off-by: Markus Bader <markus.bader@tirol.com>

clalancette · 2023-11-09T13:01:22Z

Here's another go at CI:

Linux
Linux-aarch64
Windows

Stale review.

ahcorde · 2023-11-24T14:28:24Z

https://github.com/Mergifyio backport humble iron

mergify · 2023-11-24T14:28:30Z

backport humble iron

✅ Backports have been created

#1099 Rolling namespace in title (backport #1074) has been created for branch humble but encountered conflicts
#1100 Rolling namespace in title (backport #1074) has been created for branch iron but encountered conflicts

* Update visualization_frame.cpp * window title format option added Signed-off-by: Markus Bader <markus.bader@tirol.com> Co-authored-by: Chris Lalancette <clalancette@gmail.com> (cherry picked from commit ea2dbb3) # Conflicts: # rviz_common/src/rviz_common/visualizer_app.cpp

* Rolling namespace in title (#1074) Signed-off-by: Markus Bader <markus.bader@tirol.com> Co-authored-by: Chris Lalancette <clalancette@gmail.com> (cherry picked from commit ea2dbb3) Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>

maxbader and others added 3 commits October 7, 2023 13:56

Update visualization_frame.cpp

d061559

minor

3f7dd68

window title format option added

8bfaaa7

maxbader requested a review from ahcorde as a code owner October 9, 2023 18:59

ahcorde previously requested changes Oct 10, 2023

View reviewed changes

clalancette requested changes Oct 11, 2023

View reviewed changes

maxbader force-pushed the rolling_namespace_in_title branch from 13e878d to 8bfaaa7 Compare October 12, 2023 06:19

linter fixed

c093a21

clalancette requested changes Oct 13, 2023

View reviewed changes

maxbader and others added 2 commits October 13, 2023 15:14

defaut title restored and {} on title variables set

a07eb9d

Merge remote-tracking branch 'upstream/rolling' into rolling_namespac…

4ab8350

…e_in_title

clalancette requested changes Oct 17, 2023

View reviewed changes

maxbader and others added 5 commits October 19, 2023 12:17

Merge remote-tracking branch 'upstream/rolling' into rolling_namespac…

a16ec51

…e_in_title

lambda added to replace title token and token renamed to {NAMESPACE},…

1c3a55b

… {CONFIG_PATH} and {CONFIG_FILENAME}

uncrustified

61e5b08

whitespaces removed

b9fba99

space removed

186798a

clalancette requested changes Oct 25, 2023

View reviewed changes

maxbader and others added 4 commits November 5, 2023 09:36

Merge remote-tracking branch 'upstream/rolling' into rolling_namespac…

a55cc1c

…e_in_title

Apply suggestions from code review

cb698f0

Co-authored-by: Chris Lalancette <clalancette@gmail.com> Signed-off-by: Markus Bader <markus.bader@tirol.com>

Merge branch 'rolling_namespace_in_title' of github.com:tuw-robotics/…

b46bfe9

…rviz into rolling_namespace_in_title

include order fixed

5367570

clalancette requested changes Nov 5, 2023

View reviewed changes

rviz_common/include/rviz_common/visualization_frame.hpp Outdated Show resolved Hide resolved

maxbader commented Nov 5, 2023

View reviewed changes

Error in doxygen header fixed by adding " after RVIZ

b100137

clalancette approved these changes Nov 6, 2023

View reviewed changes

maxbader added 2 commits November 7, 2023 09:09

Merge remote-tracking branch 'upstream/rolling' into rolling_namespac…

f2a8db7

…e_in_title

fix for windows filesystem::path to string by adding c_str()

5e3ba18

clalancette requested changes Nov 7, 2023

View reviewed changes

rviz_common/src/rviz_common/visualization_frame.cpp Outdated Show resolved Hide resolved

maxbader commented Nov 8, 2023

View reviewed changes

Apply suggestions from code review

3db7cf8

Co-authored-by: Chris Lalancette <clalancette@gmail.com> Signed-off-by: Markus Bader <markus.bader@tirol.com>

clalancette requested changes Nov 8, 2023

View reviewed changes

rviz_common/src/rviz_common/visualization_frame.cpp Outdated Show resolved Hide resolved

rviz_common/src/rviz_common/visualization_frame.cpp Outdated Show resolved Hide resolved

maxbader and others added 2 commits November 9, 2023 11:36

Update rviz_common/src/rviz_common/visualization_frame.cpp

ca528be

Co-authored-by: Chris Lalancette <clalancette@gmail.com> Signed-off-by: Markus Bader <markus.bader@tirol.com>

Update rviz_common/src/rviz_common/visualization_frame.cpp

5d0b1e7

Co-authored-by: Chris Lalancette <clalancette@gmail.com> Signed-off-by: Markus Bader <markus.bader@tirol.com>

clalancette approved these changes Nov 9, 2023

View reviewed changes

clalancette merged commit ea2dbb3 into ros2:rolling Nov 9, 2023
2 checks passed

maxbader deleted the rolling_namespace_in_title branch November 10, 2023 12:23

mergify bot mentioned this pull request Nov 24, 2023

Rolling namespace in title (backport #1074) #1099

Merged

mergify bot mentioned this pull request Nov 24, 2023

Rolling namespace in title (backport #1074) #1100

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rolling namespace in title #1074

Rolling namespace in title #1074

maxbader commented Oct 9, 2023 •

edited

ahcorde left a comment

maxbader commented Oct 11, 2023

clalancette left a comment

maxbader commented Oct 12, 2023

clalancette commented Oct 12, 2023

maxbader commented Oct 13, 2023

clalancette Oct 13, 2023

clalancette left a comment

clalancette left a comment

maxbader left a comment

clalancette commented Nov 5, 2023

maxbader commented Nov 6, 2023

clalancette left a comment

clalancette commented Nov 6, 2023

clalancette commented Nov 6, 2023

maxbader commented Nov 7, 2023

clalancette commented Nov 7, 2023

maxbader left a comment •

edited

clalancette commented Nov 8, 2023

clalancette left a comment

clalancette commented Nov 9, 2023

ahcorde commented Nov 24, 2023

mergify bot commented Nov 24, 2023 •

edited

Rolling namespace in title #1074

Rolling namespace in title #1074

Conversation

maxbader commented Oct 9, 2023 • edited

ahcorde left a comment

Choose a reason for hiding this comment

maxbader commented Oct 11, 2023

clalancette left a comment

Choose a reason for hiding this comment

maxbader commented Oct 12, 2023

clalancette commented Oct 12, 2023

maxbader commented Oct 13, 2023

clalancette Oct 13, 2023

Choose a reason for hiding this comment

clalancette left a comment

Choose a reason for hiding this comment

clalancette left a comment

Choose a reason for hiding this comment

maxbader left a comment

Choose a reason for hiding this comment

clalancette commented Nov 5, 2023

maxbader commented Nov 6, 2023

clalancette left a comment

Choose a reason for hiding this comment

clalancette commented Nov 6, 2023

clalancette commented Nov 6, 2023

maxbader commented Nov 7, 2023

clalancette commented Nov 7, 2023

maxbader left a comment • edited

Choose a reason for hiding this comment

clalancette commented Nov 8, 2023

clalancette left a comment

Choose a reason for hiding this comment

clalancette commented Nov 9, 2023

ahcorde commented Nov 24, 2023

mergify bot commented Nov 24, 2023 • edited

✅ Backports have been created

maxbader commented Oct 9, 2023 •

edited

maxbader left a comment •

edited

mergify bot commented Nov 24, 2023 •

edited