GNOME.org
 

[gnome-initial-setup: 1/2] gis-page: Add an error argument to save_data()

commit 9eecaf1a2543684f6ce448d3415fa1721425523c
Author: Philip Withnall <withnall endlessm com>
Date:   Fri Feb 14 17:03:06 2020 +0000

    gis-page: Add an error argument to save_data()
    
    This allows errors to be returned from pages if saving data fails.
    Currently, the top-level handling for these is to print a `g_warning()`,
    but that could change in future to display the errors in the UI.
    
    This introduces one behaviour difference from before:
    `gis_driver_save_data()` now stops saving data after the first error
    from a page, rather than carrying on calling `save_data()` on all pages
    regardless.
    
    Signed-off-by: Philip Withnall <withnall endlessm com>
    
    Fixes: #95

 gnome-initial-setup/gis-assistant.c                | 12 ++++--
 gnome-initial-setup/gis-assistant.h                |  3 +-
 gnome-initial-setup/gis-driver.c                   |  9 ++--
 gnome-initial-setup/gis-driver.h                   |  3 +-
 gnome-initial-setup/gis-page.c                     | 14 +++++--
 gnome-initial-setup/gis-page.h                     |  6 ++-
 .../pages/account/gis-account-page-local.c         | 48 ++++++++++++++--------
 .../pages/account/gis-account-page-local.h         |  5 ++-
 .../pages/account/gis-account-page.c               | 12 +++---
 .../parental-controls/gis-parental-controls-page.c | 40 ++++++++----------
 .../pages/password/gis-password-page.c             | 12 +++---
 .../pages/summary/gis-summary-page.c               |  8 +++-
 12 files changed, 102 insertions(+), 70 deletions(-)
---
diff --git a/gnome-initial-setup/gis-assistant.c b/gnome-initial-setup/gis-assistant.c
index cb7e1bd0..2ea0035b 100644
--- a/gnome-initial-setup/gis-assistant.c
+++ b/gnome-initial-setup/gis-assistant.c
@@ -428,14 +428,20 @@ gis_assistant_locale_changed (GisAssistant *assistant)
   update_titlebar (assistant);
 }
 
-void
-gis_assistant_save_data (GisAssistant *assistant)
+gboolean
+gis_assistant_save_data (GisAssistant  *assistant,
+                         GError       **error)
 {
   GisAssistantPrivate *priv = gis_assistant_get_instance_private (assistant);
   GList *l;
 
   for (l = priv->pages; l != NULL; l = l->next)
-    gis_page_save_data (l->data);
+    {
+      if (!gis_page_save_data (l->data, error))
+        return FALSE;
+    }
+
+  return TRUE;
 }
 
 static void
diff --git a/gnome-initial-setup/gis-assistant.h b/gnome-initial-setup/gis-assistant.h
index 978185a8..5e2dd9fc 100644
--- a/gnome-initial-setup/gis-assistant.h
+++ b/gnome-initial-setup/gis-assistant.h
@@ -59,7 +59,8 @@ const gchar *gis_assistant_get_title      (GisAssistant *assistant);
 GtkWidget *gis_assistant_get_titlebar     (GisAssistant *assistant);
 
 void      gis_assistant_locale_changed    (GisAssistant *assistant);
-void      gis_assistant_save_data         (GisAssistant *assistant);
+gboolean  gis_assistant_save_data         (GisAssistant  *assistant,
+                                           GError       **error);
 
 G_END_DECLS
 
diff --git a/gnome-initial-setup/gis-driver.c b/gnome-initial-setup/gis-driver.c
index eba34f44..f6f9a049 100644
--- a/gnome-initial-setup/gis-driver.c
+++ b/gnome-initial-setup/gis-driver.c
@@ -1025,18 +1025,19 @@ gis_driver_class_init (GisDriverClass *klass)
   g_object_class_install_properties (gobject_class, G_N_ELEMENTS (obj_props), obj_props);
 }
 
