On Thu, Feb 25, 2016 at 11:37 AM, Victor Toso <lists@xxxxxxxxxxxxxx> wrote: > Hi, > > On Thu, Feb 25, 2016 at 08:21:00AM +0100, Fabiano Fidêncio wrote: >> The function was a bit hard to read and, mainly, hard to expand in a >> readable way when adding one more directory to try to load the ui file >> from. >> >> Signed-off-by: Fabiano Fidêncio <fidencio@xxxxxxxxxx> >> --- >> src/virt-viewer-util.c | 60 ++++++++++++++++++++++++++++++-------------------- >> 1 file changed, 36 insertions(+), 24 deletions(-) >> >> diff --git a/src/virt-viewer-util.c b/src/virt-viewer-util.c >> index 76b61a1..6fe6c04 100644 >> --- a/src/virt-viewer-util.c >> +++ b/src/virt-viewer-util.c >> @@ -47,6 +47,40 @@ virt_viewer_error_quark(void) >> return g_quark_from_static_string ("virt-viewer-error-quark"); >> } >> >> +static gboolean >> +virt_viewer_util_try_to_load_ui_from_system_data_dirs(GtkBuilder *builder, const gchar *name, GError **error) >> +{ >> + const gchar * const * dirs = g_get_system_data_dirs(); >> + g_return_val_if_fail(dirs != NULL, FALSE); >> + gboolean success = FALSE; > > Particularly, I find easier to read it without the success variable.. > >> + >> + while (dirs[0] != NULL && !success) { >> + gchar *path = g_build_filename(dirs[0], PACKAGE, "ui", name, NULL); >> + success = (gtk_builder_add_from_file(builder, path, error) != 0); > > .. with a if here and return TRUE directly. It might duplicates the > g_free but it one less variable to consider overall I disagree. So, I'll keep it as it is. > >> + g_free(path); >> + >> + dirs++; >> + } >> + >> + return success; >> +} >> + >> +static gboolean >> +virt_viewer_util_try_to_load_ui_from_non_system_data_dirs(GtkBuilder *builder, const gchar *name, GError **error) >> +{ >> + gchar *path = g_build_filename(PACKAGE_DATADIR, "ui", name, NULL); >> + gboolean success = (gtk_builder_add_from_file(builder, path, error) != 0); >> + >> + if (*error != NULL) { >> + if (!((*error)->domain == G_FILE_ERROR && (*error)->code == G_FILE_ERROR_NOENT)) > > g_error_matches() could be used here I guess \o/ Actually, I'll introduce a first patch just changing it to g_error_matches(), rebase and re-send the others. > > Cheers, > toso > >> + g_warning("Failed to add ui file '%s': %s", path, (*error)->message); >> + g_clear_error(error); >> + } >> + g_free(path); >> + >> + return success; >> +} >> + >> GtkBuilder *virt_viewer_util_load_ui(const char *name) >> { >> struct stat sb; >> @@ -57,31 +91,9 @@ GtkBuilder *virt_viewer_util_load_ui(const char *name) >> if (stat(name, &sb) >= 0) { >> gtk_builder_add_from_file(builder, name, &error); >> } else { >> - gchar *path = g_build_filename(PACKAGE_DATADIR, "ui", name, NULL); >> - gboolean success = (gtk_builder_add_from_file(builder, path, &error) != 0); >> - if (error) { >> - if (!(error->domain == G_FILE_ERROR && error->code == G_FILE_ERROR_NOENT)) >> - g_warning("Failed to add ui file '%s': %s", path, error->message); >> - g_clear_error(&error); >> - } >> - g_free(path); >> - >> - if (!success) { >> - const gchar * const * dirs = g_get_system_data_dirs(); >> - g_return_val_if_fail(dirs != NULL, NULL); >> - >> - while (dirs[0] != NULL) { >> - path = g_build_filename(dirs[0], PACKAGE, "ui", name, NULL); >> - if (gtk_builder_add_from_file(builder, path, NULL) != 0) { >> - g_free(path); >> - break; >> - } >> - g_free(path); >> - dirs++; >> - } >> - if (dirs[0] == NULL) >> + if (!virt_viewer_util_try_to_load_ui_from_non_system_data_dirs(builder, name, &error)) >> + if (!virt_viewer_util_try_to_load_ui_from_system_data_dirs(builder, name, &error)) >> goto failed; >> - } >> } >> >> if (error) { >> -- >> 2.5.0 >> >> _______________________________________________ >> virt-tools-list mailing list >> virt-tools-list@xxxxxxxxxx >> https://www.redhat.com/mailman/listinfo/virt-tools-list > > _______________________________________________ > virt-tools-list mailing list > virt-tools-list@xxxxxxxxxx > https://www.redhat.com/mailman/listinfo/virt-tools-list Thanks for the review. -- Fabiano Fidêncio _______________________________________________ virt-tools-list mailing list virt-tools-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/virt-tools-list