[Freeipa-devel] Nesting widgets

[Petr Vobornik]

Replying for the third mail of original thread (its content is on the 
end of this mail)) as it brought some thoughts.

I like the idea of composite pattern.

As I'm working on #1515 (Sudo, HBAC refactoring) I used this concept 
because sudo rule details facet uses a lot of code repetition to 
implement minor changes.

Attaching a WIP patch for with these changes:

Transforming details_section to composite_widget. The term section 
remained and sections are considered as top level widgets which serves 
for for conceptual separation in details facet(headers, folding...)

I introduced there a concept of update_info. It's an object containing 
changes in widgets. At the moment update_info consists of changes in 
fields - array of field_info  (corresponds to save(record) method) and 
array of commands. Changes in fields are used for classic entity-mod 
operation. The commands can be used for specific operations like 
enabling rules, deleting members etc. In order to use it I have created 
a new implementation of details_facet (needs to be named). The old one 
remains to be backward compatible.
  During update method call parents widget are responsible for obtaining 
and returning changes of their children or they can modify it and return 
some specific commands. New details facet execute it in a single or 
batch command (depending on the presence of fields, commands or both).

In the patch is an example how it can benefit - refactored Sudo Rules 
details facet.

There is also a slight modification of save(record) method which allows 
to fill record even from composite widgets.

