On Wed, Jul 28, 2010 at 8:23 PM, Stephen Houston <unixti...@gmail.com>wrote:
>
>
> On Wed, Jul 28, 2010 at 8:18 PM, Gustavo Sverzut Barbieri <
> barbi...@profusion.mobi> wrote:
>
>> On Wed, Jul 28, 2010 at 10:05 PM, Stephen Houston <unixti...@gmail.com>
>> wrote:
>> > On Wed, Jul 28, 2010 at 6:33 PM, Gustavo Sverzut Barbieri
>> > <barbi...@profusion.mobi> wrote:
>> >>
>> >> On Wed, Jul 28, 2010 at 7:12 PM, Enlightenment SVN
>> >> <no-re...@enlightenment.org> wrote:
>> >> > Log:
>> >> > Add the ability to perform standard IO operations on a file in a
>> >> > thread.
>> >> > +#define EIO_FILE_MOD_TIME 1
>> >> > +#define EIO_FILE_SIZE 2
>> >> > +#define EIO_FILE_EXISTS 4
>> >> > +#define EIO_FILE_IS_DIR 8
>> >> > +#define EIO_FILE_CAN_READ 16
>> >> > +#define EIO_FILE_CAN_WRITE 32
>> >> > +#define EIO_FILE_CAN_EXECUTE 64
>> >>
>> >> You should provide some EIO_FILE_STAT, to return the whole stat
>> >> result, maybe that is more useful than doing it separated for size,
>> >> exists... you can leave what you did, but also provide the generic
>> >> function as people will consult more than one property at time.
>> >
>> > I left that out because you can specify as many of the operations as you
>> > want anyway.
>>
>> You force people to define many callbacks, or one with a big switch
>> that is called multiple times. Still you don't provide other stat
>> information that may be useful, like owner, mode...
>>
>> Right. What I put in now just covers what was in ecore_file. With it
> being the flag style, it is obviously intended to grow in capabilities :)
> I think a full stat flag is reasonable and I will add it.
>
>>
>> >> Also, why not make it an enum?
>> >
>> > It was an enum originally, but cedric thought bit flags would be better.
>>
>> BAD cedric ;-)
>>
>
> I tend to agree with Cedric though. Enums are intended to be used as
> states or single options. Bit flags are better for specifying multiple
> flags.
>
>>
>> In fact after talking with several people in #edevelop, bit flags seem to
be the consensus.
>
>>
>> >> > +static void
>> >> > +_eio_file_op_end(void *data)
>> >> > +{
>> >> > + Eio_File_Op *async;
>> >> > +
>> >> > + async = data;
>> >>
>> >> Just do Eio_File_Op *async = data, it is smaller and makes sense as
>> >> you're just converting the parameter.
>> >>
>> > Ahhh the E formatting... :(
>>
>> strictly it is not e formatting, raster uses what you did, but it just
>> adds lines to code, and leads to easier "unused variable" errors for
>> no good.
>>
>>
>>
>> >> > + if (!async->exists)
>> >> > + {
>> >> > + if (async->flags & EIO_FILE_EXISTS == EIO_FILE_EXISTS)
>> >> > + async->main_cb((void *)EINA_FALSE, EIO_FILE_EXISTS, data);
>> >> > + ecore_thread_cancel(async->common.thread);
>> >> > + return;
>> >> > + }
>> >>
>> >> I don't like this, you don't return anything for CAN_READ, CAN_WRITE
>> >> and others in the case it does not exists? Not even an error? IMO
>> >> the user should always get called back, with an error whenever
>> >> appropriate. Or call error_cb if there were requirement not fulfilled.
>> >>
>> >> also a bug there: the last parameter is USER data, not the one you got
>> >> from the callback ("async"), you really want: async->common.data
>> >> instead!
>> >>
>> > Actually CAN_READ CAN_WRITE and CAN_EXECUTE will return error if the
>> file
>> > doesn't exist (see if (!async->exists)). Otherwise is EINA_TRUE or
>> > EINA_FALSE as to whether it can do those. Good catch on the bug.
>>
>> I fail to see how they will return error, if the file did not exist,
>> then it will report for EIO_FILE_EXISTS and return :-/
>>
>>
>>
>>
>> >> > + if (async->flags & EIO_FILE_EXISTS == EIO_FILE_EXISTS)
>> >> > + async->main_cb((void *)EINA_TRUE, EIO_FILE_EXISTS, data);
>> >>
>> >> this is ugly and took me a while to notice why you did the EINA_TRUE
>> >> constant here... it was because of the other test before. And really,
>> >> why not call this one first?
>> >>
>> > I'm not sure what you are getting at here? It returns FALSE if it
>> doesn't
>> > exists, and TRUE if it does. The reason the FALSE is called first is
>> > because the app will seg if any other operations are done on a file that
>> > doesn't exists.
>>
>> I did notice that, but it took some time as at first it looked like a
>> bug "he should use the variable and not a constant!", then I noticed
>> the other case was handled before.
>>
>> what I meant to call this one first, is that we should dispatch the
>> EIO_FILE_EXISTS before dispatching CAN_READ, CAN_WRITE...
>>
>>
>>
>> --
>> Gustavo Sverzut Barbieri
>> http://profusion.mobi embedded systems
>> --------------------------------------
>> MSN: barbi...@gmail.com
>> Skype: gsbarbieri
>> Mobile: +55 (19) 9225-2202
>>
>
>
------------------------------------------------------------------------------
The Palm PDK Hot Apps Program offers developers who use the
Plug-In Development Kit to bring their C/C++ apps to Palm for a share
of $1 Million in cash or HP Products. Visit us here for more details:
http://p.sf.net/sfu/dev2dev-palm
_______________________________________________
enlightenment-devel mailing list
enlightenment-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
