summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorBent Bisballe Nyeng <deva@aasimon.org>2020-04-09 14:45:53 +0200
committerBent Bisballe Nyeng <deva@aasimon.org>2020-04-09 14:45:53 +0200
commit2b35cdf78038a4ecc4573fcbef5fbad0c8403cb1 (patch)
tree61c6a99bf6b037882a3738ee64dad1f65abd7757
parentad8c3a178ddbf561e08d800372979eaf54986a9c (diff)
Review comments.
-rw-r--r--src/configfile.cc26
-rw-r--r--src/configfile.h2
2 files changed, 28 insertions, 0 deletions
diff --git a/src/configfile.cc b/src/configfile.cc
index a1494a8..6747090 100644
--- a/src/configfile.cc
+++ b/src/configfile.cc
@@ -41,28 +41,37 @@
#include <hugin.hpp>
#if DG_PLATFORM == DG_PLATFORM_WINDOWS
+// TODO: lowercase name for non-defines, 'sep'?
static const std::string SEP = "\\";
#else
static const std::string SEP = "/";
#endif
#if DG_PLATFORM == DG_PLATFORM_WINDOWS
+// TODO: lowercase name for non-defines, 'config_dir_name'?
static const std::string CONFIGDIRNAME = "DrumGizmo";
#else
+// TODO: Should we use this on OSX or is there another standard naming?
static const std::string CONFIGDIRNAME = ".drumgizmo";
#endif
namespace
{
+// TODO: Put sep and configdirname into anonymous namespace as well instead of
+// being static?
+// TODO: c++ style comments
/**
* Return the path containing the config files.
*/
std::string getConfigPath()
{
+ // TODO: Move this utility function as a static function into directory.cc/h?
+ // This seems like something we could use in other places as well.
std::string configpath;
#if DG_PLATFORM == DG_PLATFORM_WINDOWS
TCHAR szPath[256];
+ // TODO: Compare return value to S_OK instead of using SUCCEEDED macro.
if(SUCCEEDED(SHGetFolderPath(NULL, CSIDL_APPDATA | CSIDL_FLAG_CREATE, NULL, 0, szPath)))
{
configpath = szPath;
@@ -81,6 +90,7 @@ std::string getConfigPath()
*/
bool createConfigPath()
{
+ // TODO: West-const ;)
std::string const configpath = getConfigPath();
struct stat st;
@@ -105,6 +115,7 @@ bool createConfigPath()
} // end anonymous namespace
+// TODO: West-const ;)
ConfigFile::ConfigFile(std::string const& filename)
: filename(filename)
{
@@ -119,6 +130,9 @@ ConfigFile::~ConfigFile()
}
}
+// TODO: Perhaps return a (homemade) optional carrying an error description on
+// failure? We might want to show the error in the UI and not just in the
+// terminal.
bool ConfigFile::load()
{
DEBUG(configfile, "Loading config file...\n");
@@ -130,6 +144,14 @@ bool ConfigFile::load()
values.clear();
std::string line;
+ // TODO: What happens if the parser encounters a windows newline on linux, or
+ // the other way round?
+ // Perhaps a unit-test for this would be in order?
+ // TODO: What does line != "" do here? If the newline is part of the string,
+ // I think it is redundant with getline returning false (end-of-file)
+ // but if the newline character is stripped from the line, then this
+ // will make the parser stop if an empty line is encountered in the
+ // config file.
while(std::getline(current_file, line) && line != "")
{
if(!parseLine(line))
@@ -147,6 +169,8 @@ bool ConfigFile::save()
{
DEBUG(configfile, "Saving configuration...\n");
+ // TODO: Check for failure? It wouldn't make sense to write the file if the
+ // folder couldn't be created.
createConfigPath();
if(!open(std::ios_base::out))
@@ -154,6 +178,7 @@ bool ConfigFile::save()
return false;
}
+ // TODO: West-const ;)
for(auto const& value: values)
{
current_file << value.first << ":" << value.second << std::endl;
@@ -198,6 +223,7 @@ bool ConfigFile::open(std::ios_base::openmode mode)
bool ConfigFile::parseLine(const std::string& line)
{
+ // TODO: enum class?
enum State
{
before_key,
diff --git a/src/configfile.h b/src/configfile.h
index 1513897..0934f46 100644
--- a/src/configfile.h
+++ b/src/configfile.h
@@ -33,6 +33,7 @@
class ConfigFile
{
public:
+ // TODO: west-const
ConfigFile(std::string const& filename);
virtual ~ConfigFile();
@@ -47,6 +48,7 @@ protected:
std::string filename;
std::fstream current_file;
+ // TODO: Does this have to be virtual?
virtual bool open(std::ios_base::openmode mode);
std::string readLine();
bool parseLine(const std::string& line);