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

Refactors the MessageWidget class #402

Merged
merged 5 commits into from
May 18, 2021

Conversation

agalbachicar
Copy link
Collaborator

@agalbachicar agalbachicar commented May 12, 2021

  • Renames the MessageWidget class to Message
  • Replaces Message::Variant by a using directive that points to std::variant.
  • Simplifies the code by using templates to avoid code duplication.

With this PR, the refactor to TopicInterfacePlugin finishes and #332 can be closed.

@agalbachicar agalbachicar force-pushed the agalbachicar/#332_rename_message_widget branch from 25961c6 to a322e77 Compare May 14, 2021 10:47
os << value.enumVal.value().name;
}
std::ostream& operator<<(std::ostream& os, const internal::Message::Variant& value) {
std::visit([&os](auto&& arg) { os << arg; }, value);
Copy link
Contributor

Choose a reason for hiding this comment

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

nice

scpeters
scpeters previously approved these changes May 15, 2021
Copy link
Collaborator

@francocipollone francocipollone left a comment

Choose a reason for hiding this comment

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

LGTM!


/// @brief Holds the information of a google::protobuf::Message to be consumed
/// by a Qt widget.
/// @details Holds a variant like-struct when it is a leaf node in the message
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Techincally, it isn't holding a "variant like-struct" anymore.

// Serializes a @p value into @p os. Provides a valid operator overload for
// internal::Message::EnumValue so the following function's lambda can be
// resolved.
std::ostream& operator<<(std::ostream& os, const internal::Message::EnumValue& value) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: _os instead of os. _value instead of value.

os << value.enumVal.value().name;
}
std::ostream& operator<<(std::ostream& os, const internal::Message::Variant& value) {
std::visit([&os](auto&& arg) { os << arg; }, value);
Copy link
Collaborator

Choose a reason for hiding this comment

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

😎

Copy link
Collaborator

@francocipollone francocipollone left a comment

Choose a reason for hiding this comment

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

LGTM

@agalbachicar agalbachicar merged commit 3fb8472 into master May 18, 2021
@agalbachicar agalbachicar deleted the agalbachicar/#332_rename_message_widget branch May 18, 2021 17:23
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.

3 participants