I'm afraid that the multiple-backend design is overly-engineered.
Though it looks much more flexible, there is no real-world use cases
since we don't mix multiple image libraries in one GUI project. For
example, you won't use both of QImage and GdkPixbuf in a Qt project,
and vice versa. I'm not against making things flexible, but if there
are no valid real-world use case, I prefer KISS.
For those who really need to use other formats, here are 2 options:
1. Write a backend for GdkPixbuf or Qt/QImage, so all other gtk+ or Qt
programs can use it, not just the file manager. (This one is the best
because the whole desktop environment can use it, and the work needs
to be done is not significantly more than making it work with libfm
only)
2. Do type checking in backend functions yourself like this:
GObject* scale_image(GObject* source, ...) {
GType src_type = G_OBJECT_TYPE(source);
if(src_type == GDK_TYPE_PIXBUF) {
}
else if(src_type == OTHER_TYPE) {
}
return NULL;
}
In this way, programs which really need so complicated design can
still use multiple formats. The type checking is done in the backend
functions by themselves, not libfm.
I think this is more than enough.
The idea behind this design is, libfm does not need to know the format
of the image objects. To libfm, they're only memory blocks with
reference-counting. That's all. Libfm is only responsible for locating
the thumbnails and reading the image file stream. It handles I/O and
caching, but knows nothing about the implementation details of the GUI
parts.
The code in fm-thumbnail-loader.c is already messy, and I really think
that KISS is better. We don't need to mess with the implementation
details in the GUI libraries.
When I designed libfm, it's not intended to be used with mixing
different GUI toolkits in one single application. Most of the time we
only use one GUI toolkit at a time and your GUI toolkit normally only
recognizes one image format, so I think one backend is enough. If
multiple formats is required, type checking can be done inside backend
functions.
Thanks.
On Sun, Mar 17, 2013 at 8:17 AM, Andrej N. Gritsenko <[email protected]> wrote:
> 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.
>
> ------------------------------------------------------------------------------
> Everyone hates slow websites. So do we.
> Make your web apps faster with AppDynamics
> Download AppDynamics Lite for free today:
> http://p.sf.net/sfu/appdyn_d2d_mar
> _______________________________________________
> Pcmanfm-develop mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/pcmanfm-develop
------------------------------------------------------------------------------
Everyone hates slow websites. So do we.
Make your web apps faster with AppDynamics
Download AppDynamics Lite for free today:
http://p.sf.net/sfu/appdyn_d2d_mar
_______________________________________________
Pcmanfm-develop mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/pcmanfm-develop
