Re: [Pcmanfm-develop] thumbnail branch of libfm - call for review

[Andrej N. G. <an...@re...>]

    Hello!

PCMan has written on Friday, 15 March, at 16:12:
>Hello,
>I just finished the rework of thumbnail loading for libfm.
>The code is in thumbnails branch of libfm repo.
>git://pcmanfm.git.sourceforge.net/gitroot/pcmanfm/libfm-qt

>Things changed:
>1. Thumbnail loading code is moved from libfm-gtk to libfm and becomes
>GUI toolkit independent
>2. The thumbnails branch of pcmanfm-qt can utilize the new code to
>load thumbnails in Qt now.

>If there are no big problems, I'd suggest merge the changes to master branch.

    That change seems good. There is a problem with it though - it is
never scalable despite of being library call. In old thumbnail code there
was only one image format and it was unchangeable. New code introduces
setup for format but format still left unchangeable! That is invalid
design since that static data can be accidentally overwritten. To avoid
this problem calls below should be changed:

FmThumbnailResult* fm_thumbnail_loader_load(FmFileInfo* src_file,
                                            guint size,
                                            FmThumbnailResultCallback callback,
                                            gpointer user_data);

should become

FmThumbnailResult* fm_thumbnail_loader_load(FmFileInfo* src_file,
                                            GType type, guint size,
                                            FmThumbnailResultCallback callback,
                                            gpointer user_data);

and

void fm_thumbnail_loader_set_backend(ThumbnailLoaderBackend* _backend);

should become

void fm_thumbnail_loader_set_backend(GType type, ThumbnailLoaderBackend* _backend);

and therefore we can support more than one image type. And instead of

static ThumbnailLoaderBackend backend = {0};

there should be

static GSList *backends = NULL;

along with few tiny changes in places which are related to that static
backend data. And also GType member should be added into ThumbnailTask
structure due to that change.

    And I also think fm_thumbnail_result_* isn't good name for the API.
Result is an item literally and fm_thumbnail_result_cancel() sounds very
odd. How can we cancel result? We can cancel task, request, whatever is a
process, only process is cancellable, not item which we got. Therefore I
think there should be better name for this API family. What about, for
example, fm_thumbnail_loader_*? That should look better I believe, and
also it unifies all names (part of them are fm_thumbnail_loader_* now but
part are fm_thumbnail_result_*).

    What do you think about it?

    Andriy.