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

dds config tool repair #13681

Merged

Conversation

Noy-Zini
Copy link

Fix according to comments on PR #13628

Tracked on : [RSDEV-2884]

@Noy-Zini Noy-Zini force-pushed the viewer-dds-config-tool-repair branch from c20bd3c to 2289871 Compare January 14, 2025 12:44
@Noy-Zini Noy-Zini force-pushed the viewer-dds-config-tool-repair branch 2 times, most recently from 4fbc5d8 to 97b51f5 Compare January 14, 2025 13:26
common/rs-config.h Outdated Show resolved Hide resolved
common/dds-model.cpp Outdated Show resolved Hide resolved
common/dds-model.cpp Outdated Show resolved Hide resolved
@Noy-Zini Noy-Zini force-pushed the viewer-dds-config-tool-repair branch from 97b51f5 to 07bbd98 Compare January 14, 2025 13:43
@Noy-Zini Noy-Zini requested a review from OhadMeir January 14, 2025 13:45
common/dds-model.cpp Outdated Show resolved Hide resolved
common/dds-model.cpp Outdated Show resolved Hide resolved
@@ -15,6 +18,12 @@ using namespace rs2;
using rsutils::json;
using rsutils::type::ip_address;

uint32_t const GET_ETH_CONFIG = 0xBB;
Copy link
Contributor

Choose a reason for hiding this comment

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

Better that the constants will be declared as static. in this specific case probably does not matter, but it's a "best practice".
static will tell the compiler that the constants are declared for this compilation unit only (this file) and they won't clash with other similarly named symbols in the symbol table.
Another way to do it is to declare them in a nameless namespace.

namespace {
uint32_t const GET_ETH_CONFIG = 0xBB;
....
}

Copy link
Author

Choose a reason for hiding this comment

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

I saw we mainly use namespace, so I chose to use that unfortunately it affects all of the file and shows all of it in the diff.

Copy link
Author

Choose a reason for hiding this comment

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

I saw we mainly use namespace, so I chose to use that unfortunately it affects all of the file and shows all of it in the diff.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can close the namespace after the constants definition, not at the end of file, this way rest of file won't be changed.

What actually happens is that the compiler internally generates a name and a using statement that uses that name.

Copy link
Author

Choose a reason for hiding this comment

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

Made that change thanks!

common/rs-config.h Outdated Show resolved Hide resolved
Copy link
Contributor

@OhadMeir OhadMeir left a comment

Choose a reason for hiding this comment

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

When viewer window is minimized DDS settings might not be shown. Is it possible to add a scroll bar to the settings window?

@Noy-Zini
Copy link
Author

When viewer window is minimized DDS settings might not be shown. Is it possible to add a scroll bar to the settings window?

I added a scroll bar to the settings window 👍

@Noy-Zini Noy-Zini force-pushed the viewer-dds-config-tool-repair branch from 6d8d339 to 5a622f1 Compare January 15, 2025 20:23
@Noy-Zini Noy-Zini force-pushed the viewer-dds-config-tool-repair branch from 5a622f1 to b11b18f Compare January 15, 2025 20:26
@OhadMeir OhadMeir merged commit b1f6710 into IntelRealSense:development Jan 16, 2025
23 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.

2 participants