Skip to content

Commit

Permalink
Fix attribute update when <dialog> isn't present (#455)
Browse files Browse the repository at this point in the history 
Signed-off-by: Louise Poubel <louise@openrobotics.org>
  • Loading branch information
@chapulina
chapulina authored Aug 8, 2022
1 parent f59ae67 commit 6758eb0
Show file tree
Hide file tree
Showing 3 changed files with 132 additions and 29 deletions.
26 changes: 19 additions & 7 deletions include/ignition/gui/Dialog.hh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,23 +58,35 @@ namespace ignition

/// \brief Store dialog default config
/// \param[in] _config XML config as string
/// \deprecated Introduce deprecation warnings on v7.
public: void SetDefaultConfig(const std::string &_config);

/// \brief Write dialog config
/// \param[in] _path config path
/// \brief Update an attribute on an XML file. The attribute belongs to
/// a `<dialog>` element that has a `name` attrbute matching this dialog's
/// name, i.e.
///
/// `<dialog name="dialog_name" attribute="value"/>`
///
/// If a dialog element with this dialog's name doesn't exist yet, one
/// will be created.
///
/// \param[in] _path File path. File must already exist, this function
/// will not create a new file.
/// \param[in] _attribute XMLElement attribute name
/// \param[in] _value XMLElement attribute value
/// \return true if written to config file
/// \return True if written to config file
public: bool UpdateConfigAttribute(
const std::string &_path, const std::string &_attribute,
const bool _value) const;

/// \brief Gets a config attribute value.
/// It will return an empty string if the config file or the attribute
/// \brief Gets an attribute value from an XML file. The attribute belongs
/// to a `<dialog>` element that has a `name` attribute matching this
/// dialog's name.
/// It will return an empty string if the file or the attribute
/// don't exist.
/// \param[in] _path config path
/// \param[in] _path File path
/// \param[in] _attribute attribute name
/// \return attribute value as string
/// \return Attribute value as string
public: std::string ReadConfigAttribute(const std::string &_path,
const std::string &_attribute) const;

Expand Down
42 changes: 25 additions & 17 deletions src/Dialog.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,6 @@ namespace ignition
{
class DialogPrivate
{
/// \brief default dialog config
public: std::string config{""};

/// \brief Pointer to quick window
public: QQuickWindow *quickWindow{nullptr};
};
Expand All @@ -51,9 +48,11 @@ Dialog::Dialog()
App()->Engine()->rootObjects().value(0));
if (!this->dataPtr->quickWindow)
{
// We'd only get here if the QML file is malformed
// LCOV_EXCL_START
ignerr << "Internal error: Failed to instantiate QML file [" << qmlFile
<< "]" << std::endl;
return;
// LCOV_EXCL_STOP
}
}

Expand All @@ -74,7 +73,10 @@ QQuickItem *Dialog::RootItem() const
auto dialogItem = this->dataPtr->quickWindow->findChild<QQuickItem *>();
if (!dialogItem)
{
// We'd only get here if the QML file is malformed
// LCOV_EXCL_START
ignerr << "Internal error: Null dialog root item!" << std::endl;
// LCOV_EXCL_STOP
}