-void
-gis_driver_save_data (GisDriver *driver)
+gboolean
+gis_driver_save_data (GisDriver  *driver,
+                      GError    **error)
 {
   GisDriverPrivate *priv = gis_driver_get_instance_private (driver);
 
   if (gis_get_mock_mode ())
     {
       g_message ("%s: Skipping saving data due to being in mock mode", G_STRFUNC);
-      return;
+      return TRUE;
     }
 
-  gis_assistant_save_data (priv->assistant);
+  return gis_assistant_save_data (priv->assistant, error);
 }
 
 GisDriver *
diff --git a/gnome-initial-setup/gis-driver.h b/gnome-initial-setup/gis-driver.h
index aace51d3..59ea1a2c 100644
--- a/gnome-initial-setup/gis-driver.h
+++ b/gnome-initial-setup/gis-driver.h
@@ -126,7 +126,8 @@ void gis_driver_add_page (GisDriver *driver,
 
 void gis_driver_hide_window (GisDriver *driver);
 
-void gis_driver_save_data (GisDriver *driver);
+gboolean gis_driver_save_data (GisDriver  *driver,
+                               GError    **error);
 
 gboolean gis_driver_conf_get_boolean (GisDriver *driver,
                                       const gchar *group,
diff --git a/gnome-initial-setup/gis-page.c b/gnome-initial-setup/gis-page.c
index 8564935b..d92b11c9 100644
--- a/gnome-initial-setup/gis-page.c
+++ b/gnome-initial-setup/gis-page.c
@@ -369,11 +369,17 @@ gis_page_apply_cancel (GisPage *page)
   g_cancellable_cancel (priv->apply_cancel);
 }
 
-void
-gis_page_save_data (GisPage *page)
+gboolean
+gis_page_save_data (GisPage  *page,
+                    GError  **error)
 {
-  if (GIS_PAGE_GET_CLASS (page)->save_data)
-    GIS_PAGE_GET_CLASS (page)->save_data (page);
+  if (GIS_PAGE_GET_CLASS (page)->save_data == NULL)
+    {
+      /* Not implemented, which presumably means the page has nothing to save. */
+      return TRUE;
+    }
+
+  return GIS_PAGE_GET_CLASS (page)->save_data (page, error);
 }
 
 void
diff --git a/gnome-initial-setup/gis-page.h b/gnome-initial-setup/gis-page.h
index 9bc9af43..d98590a2 100644
--- a/gnome-initial-setup/gis-page.h
+++ b/gnome-initial-setup/gis-page.h
@@ -60,7 +60,8 @@ struct _GisPageClass
   void         (*locale_changed) (GisPage *page);
   gboolean     (*apply) (GisPage *page,
                          GCancellable *cancellable);
-  void         (*save_data) (GisPage *page);
+  gboolean     (*save_data) (GisPage  *page,
+                             GError  **error);
   void         (*shown) (GisPage *page);
   gboolean     (*skip) (GisPage *page);
 };
