[virt-tools-list] [virt-viewer 1/2] util: slightly reorganize _load_ui()

[Fabiano Fidêncio]

On Thu, Feb 25, 2016 at 11:37 AM, Victor Toso <lists at victortoso.com> 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 at redhat.com>
>> ---
>>  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 at redhat.com
>> https://www.redhat.com/mailman/listinfo/virt-tools-list
>
> _______________________________________________
> virt-tools-list mailing list
> virt-tools-list at redhat.com
> https://www.redhat.com/mailman/listinfo/virt-tools-list


Thanks for the review.
-- 
Fabiano Fidêncio