return dialogItem;
Expand All @@ -84,7 +86,7 @@ QQuickItem *Dialog::RootItem() const
bool Dialog::UpdateConfigAttribute(const std::string &_path,
const std::string &_attribute, const bool _value) const
{
if (_path.empty())
if (!common::exists(_path))
{
ignerr << "Missing config file" << std::endl;
return false;
Expand All @@ -101,37 +103,43 @@ bool Dialog::UpdateConfigAttribute(const std::string &_path,
}

// Update attribute value for the correct dialog
bool updated{false};
for (auto dialogElem = doc.FirstChildElement("dialog");
dialogElem != nullptr;
dialogElem = dialogElem->NextSiblingElement("dialog"))
{
if(dialogElem->Attribute("name") == this->objectName().toStdString())
if (dialogElem->Attribute("name") == this->objectName().toStdString())
{
dialogElem->SetAttribute(_attribute.c_str(), _value);
updated = true;
}
}

// Write config file
tinyxml2::XMLPrinter printer;
doc.Print(&printer);
// Create new <dialog> if missing
if (!updated)
{
auto dialogElem = doc.NewElement("dialog");
dialogElem->SetAttribute("name", this->objectName().toStdString().c_str());
dialogElem->SetAttribute(_attribute.c_str(), _value);
doc.InsertEndChild(dialogElem);
}

std::string config = printer.CStr();
std::ofstream out(_path.c_str(), std::ios::out);
if (!out)
// Write config file
if (!doc.SaveFile(_path.c_str()))
{
ignerr << "Unable to open file: " << _path
// LCOV_EXCL_START
ignerr << "Failed to save file: " << _path
<< ".\nCheck file permissions.\n";
// LCOV_EXCL_STOP
}
else
out << config;

return true;
}

/////////////////////////////////////////////////
void Dialog::SetDefaultConfig(const std::string &_config)
void Dialog::SetDefaultConfig(const std::string &)
{
this->dataPtr->config = _config;
ignwarn << "Dialog::SetDefaultConfig has no effect." << std::endl;
}

/////////////////////////////////////////////////
Expand Down
93 changes: 88 additions & 5 deletions src/Dialog_TEST.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,19 @@ char* g_argv[] =
using namespace ignition;
using namespace gui;

/////////////////////////////////////////////////
TEST(DialogTest, IGN_UTILS_TEST_DISABLED_ON_WIN32(Accessors))
{
common::Console::SetVerbosity(4);
Application app(g_argc, g_argv, WindowType::kDialog);

auto dialog = new Dialog;
ASSERT_NE(nullptr, dialog);

EXPECT_NE(nullptr, dialog->RootItem());
EXPECT_NE(nullptr, dialog->QuickWindow());
}

/////////////////////////////////////////////////
TEST(DialogTest, IGN_UTILS_TEST_DISABLED_ON_WIN32(UpdateDialogConfig))
{
Expand All @@ -44,10 +57,13 @@ TEST(DialogTest, IGN_UTILS_TEST_DISABLED_ON_WIN32(UpdateDialogConfig))
ASSERT_NE(nullptr, dialog);
dialog->setObjectName("quick_menu");

// Call deprecated function for test coverage
dialog->SetDefaultConfig("");

// Start without a file
std::remove(kTestConfigFile.c_str());

// Read attribute value when the config doesn't exist
// The file doesn't exist
{
EXPECT_FALSE(common::exists(kTestConfigFile));
std::string allow = dialog->ReadConfigAttribute(kTestConfigFile,
Expand All @@ -56,6 +72,31 @@ TEST(DialogTest, IGN_UTILS_TEST_DISABLED_ON_WIN32(UpdateDialogConfig))

// Config file still doesn't exist
EXPECT_FALSE(common::exists(kTestConfigFile));

EXPECT_FALSE(dialog->UpdateConfigAttribute(kTestConfigFile, "allow", true));

// Config file still doesn't exist
EXPECT_FALSE(common::exists(kTestConfigFile));
}

// Malformed file
{
EXPECT_FALSE(common::exists(kTestConfigFile));

// Create file
std::ofstream configFile(kTestConfigFile);
configFile << "banana";
configFile.close();
EXPECT_TRUE(common::exists(kTestConfigFile));

std::string allow = dialog->ReadConfigAttribute(kTestConfigFile,
"allow");
EXPECT_TRUE(allow.empty());

EXPECT_FALSE(dialog->UpdateConfigAttribute(kTestConfigFile, "allow", true));

// Delete file
std::remove(kTestConfigFile.c_str());
}

// Read a non existing attribute
Expand Down Expand Up @@ -105,7 +146,7 @@ TEST(DialogTest, IGN_UTILS_TEST_DISABLED_ON_WIN32(UpdateDialogConfig))
EXPECT_TRUE(common::exists(kTestConfigFile));

// Update value
dialog->UpdateConfigAttribute(kTestConfigFile, "allow", true);
EXPECT_TRUE(dialog->UpdateConfigAttribute(kTestConfigFile, "allow", true));

// Read value
auto allow = dialog->ReadConfigAttribute(kTestConfigFile, "allow");
Expand All @@ -115,18 +156,18 @@ TEST(DialogTest, IGN_UTILS_TEST_DISABLED_ON_WIN32(UpdateDialogConfig))
std::remove(kTestConfigFile.c_str());
}

// Update a existing attribute
// Update an existing attribute
{
EXPECT_FALSE(common::exists(kTestConfigFile));

// Create file
std::ofstream configFile(kTestConfigFile);
configFile << "<dialog name='quick_menu' show='true'/>";
configFile << "<dialog name='quick_menu' allow='true'/>";
configFile.close();
EXPECT_TRUE(common::exists(kTestConfigFile));

// Update value
dialog->UpdateConfigAttribute(kTestConfigFile, "allow", false);
EXPECT_TRUE(dialog->UpdateConfigAttribute(kTestConfigFile, "allow", false));

// Read value
auto allow = dialog->ReadConfigAttribute(kTestConfigFile, "allow");
Expand All @@ -136,5 +177,47 @@ TEST(DialogTest, IGN_UTILS_TEST_DISABLED_ON_WIN32(UpdateDialogConfig))
std::remove(kTestConfigFile.c_str());
}

// Update a file with a different <dialog>
{
EXPECT_FALSE(common::exists(kTestConfigFile));

// Create file
std::ofstream configFile(kTestConfigFile);
configFile << "<dialog name='banana' allow='false'/>";
configFile.close();
EXPECT_TRUE(common::exists(kTestConfigFile));

// Update value
EXPECT_TRUE(dialog->UpdateConfigAttribute(kTestConfigFile, "allow", true));

// Read value
auto allow = dialog->ReadConfigAttribute(kTestConfigFile, "allow");
EXPECT_EQ(allow, "true");

// Delete file
std::remove(kTestConfigFile.c_str());
}

// Update a file without a <dialog>
{
EXPECT_FALSE(common::exists(kTestConfigFile));

// Create file
std::ofstream configFile(kTestConfigFile);
configFile << "<banana/>";
configFile.close();
EXPECT_TRUE(common::exists(kTestConfigFile));

// Update value
EXPECT_TRUE(dialog->UpdateConfigAttribute(kTestConfigFile, "allow", true));

// Read value
auto allow = dialog->ReadConfigAttribute(kTestConfigFile, "allow");
EXPECT_EQ(allow, "true");

// Delete file
std::remove(kTestConfigFile.c_str());
}

delete dialog;
}

0 comments on commit 6758eb0

Please sign in to comment.