Re: New "Set as favorite" plugin, feedback request

[Felix Riemann <friemann gnome org>]

Hi Alessio,

Am Montag, den 21.03.2011, 19:40 +0100 schrieb Alessio 'molok'
Bolognino:
> Hi guys,
> I wrote a new plugin for EOG: "Set as favorite"; it's a plugin to
> set/toggle the favorite emblem on pictures from EOG, when it's set it
> shows a little heart ( ❤ ) in the statusbar. There is a feature request
> about this in the bugtracker [1], even though it has not received much
> attention.
> 
> My C/GTK is a bit rusty, so I'd like to hear feedback on the code.
> Moreover, I wrote it to work with EOG 2.32.0, which is the version I
> have installed on this machine, I just read that the plugin API is going
> to change, I am willing to port it to EOG 2.9X.
> 
> What would be the path to follow to include my plugin in EOG?

The inclusion point for your plugin would be the eog-plugins package.
We have no hard approval process or criteria, it's more case-by-case
decision. Just propose your plugin for inclusion (either here or in our
Bugzilla) after porting it to the new API  and we'll talk about it. We
usually work with patch series but I guess merging from a
GitHub/Gitorious clone of the eog-plugins repo could be worth a try too.

I've also taken a look over your code and so far I couldn't find
anything serious (although I didn't run your plugin yet). :)
Just a few remarks:

- #define EOG_SET_AS_FAVORITE_PLUGIN(o)
(G_TYPE_CHECK_INSTANCE_CAST ((o), EOG_TYPE_SET_AS_FAVORITE_PLUGIN,
EogStatusbarDatePlugin))

Check that the defines in the Header file reference the correct data
type (change EogStatusbarDatePlugin->EogSetAsFavoritePlugin). We had
this wrong for quite some time in our plugins and can cause compiling
errors with the new API.

- In set_as_favorite_cb you do:

old_len = 0;
if (file_info != NULL) {
    old_attributes = g_file_info_get_attribute_stringv(file_info,
                                                   "metadata::emblems");
    if (old_attributes != NULL) {
        if (*old_attributes) {
            i = 0;
            while (TRUE) {
                if (old_attributes[i] == NULL ||
                    strcmp(old_attributes[i], "") == 0) {
                    break;
                }
                i++;
            }
            old_len = i;
        }
    }
}

I think this can be reduced to 

old_len = 0;
if (file_info != NULL) {
    old_attributes = g_file_info_get_attribute_stringv(file_info,
                                                   "metadata::emblems");
    if (old_attributes != NULL) {
        old_len = g_strv_length (old_attributes);
    }
}

also you do "if (*old_attributes) {" before iterating over the
attributes (it occurs at least twice). This is from my understanding
pretty much equal to "(old_attributes[0] != NULL)" which is checked by
the iteration itself. So, I think you can drop that extra check.

- Also in set_as_favorite_cb g_file_get_path is called, but the path is
not used anywhere.

- It would be nice if image_attr_favorite_i could get a slightly more
descriptive name, e.g. image_get_favourite_attr_index.

Regards,

Felix