On Tue, Aug 9, 2011 at 2:22 PM, Gustavo Sverzut Barbieri <[email protected]> wrote: > Hi all, particularly cedric. I was to do the bindings for the new > ethumb_client_exists() API and got impressed by the number of > problems. > > - inconsistent api: void* is the first callback parameter, see > all EFL
Agreed, but I followed previous callback format of Ethumb_Client, see Ethumb_Client_Generate_Cb. I have no problem about breaking this and follow the rest of the EFL API convention. In fact, I would really like that solution as I think Ethumb API is a mess right now. > - inconsistent api: missing ethumb_CLIENT prefix, like > Ethumb_Exists should be Ethumb_Client_Exists The data type, right ? > - strange api: if I have an Ethumb_Exists handle why do I need > all the other parameters? Because Ethumb_Exists is an internal > thing exposed to user! You should return the > Ethumb_Async_Exists_Cb structure (and remove Cb from it) Maybe I should reverse the logic, have all Cb pointing to the same Ethumb_Async_Exists instance, that would solve the cancel parameter things. > - memory leak: in _ethumb_client_exists_end() you free the > async->callbacks but nothing frees 'cb' nodes. Outch and not only there, but also in cancel. > - strange behavior: you can create lots of async requests, but > if you cancel one, all of them is cancelled! Nop, only the callback is cancelled. It will wait to have no more callback before cancelling the job. > - race condition: thread may start and finish before you add the > structure to the hash: > async->thread = ecore_thread_run(...); > eina_hash_direct_add(_exists_request, async->dup, async); Yup, should check ecore_thread_run returned value. > - strange behavior: provided cb is executed even when it was > cancelled Yes, it was done that way as it's the only way to be sure that the data are not used any more. But right now, if I reverse the loginc with callback and async stuff, it should not be needed anymore as callback will not be in the list anymore. > - bad API design: there is no free_cb for exists. If you cancel > it you still get the main cb but this is not clear. This is > bad for bindings. Agreed and unecessary now in fact, as the callback is removed from the list, there is no chance to be called later. > In my opinion there should be no single-thread per ethumb_client > thing. It's a micro optimization and is useless since most users will > ask just one. I don't think so, fire elementary_test ethumb test and you will see the difference directly. Grouping command are usefull when used by gengrid as you can be very quickly asked to create one thumbnail,destroy and asked again, or some times asked twice and destroyed once. It was possible to see the difference in excessive also. Right now, the main problem of Ethumb are it's internal, to complex and really messy. Exposing Id is in my opinion a bad idea and the API is clearly a mess. This make it really difficult to fix the last issue I am facing with it, being to process one thumb per available CPU. Anyway thanks for your review, Ethumb need some love and I almost stoped my work in the middle as it worked for me... -- Cedric BAIL ------------------------------------------------------------------------------ uberSVN's rich system and user administration capabilities and model configuration take the hassle out of deploying and managing Subversion and the tools developers use with it. Learn more about uberSVN and get a free download at: http://p.sf.net/sfu/wandisco-dev2dev _______________________________________________ enlightenment-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