Original thread:
On 10/04/2011 03:34 PM, Adam Young wrote:
 > Great input Petr, thanks for the thoughtful comments. Comments inline
 >
 >
 > On 10/04/2011 07:56 AM, Petr Vobornik wrote:
 >> On 09/29/2011 02:38 AM, Adam Young wrote:
 >>> So I decided to try to get an IP Address widget working. See the
 >>> attached patch. It was fairly trivial.
 >>>
 >>> However, this widget is not really all that useful by itself. It would
 >>> need to work as a part of a multivalued_text widget in order to replace
 >>> the widget used on the dnsrecord page. And looking at the multivalued
 >>> text widget, I think you will agree that is going to be tricky.
 >>
 >> I think we can create an extend point for validation logic (EG
 >> validator object in field's spec) instead of inheriting the widget.
 >
 > Interesting concept. Yes, this would work too, and solve the issue for
 > IP addresses validation. But there are places where we may want to
 > validate only a single IP address, and we may end up with code
 > duplication, although more likely they will both just call the same
 > utility function.
 >
 >>
 >>>
 >>> The problem is that we don't really have the API set up for nesting.
 >>> This has burnt us on the Sections as well as making more complex
 >>> widgets. Usually, instead of reusing the widgets we have, it is easier
 >>> to go straight to the HTML.
 >>>
 >>> I think the crux comes from the 1-1 mapping between widgets and fields
 >>> of the request/result JSON. For widget it is: load(record) and the load
 >>> command knows how to find the value it needs inside the record. For
 >>> save, on the other hand, it just returns the values it needs.
 >>>
 >>>
 >>> In order to make the widget scheme more nestable, the section
 >>> section.load and save can do more work, such as scoping down the piece
 >>> of the request record to just that portion required by the widget.
 >>> Bascially, it can do what widget.load does, just externally
 >>
 >> I don't think this would help. This would limit the widget. Currently
 >> most widget works with the record.[widget name]. But we can override
 >> load and save method to break the 1:1 mapping between widget and
 >> record. Widget can consist of several widgets and supply them custom
 >> record object (it can assume that the simple widget would fetch the
 >> value of its name. The name is defined by the master widget (no
 >> problem here).)
 >>
 >> The concept you mentioned could be beneficial if we abandon flat
 >> records and introduce structured ones. But I think there is no will
 >> for it. Event then I don't know if sections should be responsible for
 >> this. It's not their purpose.
 >
 > Records are already structured, just that by the time we get to the
 > individual fields, they tend to be flat. But the record is JSON and is a
 > tree structure.
 >
 > But I like what you are saying. I agree that the interface for Widget
 > and Section should be the same. Right now Section is:
 > that.save = function(record) {};
 > that.load = function(record) {);
 >
 > and load is
 > that.load = function(record) {};
 > that.save = function() {};
 >
 >
 > It is this last function that needs to be changed, but it will have far
 > reaching effects. We use save to extract the value or values from the
 > field for many uses. I think we can do this: For all widgets, rename
 > save() to get_value(); and then add a save(record) method that calls
 > get_value(); Then widget and record have the same interface, which is a
 > big first step.
 >
 >
 >>
 >> Another (maybe bigger) problem is layout. For multivalued textbox with
 >> IP address widget one of the problems can be the placement (or even
 >> behaviour) of 'undo' and 'error-link'. Possible solution could be:
 >> specifying container for these parts by its container - the containing
 >> widget could control where they would be rendered. Another one can be
 >> API notification of the state change (no visual by the widget itself)
 >> and handling it by the parent widget.
 >>
 >> For more layout demanding widgets section layout can be limiting (like
 >> in host adder dialog which force to implement custom section instead
 >> of widget). Which in my opinion is bad, but we don't have any other
 >> solution for this yet. I think there is a ticket for it.
 >
 > Perhaps for more complex widgets, we can extract the layout into its own
 > object, and pass it to the constructore. We'll provide one by default,
 > but allow overloading.
 >
 >
 >>
 >>>
 >>>
 >>> var value = record[that.name];
 >>> if (value instanceof Array) {
 >>> widget.values = value;
 >>> } else {
 >>> widget.values = value ? [value] : [];
 >>> }
 >>>
 >>> widget.writable = true;
 >>>
 >>> if (widget.param_info) {
 >>> if (widget.param_info.primary_key) {
 >>> widget.writable = false;
 >>> }
 >>>
 >>> if (widget.param_info.flags && 'no_update' in that.param_info.flags) {
 >>> widget.writable = false;
 >>> }
 >>> }
 >>>
 >>> if (widget.record.attributelevelrights) {
 >>> var rights = that.record.attributelevelrights[that.name];
 >>> if (!rights || rights.indexOf('w') < 0) {
 >>> widget.writable = false;
 >>> }
 >>> }
 >>>
 >>>
 >>> This seems to break encapsulation, but I think it might be the right
 >>> level of abstraction. We can make this code reusable by other sections
 >>> by calling it something like section.widget_load.
 >>>
 >>> load then becomes nothing more than a call to reset for most widgets.
 >>> But, for the widgets that need nesting, or that need additional
 >>> rendering logic (like select) we still have a place for them to over
 >>> ride it. I think it is telling that most of the overridden load
 >>> functions still cal the parent function first. I thik that is an
 >>> indicator that what we have can be refactored.
 >>
 >>>
 >>>
 >>> Looking through widget.js, I'd say that most of the widgets can be
 >>> safely redone this way. Text_area.load looks suspect, it should probbly
 >>> be using the load functionality from the base class. table_widget will
 >>> need some thought, but I think it will work is the value is set as an
 >>> array, and then the code is called.
 >>>
 >>> if (that.values) {
 >>> for (var i=0; i<that.values.length; i++) {
 >>> var record = that.get_record(result, i);
 >>> that.add_record(record);
 >>> }
 >>> }
 >>> that.unselect_all();
 >>>
 >>>
 >>> I think that the save functions be OK as is. the multivalue save should
 >>> be able to work with the result of calling sae from each individual 
text
 >>> area.
 >> Agree
 >>>
 >>>
 >>> So where would this help us. DNS records is probably the big one. With
 >>> users, we might have to store multiple keys or certificates. We don't
 >>> currently validate email addresses, and we could there. It would
 >>> probably simplify the code for ACIs, but there really is no reason to
 >>> put effort in to reworking that for its own sake.
 >>>
 >>>
 >>>
 >>> The other possibility is that we could redo the multivalue widget as a
 >>> section. I know we've done that else where. On the user page, the Email
 >>> addresses and phone numbers would then each go into their own
 >>> section....or we leave the multivlue widget there, but apply this
 >>> approach else where. For DNS records, this means that each record type
 >>> would basically become its own section. I don't favor this 
approach, but
 >>> I could buy the argument that redoing the widget API is too much work.
 >>
 >> What is the purpose of sections? It seems to me, that the main idea
 >> was to separate fields by their meaning with appropriate header. Not
 >> to define custom layouts for individual fields.
 >>
 >>
 > What you are seeing is the evolution of the concept. Section was
 > originally the grouping mechanism we used for the User page to allow us
 > to collapse and expand a group of fields together. We hijacked the
 > concept in the ACI Permissions pages when we needed a way to group the
 > set of fields based on the permission type. The major difference in
 > abstractions is that Widget is a single field out of the record, and a
 > section is agroup of fields. That split is, I think what is holding us
 > back. I thin we can make Section a widget, and create a scheme for
 > nesting widgets to get to an implementation of the composite pattern,
 > and have a much more usable abstraction.

-- 
Petr Vobornik
-------------- next part --------------
A non-text attachment was scrubbed...
Name: wip-freeipa-pvoborni-0002-WIP-Code-cleanup-of-HBAC-Sudo-rules.patch
Type: text/x-patch
Size: 63072 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20111020/d372db0c/attachment.bin>