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.

Sign up for GitHub

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

Added convenience constructor to plugin #911

Merged
merged 8 commits into from
Apr 4, 2022
Merged

Added convenience constructor to plugin #911

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
6f94339
Added convenience constructor to plugin
Mar 25, 2022
7dab7b9
Merge branch 'sdf12' into plugin_constructor
nkoenig Mar 25, 2022
8170c6a
Merge branch 'sdf12' into plugin_constructor
nkoenig Mar 28, 2022
5d574a4
Merge branch 'sdf12' into plugin_constructor
nkoenig Mar 29, 2022
4e73bcb
Updated requirement of ign-common
Mar 29, 2022
e6fc820
use sdf::trim
Mar 30, 2022
4306703
Merge branch 'sdf12' into plugin_constructor
nkoenig Mar 30, 2022
b8e2d3b
Merge branch 'sdf12' into plugin_constructor
nkoenig Apr 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions include/sdf/Plugin.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,16 @@ namespace sdf
/// \param[in] _plugin Plugin to copy.
public: Plugin(Plugin &&_plugin) noexcept;

/// \brief A constructor that initializes the plugin's filename, name, and
/// optionally the content.
/// \param[in] _filename Filename of the shared library associated with
/// this plugin.
/// \param[in] _name The name of the plugin.
/// \param[in] _xmlContent Optional XML content that will be stored in
/// this plugin.
public: Plugin(const std::string &_filename, const std::string &_name,
const std::string &_xmlContent = "");

/// \brief Load the plugin based on a element pointer. This is *not* the
/// usual entry point. Typical usage of the SDF DOM is through the Root
/// object.
Expand Down Expand Up @@ -97,6 +107,15 @@ namespace sdf
/// \param[in] _elem Element to insert.
public: void InsertContent(const sdf::ElementPtr _elem);

/// \brief Insert XML content into this plugin. This function does not
/// modify the values in the sdf::ElementPtr returned by the `Element()`
/// function. The provided content must be valid XML.
/// \param[in] _content A string that contains valid XML. The XML is
/// inserted into this plugin if it is valid.
/// \return False if the provided content was invalid, in which case the
/// content of this plugin is not modified. True otherwise
public: bool InsertContent(const std::string _content);

/// \brief Set the filename of the shared library.
/// \param[in] _filename Filename of the shared library associated with
/// this plugin.
Expand Down
3 changes: 3 additions & 0 deletions src/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ if (BUILD_TESTING)

if (TARGET UNIT_FrameSemantics_TEST)
target_sources(UNIT_FrameSemantics_TEST PRIVATE FrameSemantics.cc Utils.cc)
target_link_libraries(UNIT_FrameSemantics_TEST TINYXML2::TINYXML2)
endif()

if (TARGET UNIT_ParamPassing_TEST)
Expand All @@ -89,6 +90,7 @@ if (BUILD_TESTING)

if (TARGET UNIT_Utils_TEST)
target_sources(UNIT_Utils_TEST PRIVATE Utils.cc)
target_link_libraries(UNIT_Utils_TEST TINYXML2::TINYXML2)
endif()