@@ -80,7 +81,8 @@ void         gis_page_apply_begin (GisPage *page, GisPageApplyCallback callback,
 void         gis_page_apply_cancel (GisPage *page);
 void         gis_page_apply_complete (GisPage *page, gboolean valid);
 gboolean     gis_page_get_applying (GisPage *page);
-void         gis_page_save_data (GisPage *page);
+gboolean     gis_page_save_data (GisPage  *page,
+                                 GError  **error);
 void         gis_page_shown (GisPage *page);
 gboolean     gis_page_skip (GisPage *page);
 
diff --git a/gnome-initial-setup/pages/account/gis-account-page-local.c 
b/gnome-initial-setup/pages/account/gis-account-page-local.c
index ade79c6a..c858c608 100644
--- a/gnome-initial-setup/pages/account/gis-account-page-local.c
+++ b/gnome-initial-setup/pages/account/gis-account-page-local.c
@@ -534,14 +534,14 @@ set_user_avatar (GisAccountPageLocal *page,
   g_clear_object (&file);
 }
 
-static void
-local_create_user (GisAccountPageLocal *local,
-                   GisPage             *page)
+static gboolean
+local_create_user (GisAccountPageLocal  *local,
+                   GisPage              *page,
+                   GError              **error)
 {
   GisAccountPageLocalPrivate *priv = gis_account_page_local_get_instance_private (local);
   const gchar *username;
   const gchar *fullname;
-  g_autoptr(GError) local_error = NULL;
   gboolean parental_controls_enabled;
   g_autoptr(ActUser) main_user = NULL;
   g_autoptr(ActUser) parent_user = NULL;
@@ -553,15 +553,19 @@ local_create_user (GisAccountPageLocal *local,
   /* Always create the admin user first, in case of failure part-way through
    * this function, which would leave us with no admin user at all. */
   if (parental_controls_enabled) {
+    g_autoptr(GError) local_error = NULL;
     g_autoptr(GDBusConnection) connection = NULL;
     const gchar *parent_username = "administrator";
     const gchar *parent_fullname = _("Administrator");
 
-    parent_user = act_user_manager_create_user (priv->act_client, parent_username, parent_fullname, 
ACT_USER_ACCOUNT_TYPE_ADMINISTRATOR, &local_error);
-    if (local_error != NULL) {
-      g_warning ("Failed to create parent user: %s", local_error->message);
-      return;
-    }
+    parent_user = act_user_manager_create_user (priv->act_client, parent_username, parent_fullname, 
ACT_USER_ACCOUNT_TYPE_ADMINISTRATOR, error);
+    if (parent_user == NULL)
+      {
+        g_prefix_error (error,
+                        _("Failed to create user '%s': "),
+                        parent_username);
+        return FALSE;
+      }
 
     /* Make the admin account usable in case g-i-s crashes. If all goes
      * according to plan a password will be set on it in gis-password-page.c */
@@ -597,15 +601,22 @@ local_create_user (GisAccountPageLocal *local,
   }
 
   /* Now create the main user. */
-  main_user = act_user_manager_create_user (priv->act_client, username, fullname, priv->account_type, 
&local_error);
-  if (local_error != NULL) {
-    g_warning ("Failed to create user: %s", local_error->message);
-    return;
-  }
+  main_user = act_user_manager_create_user (priv->act_client, username, fullname, priv->account_type, error);
+  if (main_user == NULL)
+    {
+      g_prefix_error (error,
+                      _("Failed to create user '%s': "),
+                      username);
+      /* FIXME: Could we delete the @parent_user at this point to reset the state
+       * and allow g-i-s to be run again after a reboot? */
+      return FALSE;
+    }
 
   set_user_avatar (local, main_user);
 
   g_signal_emit (local, signals[MAIN_USER_CREATED], 0, main_user, "");
+
+  return TRUE;
 }
 
 static void
@@ -659,11 +670,12 @@ gis_account_page_local_validate (GisAccountPageLocal *page)
   return priv->valid_name && priv->valid_username;
 }
 
-void
-gis_account_page_local_create_user (GisAccountPageLocal *local,
-                                    GisPage             *page)
+gboolean
+gis_account_page_local_create_user (GisAccountPageLocal  *local,
+                                    GisPage              *page,
+                                    GError              **error)
 {
-  local_create_user (local, page);
+  return local_create_user (local, page, error);
 }
 
 gboolean
diff --git a/gnome-initial-setup/pages/account/gis-account-page-local.h 
b/gnome-initial-setup/pages/account/gis-account-page-local.h
index 7e7fea1c..1674ad88 100644
--- a/gnome-initial-setup/pages/account/gis-account-page-local.h
+++ b/gnome-initial-setup/pages/account/gis-account-page-local.h
@@ -50,8 +50,9 @@ GType gis_account_page_local_get_type (void);
 
 gboolean gis_account_page_local_validate (GisAccountPageLocal *local);
 gboolean gis_account_page_local_apply (GisAccountPageLocal *local, GisPage *page);
-void gis_account_page_local_create_user (GisAccountPageLocal *local,
-                                         GisPage             *page);
+gboolean gis_account_page_local_create_user (GisAccountPageLocal  *local,
+                                             GisPage              *page,
+                                             GError              **error);
 void gis_account_page_local_shown (GisAccountPageLocal *local);
 
 G_END_DECLS
diff --git a/gnome-initial-setup/pages/account/gis-account-page.c 
b/gnome-initial-setup/pages/account/gis-account-page.c
index ebccf969..f319a267 100644
--- a/gnome-initial-setup/pages/account/gis-account-page.c
+++ b/gnome-initial-setup/pages/account/gis-account-page.c
@@ -142,20 +142,22 @@ gis_account_page_apply (GisPage *gis_page,
   }
 }
 
-static void
-gis_account_page_save_data (GisPage *gis_page)
+static gboolean
+gis_account_page_save_data (GisPage  *gis_page,
+                            GError  **error)
 {
   GisAccountPage *page = GIS_ACCOUNT_PAGE (gis_page);
   GisAccountPagePrivate *priv = gis_account_page_get_instance_private (page);
 
   switch (priv->mode) {
   case UM_LOCAL:
-    gis_account_page_local_create_user (GIS_ACCOUNT_PAGE_LOCAL (priv->page_local), gis_page);
-    break;
+    return gis_account_page_local_create_user (GIS_ACCOUNT_PAGE_LOCAL (priv->page_local), gis_page, error);
   case UM_ENTERPRISE:
-    break;
+    /* Nothing to do. */
+    return TRUE;
   default:
     g_assert_not_reached ();
+    return FALSE;
   }
 }
 
diff --git a/gnome-initial-setup/pages/parental-controls/gis-parental-controls-page.c 
b/gnome-initial-setup/pages/parental-controls/gis-parental-controls-page.c
index 92a5f293..89f3e34a 100644
--- a/gnome-initial-setup/pages/parental-controls/gis-parental-controls-page.c
+++ b/gnome-initial-setup/pages/parental-controls/gis-parental-controls-page.c
@@ -46,49 +46,41 @@ struct _GisParentalControlsPage
 
 G_DEFINE_TYPE (GisParentalControlsPage, gis_parental_controls_page, GIS_TYPE_PAGE)
 
-static void
-gis_parental_controls_page_save_data (GisPage *gis_page)
+static gboolean
+gis_parental_controls_page_save_data (GisPage  *gis_page,
+                                      GError  **error)
 {
   GisParentalControlsPage *page = GIS_PARENTAL_CONTROLS_PAGE (gis_page);
   g_autoptr(GDBusConnection) system_bus = NULL;
   g_autoptr(MctManager) manager = NULL;
   g_auto(MctAppFilterBuilder) builder = MCT_APP_FILTER_BUILDER_INIT ();
   g_autoptr(MctAppFilter) app_filter = NULL;
-  g_autoptr(GError) local_error = NULL;
   ActUser *main_user;
 
   /* The parent and child users are created by the #GisAccountPage earlier in
    * the save_data() process. We now need to set the parental controls on the
-   * child user. */
+   * child user. The earlier step in the process must have succeeded. */
   gis_driver_get_user_permissions (gis_page->driver, &main_user, NULL);
-
-  /* FIXME: https://gitlab.gnome.org/GNOME/gnome-initial-setup/issues/95 */
-  g_return_if_fail (main_user != NULL);
+  g_return_val_if_fail (main_user != NULL, FALSE);
 
   mct_user_controls_build_app_filter (MCT_USER_CONTROLS (page->user_controls), &builder);
   app_filter = mct_app_filter_builder_end (&builder);
 
   /* FIXME: should become asynchronous */
-  system_bus = g_bus_get_sync (G_BUS_TYPE_SYSTEM, NULL, &local_error);
+  system_bus = g_bus_get_sync (G_BUS_TYPE_SYSTEM, NULL, error);
   if (system_bus == NULL)
-    {
-      g_warning ("Error getting system bus while setting up parental controls: %s", local_error->message);
-      return;
-    }
+    return FALSE;
 
   manager = mct_manager_new (system_bus);
-  mct_manager_set_app_filter (manager,
-                              act_user_get_uid (main_user),
-                              app_filter,
-                              MCT_MANAGER_SET_VALUE_FLAGS_NONE,
-                              NULL,
-                              &local_error);
-
-  if (local_error != NULL)
-    {
-      g_warning ("Error setting parental controls: %s", local_error->message);
-      return;
-    }
+  if (!mct_manager_set_app_filter (manager,
+                                   act_user_get_uid (main_user),
+                                   app_filter,
+                                   MCT_MANAGER_SET_VALUE_FLAGS_NONE,
+                                   NULL,
+                                   error))
+    return FALSE;
+
+  return TRUE;
 }
 
 static void
diff --git a/gnome-initial-setup/pages/password/gis-password-page.c 
b/gnome-initial-setup/pages/password/gis-password-page.c
index e15a9b51..d7a62cdf 100644
--- a/gnome-initial-setup/pages/password/gis-password-page.c
+++ b/gnome-initial-setup/pages/password/gis-password-page.c
@@ -145,8 +145,9 @@ update_page_validation (GisPasswordPage *page)
   gis_page_set_complete (GIS_PAGE (page), page_validate (page));
 }
 
