On 10/18/2011 02:25 PM, Endi Sukma Dewata wrote:
On 10/18/2011 10:52 AM, Petr Vobornik wrote:
> 3. Another goal is to replace entity names used in spec (see
> other_entity & nested_entity spec properties) with the actual entity
> objects. In this case it might be better to use the loops described in
> #2. This can be done separately.

Wouldn't it lead to the circular dependancy problem again? I think using
entity names and calling IPA.get_entity at the time when it is needed is
fine. But we should make some naming conversions of function params or
object properties to distinguish when we are working with just name or
entity itself.

It shouldn't. The circular dependency happens when we create an entity that has a facet that depends on another entity, and that entity also has a facet that depends on the first entity, but the first entity is still being created. If we create all entities first separately, by the time we create the facets all entities are already created.

Agreed on the parameter naming.

> 5. In entity.js:201, the use of entity.title for the breadcrumb tooltip > might not be appropriate because usually the title is plural whereas the
> breadcrumb points to a single object. It would be better to use the
> entity.metadata.label_singular.

Fixed

The first and last elements in the breadcrumb actually don't have the tooltip set so it will show a default tooltip which might not be correct. This is an existing issue so we can fix this separately.

> 8. The facet_specs and dialog_specs lists can be replaced with
> ordered_map. It already has a method to find an element by its name.
> This can be done separately.

My intention was to be able specify entity facets and dialogs simply by
specifying their spec in entity spec (without calling builder function),
this is possible now. I didn't want to add initialization logic for
facet_specs (to fill the ordered_map) to entity in order to keep it as
simple as possible. It would be nice though. Maybe we could make an
utility object with methods for some simple initialization logic which
is used on many places - like checking if boolean is set or some more
complicated like initialization of ordered_map (needs key selector fn as
parameter).

I didn't mean to replace the spec with a class. I was referring to using the ordered_map to replace the plain array (i.e. facet_specs and dialog_specs) that contains the specs because the ordered_map already has a method to lookup the elements by their name. So instead of:

  entity.facet_specs.push(spec);
  var facet = get_spec_by_name(entity.facet_specs, association_name);

it will be

  entity.facet_specs.put(spec.name, spec);
  var facet = entity.facet_specs.get(association_name);

This is not crucial, we can always improve it later.

ACK and pushed to master.

Nice work. I am not 100% satisfied with the solution, but that is due to limitations in the Javascript language. I think this is the best we can get with the tools we have today.

A more perfect solution would allow us to use a lazy load proxy, and to hide from an end component that the object is a proxy. the getters code we had a while back sort of gave us that, but as we found, browser specific.

Ideally, when we set a Facet on an entity, that would be a lazy load proxy, as would be the case where we explicitly link one entity up with another.




_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to