if (TARGET UNIT_XmlUtils_TEST)
Expand Down Expand Up @@ -118,6 +120,7 @@ target_link_libraries(${PROJECT_LIBRARY_TARGET_NAME}
ignition-math${IGN_MATH_VER}::ignition-math${IGN_MATH_VER}
ignition-utils${IGN_UTILS_VER}::ignition-utils${IGN_UTILS_VER}
PRIVATE
ignition-common${IGN_COMMON_VER}::ignition-common${IGN_COMMON_VER}
Copy link
Collaborator

Choose a reason for hiding this comment

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

we added ignition::common for USD I'm not really sure that you can use this dependency here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The dependency doesn't seem bad to me. Here is the code I'd duplicate, which I could trim (haha) down:

/////////////////////////////////////////////////
void ignition::common::ltrim(std::string &_s)
{
  _s.erase(_s.begin(), std::find_if(_s.begin(), _s.end(),
        [](int c) {return !std::isspace(c);}));
}

/////////////////////////////////////////////////
void ignition::common::rtrim(std::string &_s)
{
  _s.erase(std::find_if(_s.rbegin(), _s.rend(),
        [](int c) {return !std::isspace(c);}).base(),
      _s.end());
}

/////////////////////////////////////////////////
void ignition::common::trim(std::string &_s)
{
  ignition::common::ltrim(_s);
  ignition::common::rtrim(_s);
}

/////////////////////////////////////////////////
std::string ignition::common::ltrimmed(std::string _s)
{
  ignition::common::ltrim(_s);
  return _s;
}   

/////////////////////////////////////////////////
std::string ignition::common::rtrimmed(std::string _s)
{
  ignition::common::rtrim(_s);
  return _s;
}

/////////////////////////////////////////////////
std::string ignition::common::trimmed(std::string _s)
{
  ignition::common::trim(_s);
  return _s;
}

Utilizing existing code rather than duplicating it seems like something we should do internally. Otherwise, we should be telling users to copy our code, and not use the libraries.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we should add a dependency in a stable version. We already have sdf::trim that can be used instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, this is a stable version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in e6fc820

Copy link
Contributor

Choose a reason for hiding this comment

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

Adding ign-common as a dependency to the core of SDF is something we should think carefully about. That brings a lot of dependencies. An alternative to consider would be to move some of this functionality to ign-utils.

TINYXML2::TINYXML2
using_parser_urdf)

Expand Down
57 changes: 57 additions & 0 deletions src/Plugin.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
*
*/

#include <ignition/common/Util.hh>
#include "sdf/Plugin.hh"
#include "sdf/parser.hh"
#include "Utils.hh"
Expand Down Expand Up @@ -42,6 +43,18 @@ Plugin::Plugin()
{
}

/////////////////////////////////////////////////
Plugin::Plugin(const std::string &_filename, const std::string &_name,
const std::string &_xmlContent)
: dataPtr(std::make_unique<sdf::PluginPrivate>())
{
this->SetFilename(_filename);
this->SetName(_name);
std::string trimmed = ignition::common::trimmed(_xmlContent);
Copy link
Collaborator

Choose a reason for hiding this comment

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

you are only using ignition::common here, can you remove this dependency here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in e6fc820

if (!trimmed.empty())
this->InsertContent(trimmed);
}

/////////////////////////////////////////////////
Plugin::~Plugin() = default;

Expand Down Expand Up @@ -175,6 +188,50 @@ void Plugin::InsertContent(const sdf::ElementPtr _elem)
this->dataPtr->contents.push_back(_elem->Clone());
}

/////////////////////////////////////////////////
bool Plugin::InsertContent(const std::string _content)
{
// Read the XML content
auto xmlDoc = tinyxml2::XMLDocument(true, tinyxml2::COLLAPSE_WHITESPACE);;
xmlDoc.Parse(_content.c_str());
if (xmlDoc.Error())
{
sdferr << "Error parsing XML from string: " << xmlDoc.ErrorStr() << '\n';
return false;
}

// Insert each XML element
for (tinyxml2::XMLElement *xml = xmlDoc.FirstChildElement(); xml;
xml = xml->NextSiblingElement())
{
sdf::ElementPtr element(new sdf::Element);

// Copy the name
element->SetName(xml->Name());

// Copy attributes
for (const tinyxml2::XMLAttribute *attribute = xml->FirstAttribute();
attribute; attribute = attribute->Next())
{
element->AddAttribute(attribute->Name(), "string", "", 1, "");
element->GetAttribute(attribute->Name())->SetFromString(
attribute->Value());
}

// Copy the value
if (xml->GetText() != nullptr)
element->AddValue("string", xml->GetText(), true);

// Copy all children
copyChildren(element, xml, false);

// Add the element to this plugin
this->InsertContent(element);
}

return true;
}

