On Tue, Apr 3, 2012 at 7:23 AM, Tom Hacohen <tom.haco...@samsung.com> wrote:
> On 03/04/12 01:53, Gustavo Sverzut Barbieri wrote:
>>
>> To clarify few things:
>>
>> - Eina_Value: it's gold, really. No idea why it was mixed into Model
>> discussion. Value purpose is not dependent on Model, although model
>> does use it to exchange properties in a safe way. When designing it
>> I've researched how glib and qt does that, we could do more with
>> optimized handling for arrays.
>
>
> As I said before, I haven't tested eina_value, I really don't know about
> this one, and that's my problem with it. I don't feel it was tested enough
> (API + code). Perhaps it's ready for prime-time, but without people actually
> playing with it, and real code in EFL actually benefiting from it, it's just
> too hard to know.
Discomfitor played with it, as far as I remember he liked its usage in esql.
The code is stupidly simple and my major concern was with memory usage
and speed. Both were addressed nicely, we can handle structures and
arrays in a fast and space efficient way.
Its purpose is clear: hold a generic typed value. Then it keeps the
value memory and the pointer to its operations. The operations were
added based on other languages and toolkits. They cover copy, compare
and convert, aside from basic set and get.
>> - Eina_Model: although test coverage is not 100%, it does come with
>> much more test than the average code in SVN. ProFUSION is using it for
>> customers code without issue, mostly to abstract data access to DB.
>> All in all I'm satisfied with the API for MODEL (DATA) usage.
>
>
> As I said, it's not just about test coverage, it's about API's validity. I
> just don't think we want to "get stuck" with this API for ages without
> testing it before.
What API? It's a model. Composed of named (string) properties with
generic values (strong typed with Eina_Value). Children is a sequence
of other Eina_Model.
The API clearly reflects that. They are clearly defined at:
http://docs.enlightenment.org/auto/eina/group__Eina__Model__Properties__Group.html
http://docs.enlightenment.org/auto/eina/group__Eina__Model__Children__Group.html
Other than that, it comes with common use cases in an easy to use way,
like filtering items, searching, sorting and iterating. All of them
based on real world use cases.
There are default implementations for these using super-generic array
+ hash, and efficient array + struct. One is able to override the
operations in order to optimize use-cases, such as talking directly
with DB, or abstract an already existing in-memory data structure.
The focus is on how to use the common model access. The properties and
children listed above. Do you see anything wrong with them?
The inheritance and all is a nice thing that came and I exposed them,
but they should not be the focus. They look good in my point of view,
nobody could spot a problem with them, but the inheritance is
secondary to the model usage as a consumer.
> What's the rush? really? We don't depend on it anywhere inside of EFL, so
> there's no real reason to rush it into our release. You can just create a
> profusion lib with the same symbol names and headers and just migrate to
> eina when it's ready and in the next release (if we find it fit then).
Indeed we did had this model thing for a while (since 2008), but the
eina version was written based on real world requirements from
scratch. Then we're not just releasing our code, we did plan it to
benefit everybody.
I can't get your points of "What's the rush?" I'm not rushing it, it's
there in SVN for a while (+2 months) and I'm just following the EFL
standards of having things on trunk for testing. Raster himself
believes this is the only way to get things tested, so I did it.
Should it go in a branch? There are not branches for other work AFAIK.
>> - during Eina_Model development, I create the Interface concept, with
>> multiple interfaces per type, and interfaces could extend multiple
>> interfaces. But the implementation (internals) assumed for MODEL
>> (data) the common case is not to have multiple INDEPENDENT interfaces
>> to implement the same, thus I implemented the interface lookups in a
>> naive way. THIS is marked in code for future improvements.
>>
>> http://trac.enlightenment.org/e/browser/trunk/eina/src/lib/eina_model.c#L524
>> Again, this is an internal issue and can be fixed without changing
>> API or ABI. The only synthetic example that exposes this behavior is
>> hard. This is example eina_model_04 in SVN, and the "fix" was to force
>> an unneeded dependency between interfaces.
>
>
> My example is not synthetic, I stumbled across it in a *real life* example I
> was implementing. And it *DOES* break API/ABI. While the functions may
> remain the same (uncertain) their behaviour is changed, and as you said
> yourself, actual usage ("force an unneeded dep") will change.
>
> Also, this is the tip of the iceberg, we don't know what else lies beneath.
Could you list the real world example? The eina_model_04 is not a real
world example of DATA model as far as I understand it. Could you
present us with such example?
Also, the "forced unneeded dep" will still work in future. It will not
be required anymore, but it will still work if that's what concerns
you.
I fail to see how it breaks API or ABI. The same code will work. Let's
say now there are undefined behaviors, that can be defined later.
POSIX libc is full of such.
>> - python, which uses the same object implementation for everything,
>> used a similar naive implementation for almost 10 years before
>> changing it.
>
>
> So are you suggesting we should lock ourself to one flawed design just
> because others have done the same in the past?
Uh?
>> - the fix for the above problem is suggested in the TODO, it's about
>> changing the algorithm that sorts the dependencies to consider initial
>> ordering as well when pulling the roots of topological sort.
>
>
> Which changes the behaviour!!!
Yes, but the existing code still works :-) You were relying on an
undefined behavior by current means. It may be defined later IMO.
All in all the code is simple to write and can be done. But should we?
>> - interfaces for models/data is more to help people to override a
>> single part without having to do much work. For example the sole usage
>> of it now is to independently handle properties and children.
>
>
> But people may have other uses (as shown in my example).
Which example? Your example tried to use model as general object
system to define hierarchy. It does not map any data into anything.
Again: model is about data.
>> - aside from interfaces exceptional corner cases, I did not see any
>> other relevant comment to Model;
>
>
> Because it was not tested enough. But as I said, there are some other things
> that are wrong in the API. I'm 100% sure I've told you about them in IRC,
> but maybe also in mail. It's still rough.
Are you serious? Really? Please review the last changes to all EFL.
From Eina to Elementary. If I would act like this with you, lots of
code would not get in past releases as many things were added in a
similar way. Do you remember binbuf? How about binshare? Many changes
to Evas text rendering. They got in, to get tested, and now they rock
and are very usable. Why can't we do the same here?
>> - I don't consider the code to be rushed in, as it's in SVN for more
>> than 2 months. It went through reviews, documentation, examples and is
>> being used in our projects without issues (the found issues were fixed
>> in SVN).
>
>
> It is rushing, I say there are flaws, you admit there are some flaws, but
> still we are trying to get it in. Time it has been in svn has little
> relevance to this, it's the amount of review it got that matters.
Oh dear. I give up. Yes, I'm not perfect, and I don't believe in this.
That's even why I marked the code as "TODO".
I'd like it to be tested more, but if you're such perfectionist that
can't get along with it... remove. No hard feelings. But at least I
expect the same treat to every other code getting in, fairness.
>> - Removing it because it's not in use is chicken-egg. It will not be
>> used if it does not exist. As I said I'm bit busy to code right now,
>> but I would use Eina_Model to implement, at least (no particular
>> order):
>> * SQL loader in esql;
>> * Eet loader and saver;
>> * Eio browser;
>> * Enjoy, allowing plugins to provide backend-independent data (SQL
>> as now, but also FS or network/last.fm).
>> * Elementary to provide views (genlist/grid variants);
>> * Python: to let Python objects be used by C code
>> implementing/using Eina_Model.
>
>
> EFL is released in sync, we can implement it in eina and add it to all of
> the above while in svn, like we do with all other features. That's hardly an
> excuse...
Ok, remove it then.
>> - I'm bit sad to see that knowing your code is not perfect and
>> flagging it with TODO to help others is seen as a bad thing. I always
>> marked like that, while others just omit those. I don't see why it
>> would make it a blocker for inclusion.
>
>
> Knowing your code is not perfect is one thing, but knowing the API is flawed
> (or potentially flawed) is another. FIXME/TODO are fine, but not when they
> refer to the API.
Could you name the so called API? Why not be specific? Then we can
discuss more specifically what to do: For instance we could 1) remove
the API; 2) mark it as unstable; 3) fix it. Many options, but right
now we're being too vague.
>> Again, this code is intended to map data in a common protocol so we
>> reduce code duplication everywhere. It is based on properties and
>> children. It does events signaling to notify of changes. It does
>> reference counting as multiple views may hold the data.
>> And for these cases I really fail to see how it's broken, badly
>> designed or with flaws.
>
>
> Like I mentioned many times before. I don't feel like enumerating the flaws
> again, nor do I remember all of them. Lets wait for other people to review
> as well and let them comment.
I'm not mind guesser... in my mind everything that was discussed in
private was cleared. The comment got in SVN to mark it as 'needs
implementing' with the suggested way. Are there any other flaws?
>> What does seems to happen is a confusion with general objects. This is
>> not general object. Don't expect Evas_Object to be converted to
>> Eina_Model because it has nothing to do. What you should expect is
>> every data provider to talk this API and suddenly you could show a
>> list of something in Elementary without effort.
>
>
> I know it's not a general object. Again, I'm not talking about whether it's
> needed or not (that's a different discussion that should be raised), but I'm
> talking about whether it should get into next release, and I think that it
> should not.
I fail to understand why are you being so picky about this sole piece
of code. There are lots of code introduced, rewritten... yet this is
the sole piece of code that is being bashed.
The way you put it looks like utterly crap and broken in unfixable
ways forever. Given that I've even looked at the API again, from a
fresh view after weeks without reading its code. Still looks good.
Really, why being so harsh with this specific code?
Given we're at this situation, I'd request and independent people to
review the code and give us your feedback. I don't want to decrease
the quality of EFL, but at the moment I truly believe I'm not doing
it. If people could be specific on such problems, it would help to
improve or decide to remove it.
Regards,
--
Gustavo Sverzut Barbieri
http://profusion.mobi embedded systems
--------------------------------------
MSN: barbi...@gmail.com
Skype: gsbarbieri
Mobile: +55 (19) 9225-2202
------------------------------------------------------------------------------
Better than sec? Nothing is better than sec when it comes to
monitoring Big Data applications. Try Boundary one-second
resolution app monitoring today. Free.
http://p.sf.net/sfu/Boundary-dev2dev
_______________________________________________
enlightenment-devel mailing list
enlightenment-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
