[Freeipa-devel] [PATCH] 0270-removing-setters-setup-and-init

[Endi Sukma Dewata]

On 7/22/2011 12:18 PM, Endi Sukma Dewata wrote:
> On 7/22/2011 10:34 AM, Adam Young wrote:
>> While this version has a Navigation fix in it, it is going to be rebased
>> on top of Endi Dewata's patch for putting the state inside each of the
>> facets.
>
> Rebased. I'll continue the review.

Some issues:

30. The IPA.spacer_widget is used to create spacing for layout. Widgets 
should not be used this way because it diverges from the original 
design. Widgets are supposed to correspond to attribute. It has a name 
(required for storing in a map), metadata, load/save/undo methods, none 
of which is relevant for spacer. Also, it only handles a narrow case for 
creating a space between 2 fields which I think better be handled using 
sections. This should be addressed in a separate patch.

31. The create() should only be used to create the visual elements. 
Creating fields, columns, etc. should not be done inside create() 
because we might decide to destroy the HTML elements of hidden facets 
and recreate it again. In that case the fields will be duplicated. Also, 
by delaying the creation of the fields it makes the object incomplete 
after creation, which is the reason for removing init().

32. This is an existing problem but it's worsening without init(). Since 
there is no class constructor, the initialization code is scattered 
throughout the class, making it difficult to maintain. Some code needs 
to be written in certain position, but in general should we move the 
initialization code to the bottom of the class (to make sure all methods 
are already defined)? Or should init() be used as a constructor and 
called at the bottom of the class?

33. In association.js:661 the spec object is referenced without ensuring 
it's not null. The code below it supposed to do that.

34. Commented code in entity.js:746 can be removed.

35. Commented code in widget_tests.js:181 can be removed.

36. The widget_create() is called twice in widget.js:1597.

37. Untranslated True/False labels in HBAC and sudo.

38. The file install/ui/rule.js~ got included.

39. There are jslint warnings.

40. The selenium tests could be fixed in separate patch and probably 
pushed earlier. I have not run them.

-- 
Endi S. Dewata