/////////////////////////////////////////////////
Plugin &Plugin::operator=(const Plugin &_plugin)
{
Expand Down
67 changes: 67 additions & 0 deletions src/Plugin_TEST.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,3 +318,70 @@ TEST(DOMPlugin, InputStreamOperator)
std::string elemString = elem->ToString("");
EXPECT_EQ(input, elemString);
}

/////////////////////////////////////////////////
TEST(DOMPlugin, InsertStringContent)
{
sdf::Plugin plugin("my-filename", "my-name",
"<render_engine>ogre2</render_engine>");
EXPECT_EQ("my-filename", plugin.Filename());
EXPECT_EQ("my-name", plugin.Name());
ASSERT_EQ(1u, plugin.Contents().size());
EXPECT_EQ("render_engine", plugin.Contents()[0]->GetName());
EXPECT_EQ("ogre2", plugin.Contents()[0]->Get<std::string>());

std::string extraContent = R"foo(
<with_attribute value='bar'>1.234</with_attribute>
<sibling>hello</sibling>
<with_children>
<child1>goodbye</child1>
<child2>goodbye</child2>
</with_children>
)foo";

// Insert more content using a string
EXPECT_TRUE(plugin.InsertContent(extraContent));

std::ostringstream completeContent;
completeContent << " <render_engine>ogre2</render_engine>" << extraContent;

std::ostringstream completePlugin;
completePlugin << "<plugin name='my-name' filename='my-filename'>\n"
<< completeContent.str()
<< "</plugin>\n";
EXPECT_EQ(completePlugin.str(), plugin.ToElement()->ToString(""));

// Try out curly braces intitialization
sdf::Plugin plugin2{plugin.Filename(), plugin.Name(), completeContent.str()};
EXPECT_EQ(plugin.ToElement()->ToString(""),
plugin2.ToElement()->ToString(""));

// Try to insert poorly formed XML, which should fail and not modify the
// content.
EXPECT_FALSE(plugin2.InsertContent("<a></b>"));
EXPECT_EQ(plugin.ToElement()->ToString(""),
plugin2.ToElement()->ToString(""));

// An empty string will also fail and not modify the content
EXPECT_FALSE(plugin2.InsertContent(""));
EXPECT_EQ(plugin.ToElement()->ToString(""),
plugin2.ToElement()->ToString(""));

// Contructing a new plugin with no content
sdf::Plugin plugin3{"a filename", "a name"};
EXPECT_EQ("a filename", plugin3.Filename());
EXPECT_EQ("a name", plugin3.Name());
EXPECT_TRUE(plugin3.Contents().empty());

// Contructing a new plugin with bad XML content
sdf::Plugin plugin4{"filename", "name", "<garbage>"};
EXPECT_EQ("filename", plugin4.Filename());
EXPECT_EQ("name", plugin4.Name());
EXPECT_TRUE(plugin4.Contents().empty());

// Contructing a new plugin with bad XML content
sdf::Plugin plugin5{"filename", "name", " "};
EXPECT_EQ("filename", plugin5.Filename());
EXPECT_EQ("name", plugin5.Name());
EXPECT_TRUE(plugin5.Contents().empty());
}
58 changes: 58 additions & 0 deletions src/Utils.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -304,5 +304,63 @@ sdf::Errors loadIncludedInterfaceModels(sdf::ElementPtr _sdf,

return allErrors;
}