-static void
-gis_password_page_save_data (GisPage *gis_page)
+static gboolean
+gis_password_page_save_data (GisPage  *gis_page,
+                             GError  **error)
 {
   GisPasswordPage *page = GIS_PASSWORD_PAGE (gis_page);
   GisPasswordPagePrivate *priv = gis_password_page_get_instance_private (page);
@@ -154,8 +155,7 @@ gis_password_page_save_data (GisPage *gis_page)
   UmAccountMode account_mode;
   const gchar *password = NULL;
 
-  if (gis_page->driver == NULL)
-    return;
+  g_assert (gis_page->driver != NULL);
 
   account_mode = gis_driver_get_account_mode (gis_page->driver);
 
@@ -169,7 +169,7 @@ gis_password_page_save_data (GisPage *gis_page)
 
     if (password != NULL)
       gis_update_login_keyring_password (password);
-    return;
+    return TRUE;
   }
 
   password = gtk_entry_get_text (GTK_ENTRY (priv->password_entry));
@@ -186,6 +186,8 @@ gis_password_page_save_data (GisPage *gis_page)
 
   if (!priv->parent_mode)
     gis_update_login_keyring_password (password);
+
+  return TRUE;
 }
 
 static void
diff --git a/gnome-initial-setup/pages/summary/gis-summary-page.c 
b/gnome-initial-setup/pages/summary/gis-summary-page.c
index a3f8e2c2..db232c7f 100644
--- a/gnome-initial-setup/pages/summary/gis-summary-page.c
+++ b/gnome-initial-setup/pages/summary/gis-summary-page.c
@@ -201,8 +201,14 @@ gis_summary_page_shown (GisPage *page)
 {
   GisSummaryPage *summary = GIS_SUMMARY_PAGE (page);
   GisSummaryPagePrivate *priv = gis_summary_page_get_instance_private (summary);
+  g_autoptr(GError) local_error = NULL;
 
-  gis_driver_save_data (GIS_PAGE (page)->driver);
+  if (!gis_driver_save_data (GIS_PAGE (page)->driver, &local_error))
+    {
+      /* FIXME: This should probably be shown to the user and some options
+       * provided to them. */
+      g_warning ("Error saving data: %s", local_error->message);
+    }
 
   gis_driver_get_user_permissions (GIS_PAGE (page)->driver,
                                    &priv->user_account,
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]