On 05/08/2012 01:47 AM, Endi Sukma Dewata wrote:
The code works, it's ACKed.
Patches 124-132 pushed to master.
I have some comments below:
I'll address some points in separate patches.
On 5/2/2012 8:33 AM, Petr Vobornik wrote:
This bunch of patches are implementing ticket #2247. They introduce some
new logic and types of internal objects. There might be design issues
(mainly in state evaluation). I would appreciate some opinions on what
might be improved.
See patch comments for more details.
What I think might be the main concerns:
Action list definition:
Now action lists are defined on facet level and facet header takes them
from facet. Would in be better to define action list - the widget and
actions separately. The widget could be defined in header and actions on
facet level.
Yes, the header could be considered a widget, so it contain the action
list widget.
State evaluation:
The patches are adding support for some kind of state evaluation. The
state is represented by array of string. I'm thinking that the design
might not be robust. Is a string good enough? We might have a problem
with conditions like to "have this particular access right' (#2318).
There are state_evaluators and state_listeners. They are practically the
same but the execution point and parameters are different. Should they
be somehow joined?
It might be better to use a map to represent the state. The map could
hold any attribute types, not just string flag with the current array.
So, the widget itself could be the state because it's also a map. That
said, evaluating a map might be difficult to define declaratively, we
would end up using a code again. So for now using an array of string is
fine, but we just have to know the limitations.
Code placement:
There's a lot of new objects and for some of them it is not clear to
what code file they should be placed.
This will continue to change as we add more code. Feel free to create a
new file when you see a pattern. We might want to start separating the
IPA-specific code from the framework (reusable code). The framework
could be moved into a 'lib' folder.
FYI: In close future I would like to address some problems in UI
architecture. I'm in a middle of designing phase, so there is nothing to
present at the moment. The main topics are:
* reduce the need of overriding methods when a new widgets or
capabilities are added
* make it more declarative to enhance extendibility
It may be done by:
* better inheriting model to support events
* build phases (preinit, init, postinit, create, load) to improve spec
object creating and initialization of created object.
* path representation of an event/attribute/model property to support
bindings to various events/attrs from anywhere
* introduce model and model bindings, converters between command
output/model/human readable representation
Sounds good! Some comments about the patch:
1. When you open a specific user, there's a default action that's
already selected in the action list. The thing is the current default
action (i.e. Disable) is probably not something that's regularly used.
It might be better to show something like '-- Select Action --' or '--
Action List --' so you'd have to select an action first then click Apply.
2. Suppose we did #1, do we still need the confirmation when applying
the action?
We don't.
3. I think the action list should be available in all details page for
all entities. At least it should have the Delete action.
#1,#2,#3: Sounds reasonable, will do.
Delete button can be also created as control button and we have ticket
for it. I'll do it as action in action list, though.
4. Something to think about, do we need an action list in the
association page? Entities like group default to an association page
instead of details page. So if we don't have an action list in the
association page the UI may look inconsistent because you'd have to go
to the Settings to execute an action. But if we do have an action list
in the association page, the actions could become confusing: do they
apply to the associations or to the entry itself? One possible solution
is to move the Settings to the left-most position and make it the
default page and have the action list in the details page only.
I don't think there is an optimal solution. For now I prefer current
implementation. I agree with your thoughts.
Reasons why would I leave it as is:
* if association facet is the first facet it usually means that the
details facet is less important and so the actions are not so important
(frequently used) to be offered in assoc. facet. User can always switch
to settings facet. Nice example is dnszone entity or other nested facets.
* I would show action list in association facets only for actions
related to associations even though the association is related to the
object.
5. Some details pages have status indicator (check sign or minus sign)
on the left of the page title, some others don't. This is because not
all entities have a concept of status. Would it be better to show the
status as a read-only field in the facet content?
I just want to show read only field as an addition to the icon or to
replace the icon?
I have nothing against showing it as an addition.
6. It might be better to avoid using element ID in the CSS. This would
make the CSS more reusable.
#content .facet-action-list div[name=apply] a.ui-state-default {
...
}
I'll try to avoid it. I'm also thinking about using OOCSS principals.
They seems very useful for the kind of application which IPA Web UI is.
7. In some facet class definitions the no_init parameter is defined
separately from the spec object, any particular reason?
IPA.facet = function(spec, no_init) {
...
};
You can think of spec object as named parameters. So the no_init can be
defined in the spec object and later used to determine what operations
to be done inside the init method.
It is to suppress initialization of ancestor classed before the process
of inheriting is completed.
If I would alter the spec it one class I would have to remember the
state before calling ancestor constructor because offspring classes
wouldn't be able to suppress this class init method.
Example where current class wants to disable init method in ancestors
builder function:
var no_init = spec.no_init; //remember offspring will
spec.no_init = true; //disable init in ancestor - IPA.facet
var that = IPA.facet(spec);
if (!no_init) that.init(); //my init
Current implementation seemed easier.
I already have a concept which can replace this.
http://pvoborni.fedorapeople.org/patches/wip-freeipa-pvoborni-0123-IFW-base-classes-builders.patch
http://pvoborni.fedorapeople.org/patches/wip-freeipa-pvoborni-0124-Events.patch
8. The declarative control button definitions still contain some code,
e.g. the handler function inside the button actions. Maybe the actions
should be defined in a separate list and the buttons can refer to them
by name.
This separation sounds interesting. We will be able to define some list
of actions which applies to that object and then action_list,
control_buttons and action_panels can pick whatever they want.
But the code you are referring to is just a shortcut to safe some lines
of code - lets say anonymous action.
9. The 'control_buttons' attribute in search facet definitions contains
a 'buttons' array. Any plans to create custom control buttons widget? If
not maybe the 'control_buttons' itself could be the array.
No plans but in 'control_buttons' definition can be also added
state_listeners.
10. The 'Action List' in ticket #2248 for the password reset is actually
different than the action list for Enable/Disable. I was proposing to
create a small panel under the 'Account Settings' section, probably
something like this:
---------------------------------------------------------------
Account Settings
+-----------------+
User login: admin | Actions: |
Password: | Reset password |
Password expiration: +-----------------+
UID:
GID:
This panel might better be called 'Action Panel' to distinguish from the
dropdown 'Action List' on the top. The reason for this panel is that a
Password Reset only affects an aspect of the user, not the entire object
like Enable/Disable (although that can also be argued differently), so
the action should be placed where it's relevant, in this case near the
Password field. The same concept will be used for ticket #2250, #2251,
#2252.
I'll consider my patch #133 NACKed and first create the action panels.
--
Petr Vobornik
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