/////////////////////////////////////////////////
void copyChildren(ElementPtr _sdf, tinyxml2::XMLElement *_xml,
const bool _onlyUnknown)
{
// Iterate over all the child elements
tinyxml2::XMLElement *elemXml = nullptr;
for (elemXml = _xml->FirstChildElement(); elemXml;
elemXml = elemXml->NextSiblingElement())
{
std::string elemName = elemXml->Name();

if (_sdf->HasElementDescription(elemName))
{
if (!_onlyUnknown)
{
sdf::ElementPtr element = _sdf->AddElement(elemName);

// FIXME: copy attributes
for (const auto *attribute = elemXml->FirstAttribute();
attribute; attribute = attribute->Next())
{
element->GetAttribute(attribute->Name())->SetFromString(
attribute->Value());
}

// copy value
const char *value = elemXml->GetText();
if (value)
{
element->GetValue()->SetFromString(value);
}
copyChildren(element, elemXml, _onlyUnknown);
}
}
else
{
sdf::ElementPtr element(new sdf::Element);
element->SetParent(_sdf);
element->SetName(elemName);
for (const tinyxml2::XMLAttribute *attribute = elemXml->FirstAttribute();
attribute; attribute = attribute->Next())
{
element->AddAttribute(attribute->Name(), "string", "", 1, "");
element->GetAttribute(attribute->Name())->SetFromString(
attribute->Value());
}

if (elemXml->GetText() != nullptr)
{
element->AddValue("string", elemXml->GetText(), true);
}

copyChildren(element, elemXml, _onlyUnknown);
_sdf->InsertElement(element);
}
}
}
}
}
10 changes: 10 additions & 0 deletions src/Utils.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include <optional>
#include <utility>
#include <vector>
#include <tinyxml2.h>
#include "sdf/Error.hh"
#include "sdf/Element.hh"
#include "sdf/InterfaceElements.hh"
Expand Down Expand Up @@ -229,6 +230,15 @@ namespace sdf
return &_opt.value();
return nullptr;
}

/// \brief Copy all children from the provided tinyxml2 object into the
/// provided sdf element pointer.
/// \param[in, out] _sdf A valid sdf element pointer.
/// \param[in] _xml XML to copy.
/// \param[in] _onlyUnknown Set this to true to only copy XML elements that
/// do not have a matching description in the provided sdf element pointer.
void copyChildren(ElementPtr _sdf, tinyxml2::XMLElement *_xml,
const bool _onlyUnknown);
}
}
#endif
59 changes: 0 additions & 59 deletions src/parser.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1933,65 +1933,6 @@ bool readXml(tinyxml2::XMLElement *_xml, ElementPtr _sdf,
return true;
}

/////////////////////////////////////////////////
void copyChildren(ElementPtr _sdf,
tinyxml2::XMLElement *_xml,
const bool _onlyUnknown)
{
// Iterate over all the child elements
tinyxml2::XMLElement *elemXml = nullptr;
for (elemXml = _xml->FirstChildElement(); elemXml;
elemXml = elemXml->NextSiblingElement())
{
std::string elemName = elemXml->Name();

if (_sdf->HasElementDescription(elemName))
{
if (!_onlyUnknown)
{
sdf::ElementPtr element = _sdf->AddElement(elemName);

// FIXME: copy attributes
for (const auto *attribute = elemXml->FirstAttribute();
attribute; attribute = attribute->Next())
{
element->GetAttribute(attribute->Name())->SetFromString(
attribute->Value());
}

// copy value
const char *value = elemXml->GetText();
if (value)
{
element->GetValue()->SetFromString(value);
}
copyChildren(element, elemXml, _onlyUnknown);
}
}
else
{
ElementPtr element(new Element);
element->SetParent(_sdf);
element->SetName(elemName);
for (const tinyxml2::XMLAttribute *attribute = elemXml->FirstAttribute();
attribute; attribute = attribute->Next())
{
element->AddAttribute(attribute->Name(), "string", "", 1, "");
element->GetAttribute(attribute->Name())->SetFromString(
attribute->Value());
}

if (elemXml->GetText() != nullptr)
{
element->AddValue("string", elemXml->GetText(), true);
}

copyChildren(element, elemXml, _onlyUnknown);
_sdf->InsertElement(element);
}
}
}

/////////////////////////////////////////////////
bool convertFile(const std::string &_filename, const std::string &_version,
SDFPtr _sdf)
Expand Down