[PATCH 1/2] Configuration support classes
Adrian Perez
* Interface based on GObject properties: all configuration can be accessed
as properties of objects. This makes them easy to bind to Gtk UI elements.
* Support for loading/saving ~.config/frogr/accounts.xml
* There is support for handling multiple accounts.
---
src/Makefile.am | 2 +
src/frogr-config.c | 663 ++++++++++++++++++++++++++++++++++++++++++++++++++++
src/frogr-config.h | 84 +++++++
3 files changed, 749 insertions(+), 0 deletions(-)
create mode 100644 src/frogr-config.c
create mode 100644 src/frogr-config.h
diff --git a/src/Makefile.am b/src/Makefile.am
index a010107..4d2e854 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -42,4 +42,6 @@ frogr_SOURCES = \
frogr-main-window.h \
frogr-picture.c \
frogr-picture.h \
+ frogr-config.c \
+ frogr-config.h \
main.c
diff --git a/src/frogr-config.c b/src/frogr-config.c
new file mode 100644
index 0000000..828f06d
--- /dev/null
+++ b/src/frogr-config.c
@@ -0,0 +1,663 @@
+/*
+ * frogr-config.c -- Configuration system for Frogr.
+ *
+ * Copyright (C) 2009 Adrian Perez
+ * Authors: Adrian Perez <ape...@igalia.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of version 3 of the GNU General Public
+ * License as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public
+ * License along with this program; if not, write to the
+ * Free Software Foundation, Inc., 59 Temple Place - Suite 330,
+ * Boston, MA 02111-1307, USA.
+ */
+
+#include "frogr-config.h"
+#include <libxml/parser.h>
+#include <string.h>
+#include <errno.h>
+
+
+enum FrogrConfigProperties {
+ FROGR_CONFIG_NUM_PROPERTIES,
+};
+
+enum FrogrConfigAccountProperties {
+ FROGR_CONFIG_ACCOUNT_ZERO = 0,
+
+ FROGR_CONFIG_ACCOUNT_FROB,
+ FROGR_CONFIG_ACCOUNT_TOKEN,
+ FROGR_CONFIG_ACCOUNT_USERNAME,
+ FROGR_CONFIG_ACCOUNT_ENABLED,
+ FROGR_CONFIG_ACCOUNT_PRIVATE,
+ FROGR_CONFIG_ACCOUNT_PRIVATE_FAMILY,
+ FROGR_CONFIG_ACCOUNT_PRIVATE_FRIENDS,
+
+ /* Add new ones just above this item */
+ FROGR_CONFIG_ACCOUNT_NUM_PROPERTIES,
+};
+
+
+struct _FrogrConfig {
+ /* Parent */
+ GObject parent_instance;
+
+ /* List of accounts. */
+ GList *accounts;
+};
+
+struct _FrogrConfigClass {
+ GObjectClass parent_class;
+};
+
+
+G_DEFINE_TYPE (FrogrConfig, frogr_config, G_TYPE_OBJECT);
+
+
+
+struct _FrogrConfigAccount {
+ /* Parent */
+ GObject parent_instance;
+
+ /* Instance variables */
+ gchar *frob;
+ gchar *token;
+ gchar *username;
+
+ gboolean enabled;
+ gboolean private;
+ gboolean private_family;
+ gboolean private_friends;
+};
+
+struct _FrogrConfigAccountClass {
+ GObjectClass parent_class;
+};
+
+G_DEFINE_TYPE (FrogrConfigAccount, frogr_config_account, G_TYPE_OBJECT);
+
+
+
+static gboolean
+xml_node_to_boolean (const xmlNodePtr node)
+{
+ xmlChar *xstr;
+ gchar *str;
+ gboolean result;
+
+ g_return_val_if_fail (node != NULL, FALSE);
+
+ xstr = xmlNodeGetContent (node);
+ str = (gchar*) xstr;
+
+ while (g_ascii_isspace (*str++)); /* Skip blanks */
+
+ if (g_ascii_isdigit (*str))
+ {
+ long value = strtol (str, NULL, 0);
+ result = (value != 0);
+ }
+ else
+ {
+ result = (g_ascii_strncasecmp ("false", str, 5) != 0);
+ }
+ xmlFree (xstr);
+ return result;
+}
+
+
+static FrogrConfigAccount*
+frogr_config_account_from_xml (xmlDocPtr xml, xmlNodePtr rootnode)
+{
+ xmlNodePtr node;
+ GObject *account = NULL;
+
+ xmlChar *frob = NULL;
+ xmlChar *token = NULL;
+ xmlChar *username = NULL;
+ gboolean enabled = TRUE;
+ gboolean private = TRUE;
+ gboolean private_family = FALSE;
+ gboolean private_friends = FALSE;
+
+ g_return_val_if_fail (xml != NULL, NULL);
+ g_return_val_if_fail (rootnode != NULL, NULL);
+
+ /* Traverse child nodes and extract relevant information. */
+ for (node = rootnode->children; node != NULL; node = node->next)
+ {
+ if (node->type != XML_ELEMENT_NODE)
+ continue;
+
+ if (xmlStrcmp (node->name, (const xmlChar*) "frob") == 0)
+ frob = xmlNodeGetContent (node);
+ else if (xmlStrcmp (node->name, (const xmlChar*) "token") == 0)
+ token = xmlNodeGetContent (node);
+ else if (xmlStrcmp (node->name, (const xmlChar*) "username") == 0)
+ username = xmlNodeGetContent (node);
+ else if (xmlStrcmp (node->name, (const xmlChar*) "enabled") == 0)
+ enabled = xml_node_to_boolean (node);
+ else if (xmlStrcmp (node->name, (const xmlChar*) "private") == 0)
+ {
+ /* Iterate over children elements as well */
+ xmlNodePtr sub;
+ for (sub = node->children; sub != NULL; sub = sub->next)
+ {
+ if (sub->type == XML_TEXT_NODE)
+ private = xml_node_to_boolean (sub);
+
+ if (sub->type != XML_ELEMENT_NODE)
+ continue;
+
+ if (xmlStrcmp (sub->name, (const xmlChar*) "family") == 0)
+ private_family = xml_node_to_boolean (sub);
+ else if (xmlStrcmp (sub->name, (const xmlChar*) "friends") == 0)
+ private_friends = xml_node_to_boolean (sub);
+ }
+ }
+ }
+
+ /*
+ * The account object is created only of some minimum requirements are
+ * met: user name, token and frob.
+ */
+ if (frob != NULL && token != NULL && username != NULL)
+ {
+ account = g_object_new (FROGR_CONFIG_ACCOUNT_TYPE,
+ "frob", (const gchar*) frob,
+ "token", (const gchar*) token,
+ "username", (const gchar*) username,
+ "enabled", enabled,
+ "private", private,
+ "private-family", private_family,
+ "private-friends", private_friends,
+ NULL);
+ }
+
+ if (frob != NULL) xmlFree (frob);
+ if (token != NULL) xmlFree (token);
+ if (username != NULL) xmlFree (username);
+
+ return FROGR_CONFIG_ACCOUNT (account);
+}
+
+
+static void
+frogr_config_load_accounts (FrogrConfig *config, const gchar *config_dir)
+{
+ gchar *xml_path;
+ xmlNodePtr node;
+ xmlDocPtr xml;
+
+ g_return_if_fail (config != NULL);
+ g_return_if_fail (config_dir != NULL);
+
+ xml_path = g_build_filename (config_dir, "accounts.xml", NULL);
+ xml = xmlParseFile (xml_path);
+ g_free (xml_path);
+
+ if (xml == NULL)
+ {
+ g_warning ("Could not load '%s/accounts.xml'", config_dir);
+ return;
+ }
+
+ if ((node = xmlDocGetRootElement (xml)) == NULL)
+ {
+ g_warning ("File '%s/accounts.xml' is empty", config_dir);
+ xmlFreeDoc (xml);
+ return;
+ }
+
+ if (node->name == NULL ||
+ xmlStrcmp (node->name, (const xmlChar*) "accounts") != 0)
+ {
+ g_warning ("File '%s/accounts.xml' does not start with "
+ "an <accounts> tag", config_dir);
+ xmlFreeDoc (xml);
+ return;
+ }
+
+ /* Iterate over children nodes and extract accounts. */
+ for (node = node->children; node != NULL; node = node->next)
+ {
+ if (xmlStrcmp (node->name, (const xmlChar*) "account") == 0)
+ {
+ FrogrConfigAccount *account;
+ if ((account = frogr_config_account_from_xml (xml, node)) == NULL)
+ {
+ g_warning ("Malformed account in '%s/accounts.xml', "
+ "skipping it", config_dir);
+ }
+ else
+ {
+ config->accounts = g_list_prepend (config->accounts,
+ account);
+ }
+ }
+ }
+
+ xmlFreeDoc (xml);
+ xmlCleanupParser ();
+}
+
+
+static void
+frogr_config_load (FrogrConfig *config, const gchar *config_dir)
+{
+ g_return_if_fail (config != NULL);
+ g_return_if_fail (config_dir != NULL);
+ /*
+ * XXX This method is just a placeholder for when we have program
+ * settings. For now, load just the accounts.
+ */
+
+ frogr_config_load_accounts (config, config_dir);
+}
+
+
+static xmlNodePtr
+xml_add_string_child (xmlNodePtr parent,
+ const gchar *xml_name,
+ GObject *object,
+ const gchar *prop_name)
+{
+ xmlNodePtr node;
+ xmlChar *enc;
+ gchar *value;
+
+ g_return_val_if_fail (parent != NULL, NULL);
+ g_return_val_if_fail (xml_name != NULL, NULL);
+ g_return_val_if_fail (object != NULL, NULL);
+ g_return_val_if_fail (prop_name != NULL, NULL);
+
+ g_object_get (object, prop_name, &value, NULL);
+
+ node = xmlNewNode (NULL, (const xmlChar*) xml_name);
+ enc = xmlEncodeEntitiesReentrant (NULL, (const xmlChar*) value);
+ xmlNodeSetContent (node, enc);
+ g_free (value);
+ xmlFree (enc);
+ xmlAddChild (parent, node);
+
+ return node;
+}
+
+
+static xmlNodePtr
+xml_add_boolean_child (xmlNodePtr parent,
+ const gchar *xml_name,
+ GObject *object,
+ const gchar *prop_name)
+{
+ xmlNodePtr node;
+ gboolean value;
+
+ g_return_val_if_fail (parent != NULL, NULL);
+ g_return_val_if_fail (xml_name != NULL, NULL);
+ g_return_val_if_fail (object != NULL, NULL);
+ g_return_val_if_fail (prop_name != NULL, NULL);
+
+ g_object_get (object, prop_name, &value, NULL);
+
+ node = xmlNewNode (NULL, (const xmlChar*) xml_name);
+ xmlNodeSetContent (node, (const xmlChar*) ((value) ? "true" : "false"));
+ xmlAddChild (parent, node);
+
+ return node;
+}
+
+
+static gboolean
+frogr_config_save_accounts (FrogrConfig *config)
+{
+ gboolean retval = TRUE;
+ xmlDocPtr xml;
+ xmlNodePtr node, root, sub;
+ GList *item;
+ GObject *account;
+ gchar *xml_path;
+
+ g_return_val_if_fail (config != NULL, FALSE);
+
+ xml = xmlNewDoc ((const xmlChar*) "1.0");
+ root = xmlNewNode (NULL, (const xmlChar*) "accounts");
+ xmlDocSetRootElement (xml, root);
+
+ for (item = g_list_first (config->accounts);
+ item != NULL;
+ item = g_list_next (item))
+ {
+ account = G_OBJECT (item->data);
+ node = xmlNewNode (NULL, (const xmlChar*) "account");
+ xml_add_string_child (node, "frob", account, "frob");
+ xml_add_string_child (node, "token", account, "token");
+ xml_add_string_child (node, "username", account, "username");
+ xml_add_boolean_child (node, "enabled", account, "enabled");
+
+ sub = xml_add_boolean_child (node, "private", account, "private");
+ xml_add_boolean_child (sub, "family", account, "private-family");
+ xml_add_boolean_child (sub, "friends", account, "private-friends");
+
+ xmlAddChild (root, node);
+ }
+
+ xml_path = g_build_filename (g_get_user_config_dir (),
+ "frogr", "accounts.xml", NULL);
+
+ if (xmlSaveFormatFileEnc (xml_path, xml, "UTF-8", 1) == -1) {
+ g_critical ("Unable to open '%s' for saving", xml_path);
+ retval = FALSE;
+ }
+
+ return retval;
+}
+
+
+gboolean
+frogr_config_save (void)
+{
+ FrogrConfig *config = frogr_config_get ();
+
+ /*
+ * XXX This method is just a placeholder for when we have program
+ * settings. For now, save just the accounts.
+ */
+
+ return frogr_config_save_accounts (config);
+}
+
+
+
+
+static void
+frogr_config_finalize (GObject *object)
+{
+ FrogrConfig *conf = FROGR_CONFIG(object);
+
+ g_list_foreach (conf->accounts, (GFunc) xmlFreeDoc, NULL);
+ g_list_free (conf->accounts);
+
+ /* Call superclass */
+ G_OBJECT_CLASS (frogr_config_parent_class) -> finalize (object);
+}
+
+
+static void
+frogr_config_class_init (FrogrConfigClass *klass)
+{
+ GObjectClass *obj_class = G_OBJECT_CLASS (klass);
+
+ obj_class->finalize = frogr_config_finalize;
+}
+
+
+static void
+frogr_config_init (FrogrConfig *config)
+{
+ gchar *config_dir = g_build_filename (g_get_user_config_dir (),
+ g_get_prgname (), NULL);
+
+ config->accounts = NULL;
+
+ /* Ensure that we have the config directory in place. */
+ if (g_mkdir_with_parents (config_dir, 0777) != 0)
+ {
+ g_warning ("Could not create config directory '%s' (%s)",
+ config_dir, strerror (errno));
+ }
+
+ frogr_config_load (config, config_dir);
+
+ g_free (config_dir);
+}
+
+
+
+
+FrogrConfig*
+frogr_config_get (void)
+{
+ static GObject *conf = NULL;
+
+ if (conf == NULL) conf = g_object_new (FROGR_CONFIG_TYPE, NULL);
+
+ return FROGR_CONFIG (g_object_ref (conf));
+}
+
+
+
+FrogrConfigAccount*
+frogr_config_get_default_account (void)
+{
+ return frogr_config_get_account (NULL);
+}
+
+
+void
+frogr_config_add_account (FrogrConfigAccount *account)
+{
+ FrogrConfig *config;
+
+ g_return_if_fail (account != NULL);
+
+ config = frogr_config_get ();
+ config->accounts = g_list_prepend (config->accounts,
+ g_object_ref (account));
+}
+
+
+FrogrConfigAccount*
+frogr_config_get_account (const gchar *username)
+{
+ FrogrConfig *config = frogr_config_get ();
+ FrogrConfigAccount *account;
+ GList *item;
+
+ if (config->accounts == NULL)
+ frogr_config_add_account (g_object_new (FROGR_CONFIG_ACCOUNT_TYPE, NULL));
+
+ if (username == NULL)
+ return FROGR_CONFIG_ACCOUNT (g_list_first (config->accounts)->data);
+
+ for (item = g_list_first (config->accounts);
+ item != NULL;
+ item = g_list_next (item))
+ {
+ account = FROGR_CONFIG_ACCOUNT (item->data);
+ if (strcmp (account->username, username) == 0)
+ return account;
+ }
+
+ return NULL;
+}
+
+
+
+static void
+frogr_config_account_finalize (GObject *object)
+{
+ FrogrConfigAccount *acccount = FROGR_CONFIG_ACCOUNT (object);
+
+ if (acccount->frob != NULL) g_free (acccount->frob);
+ if (acccount->token != NULL) g_free (acccount->token);
+ if (acccount->username != NULL) g_free (acccount->username);
+
+ /* Call superclass */
+ G_OBJECT_CLASS (frogr_config_parent_class) -> finalize (object);
+}
+
+
+static void
+frogr_config_account_set_property (GObject *object,
+ guint property_id,
+ const GValue *value,
+ GParamSpec *pspec)
+{
+ FrogrConfigAccount *account = FROGR_CONFIG_ACCOUNT (object);
+
+ switch (property_id) {
+ case FROGR_CONFIG_ACCOUNT_FROB:
+ g_free (account->frob);
+ account->frob = g_value_dup_string (value);
+ break;
+ case FROGR_CONFIG_ACCOUNT_TOKEN:
+ g_free (account->token);
+ account->token = g_value_dup_string (value);
+ break;
+ case FROGR_CONFIG_ACCOUNT_USERNAME:
+ g_free (account->username);
+ account->username = g_value_dup_string (value);
+ break;
+ case FROGR_CONFIG_ACCOUNT_ENABLED:
+ account->enabled = g_value_get_boolean (value);
+ break;
+ case FROGR_CONFIG_ACCOUNT_PRIVATE:
+ account->private = g_value_get_boolean (value);
+ break;
+ case FROGR_CONFIG_ACCOUNT_PRIVATE_FAMILY:
+ account->private_family = g_value_get_boolean (value);
+ break;
+ case FROGR_CONFIG_ACCOUNT_PRIVATE_FRIENDS:
+ account->private_friends = g_value_get_boolean (value);
+ break;
+
+ default:
+ G_OBJECT_WARN_INVALID_PROPERTY_ID (object, property_id, pspec);
+ }
+}
+
+
+
+static void
+frogr_config_account_get_property (GObject *object,
+ guint property_id,
+ GValue *value,
+ GParamSpec *pspec)
+{
+ FrogrConfigAccount *account = FROGR_CONFIG_ACCOUNT (object);
+
+ switch (property_id) {
+ case FROGR_CONFIG_ACCOUNT_FROB:
+ g_value_set_string (value, account->frob);
+ break;
+ case FROGR_CONFIG_ACCOUNT_TOKEN:
+ g_value_set_string (value, account->token);
+ break;
+ case FROGR_CONFIG_ACCOUNT_USERNAME:
+ g_value_set_string (value, account->username);
+ break;
+ case FROGR_CONFIG_ACCOUNT_ENABLED:
+ g_value_set_boolean (value, account->enabled);
+ break;
+ case FROGR_CONFIG_ACCOUNT_PRIVATE:
+ g_value_set_boolean (value, account->private);
+ break;
+ case FROGR_CONFIG_ACCOUNT_PRIVATE_FAMILY:
+ g_value_set_boolean (value, account->private_family);
+ break;
+ case FROGR_CONFIG_ACCOUNT_PRIVATE_FRIENDS:
+ g_value_set_boolean (value, account->private_friends);
+ break;
+
+ default:
+ G_OBJECT_WARN_INVALID_PROPERTY_ID (object, property_id, pspec);
+ }
+}
+
+
+static void
+frogr_config_account_class_init (FrogrConfigAccountClass *klass)
+{
+ GObjectClass *obj_class = G_OBJECT_CLASS (klass);
+ GParamSpec *pspec;
+
+ obj_class->get_property = frogr_config_account_get_property;
+ obj_class->set_property = frogr_config_account_set_property;
+ obj_class->finalize = frogr_config_account_finalize;
+
+ pspec = g_param_spec_string ("frob",
+ "Flickr API authentication frob",
+ "Get/set Flickr frob",
+ "",
+ G_PARAM_READWRITE);
+ g_object_class_install_property (obj_class,
+ FROGR_CONFIG_ACCOUNT_FROB,
+ pspec);
+
+ pspec = g_param_spec_string ("token",
+ "Flickr API authentication token",
+ "Get/set Flickr token",
+ "",
+ G_PARAM_READWRITE);
+ g_object_class_install_property (obj_class,
+ FROGR_CONFIG_ACCOUNT_TOKEN,
+ pspec);
+
+ pspec = g_param_spec_string ("username",
+ "Flickr account user name",
+ "Get/set Flickr user name",
+ "",
+ G_PARAM_READWRITE);
+ g_object_class_install_property (obj_class,
+ FROGR_CONFIG_ACCOUNT_USERNAME,
+ pspec);
+
+ pspec = g_param_spec_boolean ("enabled",
+ "Whether the account is enabled",
+ "Get/set enabled status",
+ TRUE,
+ G_PARAM_READWRITE);
+ g_object_class_install_property (obj_class,
+ FROGR_CONFIG_ACCOUNT_ENABLED,
+ pspec);
+
+ pspec = g_param_spec_boolean ("private",
+ "Whether photos are private by default",
+ "Get/set private by default",
+ TRUE,
+ G_PARAM_READWRITE);
+ g_object_class_install_property (obj_class,
+ FROGR_CONFIG_ACCOUNT_PRIVATE,
+ pspec);
+
+ pspec = g_param_spec_boolean ("private-family",
+ "If sharing is private, whether to share with family",
+ "Get/set family sharing",
+ FALSE,
+ G_PARAM_READWRITE);
+ g_object_class_install_property (obj_class,
+ FROGR_CONFIG_ACCOUNT_PRIVATE_FAMILY,
+ pspec);
+
+ pspec = g_param_spec_boolean ("private-friends",
+ "If sharing is private, whether to share with friends",
+ "Get/set family sharing",
+ FALSE,
+ G_PARAM_READWRITE);
+ g_object_class_install_property (obj_class,
+ FROGR_CONFIG_ACCOUNT_PRIVATE_FRIENDS,
+ pspec);
+}
+
+
+static void
+frogr_config_account_init (FrogrConfigAccount *account)
+{
+ account->frob = NULL;
+ account->token = NULL;
+ account->username = NULL;
+ account->enabled = TRUE;
+ account->private = TRUE;
+ account->private_family = FALSE;
+ account->private_friends = FALSE;
+}
+
+
diff --git a/src/frogr-config.h b/src/frogr-config.h
new file mode 100644
index 0000000..d53f13c
--- /dev/null
+++ b/src/frogr-config.h
@@ -0,0 +1,84 @@
+/*
+ * frogr-config.h -- Configuration system for Frogr.
+ *
+ * Copyright (C) 2009 Adrian Perez
+ * Authors: Adrian Perez <ape...@igalia.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of version 3 of the GNU General Public
+ * License as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public
+ * License along with this program; if not, write to the
+ * Free Software Foundation, Inc., 59 Temple Place - Suite 330,
+ * Boston, MA 02111-1307, USA.
+ */
+
+#ifndef _FROGR_CONFIG_H
+#define _FROGR_CONFIG_H
+
+#include <glib.h>
+#include <glib-object.h>
+
+G_BEGIN_DECLS
+
+#define FROGR_CONFIG_TYPE (frogr_config_get_type())
+#define FROGR_IS_CONFIG(obj) (G_TYPE_CHECK_INSTANCE_TYPE(obj, FROGR_CONFIG_TYPE))
+#define FROGR_IS_CONFIG_CLASS(klass) (G_TYPE_CHECK_CLASS_TYPE(klass, FROGR_CONFIG_TYPE))
+#define FROGR_CONFIG(obj) (G_TYPE_CHECK_INSTANCE_CAST(obj, FROGR_CONFIG_TYPE, FrogrConfig))
+#define FROGR_CONFIG_CLASS(klass) (G_TYPE_CHECK_CLASS_CAST(klass, FROGR_CONFIG_TYPE, FrogrConfigClass))
+#define FROGR_CONFIG_GET_CLASS(obj) (G_TYPE_INSTANCE_GET_CLASS(obj, FROGR_CONFIG_TYPE, FrogrConfigClass))
+
+typedef struct _FrogrConfigClass FrogrConfigClass;
+typedef struct _FrogrConfig FrogrConfig;
+
+
+#define FROGR_CONFIG_ACCOUNT_TYPE (frogr_config_account_get_type())
+#define FROGR_IS_CONFIG_ACCOUNT(obj) (G_TYPE_CHECK_INSTANCE_TYPE(obj, FROGR_CONFIG_ACCOUNT_TYPE))
+#define FROGR_IS_CONFIG_ACCOUNT_CLASS(klass) (G_TYPE_CHECK_CLASS_TYPE(klass, FROGR_CONFIG_ACCOUNT_TYPE))
+#define FROGR_CONFIG_ACCOUNT(obj) (G_TYPE_CHECK_INSTANCE_CAST(obj, FROGR_CONFIG_ACCOUNT_TYPE, FrogrConfigAccount))
+#define FROGR_CONFIG_ACCOUNT_CLASS(klass) (G_TYPE_CHECK_CLASS_CAST(klass, FROGR_CONFIG_ACCOUNT_TYPE, FrogrConfigAccountClass))
+#define FROGR_CONFIG_ACCOUNT_GET_CLASS(obj) (G_TYPE_INSTANCE_GET_CLASS(obj, FROGR_CONFIG_ACCOUNT_TYPE, FrogrConfigAccountClass))
+
+typedef struct _FrogrConfigAccountClass FrogrConfigAccountClass;
+typedef struct _FrogrConfigAccount FrogrConfigAccount;
+
+
+/*
+ * FrogrConfig
+ * ===========
+ */
+
+GType frogr_config_get_type (void) G_GNUC_CONST;
+
+/*
+ * We do not have a constructor but a factory method instead, because there
+ * is only a single config object: it is a singleton.
+ */
+FrogrConfig* frogr_config_get (void);
+
+gboolean frogr_config_save (void);
+
+FrogrConfigAccount* frogr_config_get_account (const gchar *username);
+
+FrogrConfigAccount* frogr_config_get_default_account (void);
+
+void frogr_config_add_account (FrogrConfigAccount *account);
+
+/*
+ * FrogrConfigAccount
+ * ==================
+ */
+GType frogr_config_account_get_type(void) G_GNUC_CONST;
+
+FrogrConfigAccount* frogr_config_account_new (void);
+
+G_END_DECLS
+
+#endif /* !_FROGR_CONFIG_H */
+
--
1.6.3.2
Adrian Perez
authentication token, and load the authentication token to/from the
accounts configuration file. This is done nicely using the API of
the FrogrConfig and FrogrConfigAccount objects.
---
src/frogr-facade.c | 30 ++++++++++++++++++------------
src/main.c | 1 +
2 files changed, 19 insertions(+), 12 deletions(-)
diff --git a/src/frogr-facade.c b/src/frogr-facade.c
index ad1f788..2a301aa 100644
--- a/src/frogr-facade.c
+++ b/src/frogr-facade.c
@@ -20,10 +20,12 @@
*
*/
+#include <string.h>
#include <flickcurl.h>
#include "frogr-facade.h"
#include "frogr-controller.h"
#include "frogr-picture.h"
+#include "frogr-config.h"
#define API_KEY "18861766601de84f0921ce6be729f925"
#define SHARED_SECRET "6233fbefd85f733a"
@@ -39,8 +41,6 @@ struct _FrogrFacadePrivate
{
FrogrController *controller;
flickcurl *fcurl;
- gchar *frob;
- gchar *auth_token;
};
@@ -54,8 +54,6 @@ frogr_facade_finalize (GObject* object)
/* Free memory */
g_object_unref (priv -> controller);
flickcurl_free (priv -> fcurl);
- g_free (priv -> frob);
- g_free (priv -> auth_token);
/* Call superclass */
G_OBJECT_CLASS (frogr_facade_parent_class) -> finalize(object);
@@ -74,10 +72,7 @@ static void
frogr_facade_init (FrogrFacade *ffacade)
{
FrogrFacadePrivate *priv = FROGR_FACADE_GET_PRIVATE (ffacade);
-
- /* Set default values */
- priv -> auth_token = NULL;
- priv -> frob = NULL;
+ gchar *token;
/* Get controller */
priv -> controller = frogr_controller_get_instance ();
@@ -89,6 +84,12 @@ frogr_facade_init (FrogrFacade *ffacade)
/* Set API key and shared secret */
flickcurl_set_api_key(priv -> fcurl, API_KEY);
flickcurl_set_shared_secret(priv -> fcurl, SHARED_SECRET);
+
+ /* If available, set token and frob */
+ g_object_get (frogr_config_get_default_account (), "token", &token, NULL);
+ if (token != NULL && strlen(token) > 0)
+ flickcurl_set_auth_token (priv->fcurl, token);
+ g_free (token);
}
@@ -116,7 +117,8 @@ frogr_facade_get_authorization_url (FrogrFacade *ffacade)
gchar *api_sig;
/* Save frob value */
- priv -> frob = frob;
+ g_object_set (frogr_config_get_default_account (),
+ "frob", frob, NULL);
/* Build the authorization url */
sign_str = g_strdup_printf ("%sapi_key%sfrob%spermswrite",
@@ -138,23 +140,27 @@ frogr_facade_complete_authorization (FrogrFacade *ffacade)
{
g_return_if_fail(FROGR_IS_FACADE (ffacade));
+ FrogrConfigAccount *account = frogr_config_get_default_account ();
FrogrFacadePrivate *priv = FROGR_FACADE_GET_PRIVATE (ffacade);
gchar *auth_token = NULL;
+ gchar *frob = NULL;
+
+ g_object_get (account, "frob", &frob, NULL);
/* Check if frob value is present */
- if (!priv -> frob)
+ if (!frob || !strlen(frob))
{
g_debug ("No frob defined");
return;
}
/* Get auth token */
- auth_token = flickcurl_auth_getToken (priv -> fcurl, priv -> frob);
+ auth_token = flickcurl_auth_getToken (priv -> fcurl, frob);
if (auth_token)
{
/* Set and save the auth token */
flickcurl_set_auth_token(priv -> fcurl, auth_token);
- priv -> auth_token = auth_token;
+ g_object_set (account, "token", auth_token, NULL);
}
return (auth_token != NULL);
diff --git a/src/main.c b/src/main.c
index 267a8a8..f504835 100644
--- a/src/main.c
+++ b/src/main.c
@@ -40,6 +40,7 @@ main (int argc, char **argv)
/* Run app */
fcontroller = frogr_controller_get_instance ();
frogr_controller_run_app (fcontroller);
+ frogr_config_save ();
gdk_threads_leave ();
--
1.6.3.2
Mario Sanchez Prada
Overall they look good, just let me point out some issues I quickly see
right now, basically about coding style (I've not much more time now for
anything else :P):
Adrian Perez wrote:
> * FrogrConfig and FrogrConfigAccount classes.
>
> * Interface based on GObject properties: all configuration can be accessed
> as properties of objects. This makes them easy to bind to Gtk UI elements.
>
> * Support for loading/saving ~.config/frogr/accounts.xml
^^^^^^^^
Guess you meant ~/.config here ;-)
> * There is support for handling multiple accounts.
great!
> diff --git a/src/Makefile.am b/src/Makefile.am
> index a010107..4d2e854 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -42,4 +42,6 @@ frogr_SOURCES = \
> frogr-main-window.h \
> frogr-picture.c \
> frogr-picture.h \
> + frogr-config.c \
> + frogr-config.h \
Seeing that you've actually coded two classes, I miss here two
frogr-config-account.[ch] files :-)
> +#include "frogr-config.h"
> +#include <libxml/parser.h>
> +#include <string.h>
^^^^^^^^^^^^^^^^^^^
Wondering why you need this... if you included it to use strcmp to check
whether two strings are the same, you could also use g_str_equal for
that. But not checked yet if you use this for something else, just
guessing... :-)
> +enum FrogrConfigProperties {
> + FROGR_CONFIG_NUM_PROPERTIES,
> +};
Hmmm... I'd prefer to keep consistency in how we name properties in
frogr GObjects, so I'd replace the FROGR_CONFIG_ prefix by just PROP_,
call the first property PROP_0 and would not use a name for the
enumeration, as it's done in FrogrPicture:
enum {
PROP_0,
PROP_FILEPATH,
PROP_TITLE,
PROP_DESCRIPTION,
PROP_TAGS_STRING,
PROP_IS_PUBLIC,
PROP_IS_FAMILY,
PROP_IS_FRIEND
};
So, I'd replace your code with:
enum {
PROP_0
};
> +
> +enum FrogrConfigAccountProperties {
> + FROGR_CONFIG_ACCOUNT_ZERO = 0,
> +
Being coherent with the said above, I'd rename this to PROP_0. It should
not be a probelm if you also separate this two classes in different
files, as commented above.
> + FROGR_CONFIG_ACCOUNT_FROB,
> + FROGR_CONFIG_ACCOUNT_TOKEN,
> + FROGR_CONFIG_ACCOUNT_USERNAME,
> + FROGR_CONFIG_ACCOUNT_ENABLED,
> + FROGR_CONFIG_ACCOUNT_PRIVATE,
> + FROGR_CONFIG_ACCOUNT_PRIVATE_FAMILY,
> + FROGR_CONFIG_ACCOUNT_PRIVATE_FRIENDS,
PROP_FROB,
PROP_TOKEN,
PROP_USERNAME,
...
> + /* Add new ones just above this item */
> + FROGR_CONFIG_ACCOUNT_NUM_PROPERTIES,
I'd remove this one. It's not used in the code, it's not coherent with
other files in frogr and, moreover, it's not a common practice in
GObject programming to include it.
> +};
> +
> +
> +struct _FrogrConfig {
> + /* Parent */
> + GObject parent_instance;
> +
> + /* List of accounts. */
> + GList *accounts;
> +};
> +
> +struct _FrogrConfigClass {
> + GObjectClass parent_class;
> +};
> +
This should go to .h file, as it's done in FrogrPicture, for
consistency's sake.
> +
> +G_DEFINE_TYPE (FrogrConfig, frogr_config, G_TYPE_OBJECT);
> +
> +
> +
> +struct _FrogrConfigAccount {
> + /* Parent */
> + GObject parent_instance;
> +
> + /* Instance variables */
> + gchar *frob;
> + gchar *token;
> + gchar *username;
> +
> + gboolean enabled;
> + gboolean private;
> + gboolean private_family;
> + gboolean private_friends;
> +};
> +
Private data should go to an struct defined as
_FrogrConfigAccountPrivate and also "typedeffed" as
FrogrConfigAccountPrivate, inside the .c file, while the parent_instance
should go to another struct in the .h file. This private structure would
then be added with g_type_class_add_private() and used inside the
object's functions. See example from frogr-picture.[ch].
Morever, just for consistency's sake, I'll try to preserve the same
order in declarations than in other GOBjects: first the G_DEFINE's, then
the private struct, then the properties enumeration... see FrogrPicture
for a good example of it.
> +struct _FrogrConfigAccountClass {
> + GObjectClass parent_class;
> +};
> +
Same here -> .h file
> +G_DEFINE_TYPE (FrogrConfigAccount, frogr_config_account, G_TYPE_OBJECT);
> +
> +
> +
> +static gboolean
> +xml_node_to_boolean (const xmlNodePtr node)
All private functions, except those ones related to the GOBject boiler
plate (e.g xxx_finalize) should start with "_".
> +{
> + xmlChar *xstr;
> + gchar *str;
> + gboolean result;
> +
> + g_return_val_if_fail (node != NULL, FALSE);
> +
Be careful with g_return() functions, as they log a warning message in
the output of an application, so it's not the same than:
if (node != NULL)
return FALSE
Perhaps in this case it's ok, so I'm not saying you should change it,
pointing it out just in case (but you should decide).
> + xstr = xmlNodeGetContent (node);
> + str = (gchar*) xstr;
> +
> + while (g_ascii_isspace (*str++)); /* Skip blanks */
> +
> + if (g_ascii_isdigit (*str))
> + {
> + long value = strtol (str, NULL, 0);
^^^^^^
I think there are GLib options to get the same result, and even to avoid
this strange loop. Not sure though :-P
> [...]
> + /*
> + * The account object is created only of some minimum requirements are
> + * met: user name, token and frob.
> + */
> + if (frob != NULL && token != NULL && username != NULL)
> + {
> + account = g_object_new (FROGR_CONFIG_ACCOUNT_TYPE,
> + "frob", (const gchar*) frob,
> + "token", (const gchar*) token,
> + "username", (const gchar*) username,
> + "enabled", enabled,
> + "private", private,
> + "private-family", private_family,
> + "private-friends", private_friends,
> + NULL);
> + }
> +
> + if (frob != NULL) xmlFree (frob);
> + if (token != NULL) xmlFree (token);
> + if (username != NULL) xmlFree (username);
> +
> + return FROGR_CONFIG_ACCOUNT (account);
> +}
Not sure, but I guess it would be nice to have this code separated in a
frogr_config_account_new () function.
> [...]
> + if (xmlSaveFormatFileEnc (xml_path, xml, "UTF-8", 1) == -1) {
> + g_critical ("Unable to open '%s' for saving", xml_path);
> + retval = FALSE;
> + }
Coding style used "says" that brackets for an "if" statement should go
this way:
if (condition)
{
[...]
}
Same applies for other places in the code, just noticed this one.
> [...]
> +FrogrConfig*
> +frogr_config_get (void)
> +{
> + static GObject *conf = NULL;
> +
> + if (conf == NULL) conf = g_object_new (FROGR_CONFIG_TYPE, NULL);
> +
> + return FROGR_CONFIG (g_object_ref (conf));
> +}
I guess you're trying to implement a singleton here. I'd prefer then
you'd implement it in the same way it's done for FrogrController, which
is the same method explained in GObject documentation page.
> +
> +
> +FrogrConfigAccount*
> +frogr_config_get_default_account (void)
> +{
> + return frogr_config_get_account (NULL);
> +}
This method's prototype should be:
FrogrConfigAccount*
Frogr_config_get_default_account (FrogrConfig *fconfig)
{
[...]
}
...so you would asking to the FrogrConfig object for the default
account, instead of asking to "void" :-)
> +
> +void
> +frogr_config_add_account (FrogrConfigAccount *account)
^^^^^^^
Following current notation method, I'd call this "faccount".
> +{
> + FrogrConfig *config;
> +
> + g_return_if_fail (account != NULL);
Correct assertion *for public methods* of a GObject should be this way:
g_return_if_fail (FROGR_IS_CONFIG_ACCOUNT (account)).
...instead of just the current null check condition.
> +FrogrConfigAccount*
> +frogr_config_get_account (const gchar *username)
> +{
Same here: I miss the "FrogrConfig *fconfig" first parameter.
> +static void
> +frogr_config_account_set_property (GObject *object,
> + guint property_id,
> + const GValue *value,
> + GParamSpec *pspec)
I would call this _frogr_config_account_set_property.
> +static void
> +frogr_config_account_init (FrogrConfigAccount *account)
> +{
Once you address the "private struct" issue pointed out above, you
should have here something like:
FrogrConfigAccountPrivate *priv = FROGR_CONFIG_ACCOUNT_GET_PRIVATE
(faccount);
> + account->frob = NULL;
> + account->token = NULL;
> + account->username = NULL;
> + account->enabled = TRUE;
> + account->private = TRUE;
> + account->private_family = FALSE;
> + account->private_friends = FALSE;
...and then:
+ priv->frob = NULL;
+ priv->token = NULL;
+ ...
Ah! btw, remember to put spaces both sides of '->' usage:
Wrong: priv->frob
Right: priv -> frob
Just more coding style nitpicking, sorry :-)
> new file mode 100644
> index 0000000..d53f13c
> --- /dev/null
> +++ b/src/frogr-config.h
> @@ -0,0 +1,84 @@
> +/*
> + * frogr-config.h -- Configuration system for Frogr.
As said before, you should divide this file into two files:
frogr-config.h
frogr-config-account.h
Each class in separated files, please ;-)
> +typedef struct _FrogrConfigClass FrogrConfigClass;
> +typedef struct _FrogrConfig FrogrConfig;
As pointed out before, GObject's boilerplate structs here :-)
> +/*
> + * We do not have a constructor but a factory method instead, because there
> + * is only a single config object: it is a singleton.
> + */
> +FrogrConfig* frogr_config_get (void);
As pointed out before, use the same style to build a singleton than in
FrogrController, so rename this function as well as
frogr_config_get_instance.
> +gboolean frogr_config_save (void);
> +
> +FrogrConfigAccount* frogr_config_get_account (const gchar *username);
> +
> +FrogrConfigAccount* frogr_config_get_default_account (void);
> +
> +void frogr_config_add_account (FrogrConfigAccount *account);
> +
All these functions should have the first parameter FrogrConfig *fconfig
> +/*
> + * FrogrConfigAccount
> + * ==================
> + */
> +GType frogr_config_account_get_type(void) G_GNUC_CONST;
> +
> +FrogrConfigAccount* frogr_config_account_new (void);
Although not exactly needed, I love to see getters and setters as well
for properties (despite of being able to use g_object_set/get).
Well, and that's all... please accept my apologies for perhaps such a
nitpicking code commenting, but I think keeping a common coding style is
important and I know you're new to GOBject C coding, so I thought
pointing things out in a vey exhaustive way would be good :-)
Btw, please notice I've probably not commented on all the issues as I'm
not out of time and I've just commented the issues I was seeing on the
fly, so feel free to apply the advices to all the places you find
they're needed, regardless of me explicitly pointing them out.
At last, let me apologize once again for basically commenting on the
coding style and almost nothing in the business logic, but as I said I'm
running out of time completely and, as this is also important matter, I
preferred to comment this now and to let the logic comments for a later
stage (hopefully, once you've addressed this nitpicking issues).
Although it looks really good, as I said at the beginning.
So, thanks for the great work! Looking forward to see your next patches.
Thanks,
Mario
Mario Sanchez Prada
Thanks!
Mario
Adrian Perez de Castro
>
> Wow! Impressive work! Thanks!
>
> Overall they look good, just let me point out some issues I quickly see
> right now, basically about coding style (I've not much more time now for
> anything else :P):
No problem, I am learning, you know? ;-)
I will re-send the patch for review once I incorporate your suggestions.
> > * Support for loading/saving ~.config/frogr/accounts.xml
> ^^^^^^^^
> Guess you meant ~/.config here ;-)
Yep, this was a typo.
> Seeing that you've actually coded two classes, I miss here two
> frogr-config-account.[ch] files :-)
Okay, I will split in two.
> > +#include "frogr-config.h"
> > +#include <libxml/parser.h>
> > +#include <string.h>
> ^^^^^^^^^^^^^^^^^^^
> Wondering why you need this... if you included it to use strcmp to check
> whether two strings are the same, you could also use g_str_equal for
> that. But not checked yet if you use this for something else, just
> guessing... :-)
Did not know about g_str_equal()... doing s/strcmp/g_str_equal/.
> Hmmm... I'd prefer to keep consistency in how we name properties in
> frogr GObjects, so I'd replace the FROGR_CONFIG_ prefix by just PROP_,
> call the first property PROP_0 and would not use a name for the
> enumeration, as it's done in FrogrPicture:
>
> enum {
> PROP_0,
> PROP_FROB,
> PROP_TOKEN,
> PROP_USERNAME,
> ...
Understood.
> > + /* Add new ones just above this item */
> > + FROGR_CONFIG_ACCOUNT_NUM_PROPERTIES,
>
> I'd remove this one. It's not used in the code, it's not coherent with
> other files in frogr and, moreover, it's not a common practice in
> GObject programming to include it.
Okay.
> Private data should go to an struct defined as
> _FrogrConfigAccountPrivate and also "typedeffed" as
> FrogrConfigAccountPrivate, inside the .c file, while the parent_instance
> should go to another struct in the .h file. This private structure would
> then be added with g_type_class_add_private() and used inside the
> object's functions. See example from frogr-picture.[ch].
I already saw this, but using g_type_instance_get_private() incurs in an
*insane* overhead, e.g. about 14% of CPU usage in Gobby:
http://www.arbur.net/serendipity/archives/40-g_type_instance_get_private-performance-costs.html
That was the reason why I initially added instance variables directly in
the structure. A have been digging a bit in the GObject docs (they
reference manual is *very* good :P) and I will be adding a "priv"
pointer as instance variable and then retrieving it instance init
method, as outlined here:
http://library.gnome.org/devel/gobject/2.20/howto-gobject.html
> Morever, just for consistency's sake, I'll try to preserve the same
> order in declarations than in other GOBjects: first the G_DEFINE's, then
> the private struct, then the properties enumeration... see FrogrPicture
> for a good example of it.
Loooks reasonable :)
> All private functions, except those ones related to the GOBject boiler
> plate (e.g xxx_finalize) should start with "_".
No problem with this :)
> > +{
> > + xmlChar *xstr;
> > + gchar *str;
> > + gboolean result;
> > +
> > + g_return_val_if_fail (node != NULL, FALSE);
> > +
> Be careful with g_return() functions, as they log a warning message in
> the output of an application, so it's not the same than:
>
> if (node != NULL)
> return FALSE
If I have read correctly, g_return_* functions are for programming
errors, e.g. "soft" assertions for handling cases where bad arguments
are passed to functions but execution may continue... and that's the
case I try to handle :P
> Perhaps in this case it's ok, so I'm not saying you should change it,
> pointing it out just in case (but you should decide).
>
> > + xstr = xmlNodeGetContent (node);
> > + str = (gchar*) xstr;
> > +
> > + while (g_ascii_isspace (*str++)); /* Skip blanks */
> > +
> > + if (g_ascii_isdigit (*str))
> > + {
> > + long value = strtol (str, NULL, 0);
> ^^^^^^
> I think there are GLib options to get the same result, and even to avoid
> this strange loop. Not sure though :-P
The "while" look to skip spaces is an usual C idiom. I know I could use
g_strstrip() to remove blanks, but that would allocate a new string, so
in fact I am saving one g_malloc()/g_free() roundtrip. Every saved
allocation in C is a good thing, IMHO.
There is g_ascii_strtoll(), which converts a string to a 64-bit
integer... but using 64-bit integers for a simple boolean value seems
overkill. For the rest of types there is the standard C library. And as
far as I know Glib does not try to replace the standard C library
completely...
(http://library.gnome.org/devel/glib/unstable/glib-String-Utility-Functions.html#g-ascii-strtoll)
> > [...]
> > + /*
> > + * The account object is created only of some minimum requirements are
> > + * met: user name, token and frob.
> > + */
> [...]
>
> Not sure, but I guess it would be nice to have this code separated in a
> frogr_config_account_new () function.
No problem in adding syntactic sugar. I just wanted to have "something
working" this weekend.
> > + if (xmlSaveFormatFileEnc (xml_path, xml, "UTF-8", 1) == -1) {
> > + g_critical ("Unable to open '%s' for saving", xml_path);
> > + retval = FALSE;
> > + }
>
> Coding style used "says" that brackets for an "if" statement should go
> this way [...]
I tried to change my usual coding style to match the Gtk one, so it is
normal that some bad construct slips from time to time. I will fix this.
> > [...]
> > +FrogrConfig*
> > +frogr_config_get (void)
> > +{
> > + static GObject *conf = NULL;
> > +
> > + if (conf == NULL) conf = g_object_new (FROGR_CONFIG_TYPE, NULL);
> > +
> > + return FROGR_CONFIG (g_object_ref (conf));
> > +}
>
> I guess you're trying to implement a singleton here. I'd prefer then
> you'd implement it in the same way it's done for FrogrController, which
> is the same method explained in GObject documentation page.
Haha, it looks like I took the quick path here. Now that I know that
there is a standard idiom for singletons I will change this as well.
The same goes for the methods that should receive a FrogrConfig: I
assumed that being a singleton it would be of no use passing the
FrogrConfig, as implicitly it is known which instance to use... :)
So more changes for this as well. Great ¬_¬
> Correct assertion *for public methods* of a GObject should be this way:
>
> g_return_if_fail (FROGR_IS_CONFIG_ACCOUNT (account)).
>
> ...instead of just the current null check condition.
I suppose that the macro already checks for non-NULLness, then :P
> > +static void
> > +frogr_config_account_set_property (GObject *object,
> > + guint property_id,
> > + const GValue *value,
> > + GParamSpec *pspec)
>
> I would call this _frogr_config_account_set_property.
Yes, as it is private to the implementation, as you mentioned before.
> > +static void
> > +frogr_config_account_init (FrogrConfigAccount *account)
> > +{
> Ah! btw, remember to put spaces both sides of '->' usage:
I feel like this is not beautiful, but I will try to get used to it, to
follow a common style.
> As pointed out before, use the same style to build a singleton than in
> FrogrController, so rename this function as well as
> frogr_config_get_instance.
>
> > +gboolean frogr_config_save (void);
> > +
> > +FrogrConfigAccount* frogr_config_get_account (const gchar *username);
> > +
> > +FrogrConfigAccount* frogr_config_get_default_account (void);
> > +
> > +void frogr_config_add_account (FrogrConfigAccount *account);
> > +
>
> All these functions should have the first parameter FrogrConfig *fconfig
Okay.
> Although not exactly needed, I love to see getters and setters as well
> for properties (despite of being able to use g_object_set/get).
As pointed before, no problem in adding them.
> Well, and that's all... please accept my apologies [...]
>
> At last, let me apologize once again for basically commenting on the
> coding style and almost nothing in the business logic, but as I said I'm
> running out of time completely and, as this is also important matter, I
> preferred to comment this now and to let the logic comments for a later
> stage (hopefully, once you've addressed this nitpicking issues).
> Although it looks really good, as I said at the beginning.
Don't worry, as I have said before, I am getting some know-how, and you
are far more experienced than me in the Glib/GObject/GTK/Gnome world.
I think it is good to comment out style issues to newbies like me, so
people gets used to do the "good practices". If you fix these kind of
issues by yourself, contributors could get used to send crappy code
which does not follow a common set of guidelines. It is fat better to
instruct them how to send a good patch, thus making us improve in
making good code :)
> So, thanks for the great work! Looking forward to see your next patches.
Expect a revised version of these patches anytime soon, and thanks for
all the comments. I know you are short on spare time so all of them are
greatly appreciated.
Best regards,
--
Adrian Perez de Castro <ape...@igalia.com>
Igalia - Free Software Engineering
Mario Sanchez Prada
>> Overall they look good, just let me point out some issues I quickly see
>> right now, basically about coding style (I've not much more time now for
>> anything else :P):
>
> No problem, I am learning, you know? ;-)
Yep, like me ;-)
> I will re-send the patch for review once I incorporate your suggestions.
>
Nice!
>> Private data should go to an struct defined as
>> _FrogrConfigAccountPrivate and also "typedeffed" as
>> FrogrConfigAccountPrivate, inside the .c file, while the parent_instance
>> should go to another struct in the .h file. This private structure would
>> then be added with g_type_class_add_private() and used inside the
>> object's functions. See example from frogr-picture.[ch].
>
> I already saw this, but using g_type_instance_get_private() incurs in an
> *insane* overhead, e.g. about 14% of CPU usage in Gobby:
> http://www.arbur.net/serendipity/archives/40-g_type_instance_get_private-performance-costs.html
>
> That was the reason why I initially added instance variables directly in
> the structure. A have been digging a bit in the GObject docs (they
> reference manual is *very* good :P) and I will be adding a "priv"
> pointer as instance variable and then retrieving it instance init
> method, as outlined here:
> http://library.gnome.org/devel/gobject/2.20/howto-gobject.html
Not sure I follow, but I guess you mean having just a "priv" pointer
inside the object struct (in the .h file) pointing to the private
structure defined in the .c file.
I think all the options are good, but in this case, keeping coherency
with the rest of the app I'd prefer to keep using the
g_type_instance_get_private thing right now and, later on, if confirmed
a problem with that, we could always go ahead with the priv pointer.
>>> + g_return_val_if_fail (node != NULL, FALSE);
>>> +
>> Be careful with g_return() functions, as they log a warning message in
>> the output of an application, so it's not the same than:
>>
>> if (node != NULL)
>> return FALSE
>
> If I have read correctly, g_return_* functions are for programming
> errors, e.g. "soft" assertions for handling cases where bad arguments
> are passed to functions but execution may continue... and that's the
> case I try to handle :P
As I said, I have not gone too much through the logic of the code, but
if that's the case then I've got nothing to say :-)
>> I think there are GLib options to get the same result, and even to avoid
>> this strange loop. Not sure though :-P
>
> The "while" look to skip spaces is an usual C idiom. I know I could use
> g_strstrip() to remove blanks, but that would allocate a new string, so
> in fact I am saving one g_malloc()/g_free() roundtrip. Every saved
> allocation in C is a good thing, IMHO.
Well, g_strstrip does not allocate new memory :-D. From Gtk docs:
"This function doesn't allocate or reallocate any memory; it modifies
string in place. The pointer to string is returned to allow the nesting
of functions. "
> There is g_ascii_strtoll(), which converts a string to a 64-bit
> integer... but using 64-bit integers for a simple boolean value seems
> overkill. For the rest of types there is the standard C library. And as
> far as I know Glib does not try to replace the standard C library
> completely...
As I said before, if you have a good reason for it, no problem. Just
pointing it out in case you haven't, but it seems you have :-)
> (http://library.gnome.org/devel/glib/unstable/glib-String-Utility-Functions.html#g-ascii-strtoll)
>
>>> [...]
>>> + /*
>>> + * The account object is created only of some minimum requirements are
>>> + * met: user name, token and frob.
>>> + */
>> [...]
>>
>> Not sure, but I guess it would be nice to have this code separated in a
>> frogr_config_account_new () function.
>
> No problem in adding syntactic sugar. I just wanted to have "something
> working" this weekend.
And that's great, don't get me wrong. I'm just trying to spot all the
*minor* issues that keep your code out of the repo now :-)
>>> + if (xmlSaveFormatFileEnc (xml_path, xml, "UTF-8", 1) == -1) {
>>> + g_critical ("Unable to open '%s' for saving", xml_path);
>>> + retval = FALSE;
>>> + }
>> Coding style used "says" that brackets for an "if" statement should go
>> this way [...]
>
> I tried to change my usual coding style to match the Gtk one, so it is
> normal that some bad construct slips from time to time. I will fix this.
Yes yes, no problem, just seen and pointing it out. I'm sure I've also
made this error somewhere else. You know, getting used to use different
coding styles everyday, depending on the project usually gets this effec :-P
>>> [...]
>>> +FrogrConfig*
>>> +frogr_config_get (void)
>>> +{
>>> + static GObject *conf = NULL;
>>> +
>>> + if (conf == NULL) conf = g_object_new (FROGR_CONFIG_TYPE, NULL);
>>> +
>>> + return FROGR_CONFIG (g_object_ref (conf));
>>> +}
>> I guess you're trying to implement a singleton here. I'd prefer then
>> you'd implement it in the same way it's done for FrogrController, which
>> is the same method explained in GObject documentation page.
>
> Haha, it looks like I took the quick path here. Now that I know that
> there is a standard idiom for singletons I will change this as well.
> The same goes for the methods that should receive a FrogrConfig: I
> assumed that being a singleton it would be of no use passing the
> FrogrConfig, as implicitly it is known which instance to use... :)
:-) Ok, no problem at all.
>> Correct assertion *for public methods* of a GObject should be this way:
>>
>> g_return_if_fail (FROGR_IS_CONFIG_ACCOUNT (account)).
>>
>> ...instead of just the current null check condition.
>
> I suppose that the macro already checks for non-NULLness, then :P
:-)
>> Ah! btw, remember to put spaces both sides of '->' usage:
>
> I feel like this is not beautiful, but I will try to get used to it, to
> follow a common style.
Well, just coding style. If we think it's better otherwise we could
always change it later on.
>> Well, and that's all... please accept my apologies [...]
>>
>> At last, let me apologize once again for basically commenting on the
>> coding style and almost nothing in the business logic, but as I said I'm
>> running out of time completely and, as this is also important matter, I
>> preferred to comment this now and to let the logic comments for a later
>> stage (hopefully, once you've addressed this nitpicking issues).
>> Although it looks really good, as I said at the beginning.
>
> Don't worry, as I have said before, I am getting some know-how, and you
> are far more experienced than me in the Glib/GObject/GTK/Gnome world.
Nice to know I explain myself properly :-)
> I think it is good to comment out style issues to newbies like me, so
> people gets used to do the "good practices". If you fix these kind of
> issues by yourself, contributors could get used to send crappy code
> which does not follow a common set of guidelines. It is fat better to
> instruct them how to send a good patch, thus making us improve in
> making good code :)
Well it's not about me not wanting to change the patch, it's more that
I'd love to see contributors work integrated "as it is" whenever
possible, and your patch is really great, so before pushing it I'd like
just to see it in a "more proper" shape to avoid further changes as much
as possible.
>> So, thanks for the great work! Looking forward to see your next patches.
>
> Expect a revised version of these patches anytime soon, and thanks for
> all the comments. I know you are short on spare time so all of them are
> greatly appreciated.
Yep, I'm really short of time now, so do not worry too much for being to
soon.
Thanks!
Mario