Review of the thumbnailer spec

[Philip Van Hoof]

On Fri, 2009-05-15 at 04:00 +0200, Jannis Pohlmann wrote:

First of all, thanks Jannis for doing this review. The issues you
mention are really helpful for improving the specification.

[CUT]

> In theory, this should completely hide how thumbnails are stored. For
> instance, it doesn't really matter whether a cache is used or not. It
> doesn't even matter where the thumbnails are stored.

That is correct, except of course that a consumer will need to know
about the thumbnail-spec (the storage one) to know where he can find the
thumbnails when they are ready. Or where he can find the failures.

It's however the case that for our use-case (Nokia's devices) we are
only interested in cropped, not large nor normal, and that we want to
store thumbnails as JPeg files, not PNG. 

So we believe that the storage specification for thumbnails isn't
complete or rather isn't flexible enough.

> However, the interface has a number of methods related to modify the
> thumbnail cache specified in the thumbnail managing spec [1]. These are
> Move, Copy, Delete and Cleanup. Now the question is, do we want to move
> these into a separate (possibly optional) interface like
> o.fd.thumbnailer.CacheManager? 

I think we want this, yes.

I'm one of those crazy people who believe that any interface, a DBus or
a compile-time interface, that doesn't matter, should only describe
exactly one thing.

-> A thumbnailer is a thing that services creating thumbnails 

-> It's not a thing that services managing thumbnails. That's another
   thing.

My conclusion is like yours that a split of the interface into two
things is  wanted.

If an implementer wants to make only one class (one service) doing both,
the implementer can simply implement both DBus interfaces. This doesn't
make it more complicated.

I'm sure, though, that there are people here who believe that an inter-
face should contain the solution for all of the world's problems. I call
that a facade, and it's something the programmer using the service must
make. Not something the service must be.

[CUT]

(in agreement)

> Of course this raises the question how the interfaces should be named.
> Should it be org.freedesktop.thumbnailer.* or
> org.freedesktop.thumbnails.*, or even a mixture of both? Should it be
> org.freedesktop.thumbnailer.Cache or .CacheManager? 
>   Also, the name .Generic is rather undescriptive. I'm pretty sure we
> there's a better name for this. Let me throw .Provider, .Service
> or .Delegate in the pot as a few of my first ideas.


How about :

org.freedesktop.thumbnails.Thumbnailer
	- Queue
	- Unqueue
	- Ready
	- Error signal
	- Finished signal
	- Started signal

org.freedesktop.thumbnails.Cache
	- Move
	- Copy
	- Cleanup

org.freedesktop.thumbnails.Manager
	- Register
	- GetSupported


> I think we should improve the description of Unqueue a little bit.

I agree.

There's a confusion about it among people that Unqueue can't cancel a
currently running thumbnail algorithm. In fact it can.

The spec allows, and this is why it has Started and Finished, to
preemptive stop a running Queued task. A queued task, as you know, can
contain more than one item. But also the current item can be stopped, if
the algorithm for thumbnailing permits this.

It simply means that also the currently running item never has a Ready
signal emitted.

Immediately after a Finished signal must be emitted. 

The consumer can detect the cancel by simply counting the amount of
items his task had, versus the amount of Error and Ready items he has
received before this Finished happened.

And that also explains why I added the Finished signal.

Meaning that Unqueue is allowed to cancel.

And whoever has been proposing to add a Cancel method, didn't fully
understand the power that Unqueue plus a Finished signal yields.

It means that you aren't strictly required to implement actual and
immediate cancellation of the thumbnailing algorithm. But it allows you
to do this. If you can implement it for your thumbnailing algorithm.

If not, you just finish your current item, and immediately after that
you emit Finished, and you don't handle the rest of the task. Done.

> Unqueue doesn't really prevent service-side implementations of unqueing
> batch tasks (more than one URI queued as one handle) in the middle of
> the task 

[CUT]

Indeed

> Another thing I noticed is that timestamps passed over D-Bus (e.g. the
> "since" argument to org.freedesktop.thumbnailer.Generic.Cleanup) are 32
> bit unsigned integers ("u") while they should really be 64 bit uints
> ("x"). Philip told me the reason to use "u" was that some D-Bus
> versions didn't handle "x" properly. On the long run there's no way
> around 64 bit ints though, I guess. 

You're right, this should definitely be x

> Last but not least, the spec is not an official fd.o spec yet. Once my
> implementation is ready we'll have at least two implementations used in
> two different environments (Hildon and Xfce). If Thunar, the Xfce file
> manager, turns out to work well with my implementation I don't see a
> reason why we wouldn't want to advertise the spec a bit more - e.g. by
> making it official on fd.o.

I'd agree with that.

Here's my official request:

http://bugs.freedesktop.org/show_bug.cgi?id=17376


-- 
Philip Van Hoof, freelance software developer
home: me at pvanhoof dot be 
gnome: pvanhoof at gnome dot org 
http://pvanhoof.be/blog
http://codeminded.be