You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@click.apache.org by "James P Brown (JIRA)" <ji...@apache.org> on 2009/02/27 23:42:56 UTC
[JIRA] Created: (CLK-497) FieldSet isDisabled and isReadonly
methods broken
FieldSet isDisabled and isReadonly methods broken
-------------------------------------------------
Key: CLK-497
URL: http://issues.apache.org/click/browse/CLK-497
Project: Click
Issue Type: Bug
Components: core
Affects Versions: 2.0.1
Environment: Windows XP, Java 6
Reporter: James P Brown
The FieldSet is supposed to force its child components to be disabled/readonly when it is set to disabled/readonly. I did not observe this when I attempted to create a FieldSet with a child component.
I believe the code to support this is not working as anticipated. The Field class has modified methods for isDisabled/isReadonly that specifically check if the parent component (i.e. container) is an instanceof FieldSet (or Form, which is working AFAIK). The problem is that the design of FieldSet relies on an instance of its private inner class FieldSet.InnerContainerField for managing those child elements. When I step through the code in debug mode, the class instance is of this inner class type (InnerContainerField) not FieldSet. Since InnerContainerField is not a type of FieldSet, the subsequent logic is ignored.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[JIRA] Commented: (CLK-497) FieldSet isDisabled and isReadonly
methods broken
Posted by "James P Brown (JIRA)" <ji...@apache.org>.
[ http://issues.apache.org/click/browse/CLK-497?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=11715#action_11715 ]
James P Brown commented on CLK-497:
-----------------------------------
Thank you for the prompt reply, again. I can respect the view that you would not want to add more to the framework than is necessary.
However, I'm not sure I see how changing the behaviour of setDisabled/setReadonly would make this use case work. As I see it, it is the behaviour of the Field isDisabled/isReadonly methods that determine this cascading behaviour. Children ask for the parent component, check their type, decide if they are Fieldset or Form, and then ask their state. My interpretation is that this would require changing the Field code, which I thought would be hard to manage and maintain, or derive new types from all of the existing components, which seemed redundant.
For that reason, I suggested a change at the framework level.
Regards,
James
> FieldSet isDisabled and isReadonly methods broken
> -------------------------------------------------
>
> Key: CLK-497
> URL: http://issues.apache.org/click/browse/CLK-497
> Project: Click
> Issue Type: Bug
> Components: core
> Affects Versions: 2.0.1
> Environment: Windows XP, Java 6
> Reporter: James P Brown
> Attachments: FieldSet_isDisabled_isReadonly_bug.zip
>
>
> The FieldSet is supposed to force its child components to be disabled/readonly when it is set to disabled/readonly. I did not observe this when I attempted to create a FieldSet with a child component.
> I believe the code to support this is not working as anticipated. The Field class has modified methods for isDisabled/isReadonly that specifically check if the parent component (i.e. container) is an instanceof FieldSet (or Form, which is working AFAIK). The problem is that the design of FieldSet relies on an instance of its private inner class FieldSet.InnerContainerField for managing those child elements. When I step through the code in debug mode, the class instance is of this inner class type (InnerContainerField) not FieldSet. Since InnerContainerField is not a type of FieldSet, the subsequent logic is ignored.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[JIRA] Commented: (CLK-497) FieldSet isDisabled and isReadonly
methods broken
Posted by "James P Brown (JIRA)" <ji...@apache.org>.
[ http://issues.apache.org/click/browse/CLK-497?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=11717#action_11717 ]
James P Brown commented on CLK-497:
-----------------------------------
Thank you for the speedy reply.
I actually like that the behaviour of the parent to child state is managed through the isDisabled/isReadonly methods, because it keeps the component state and behaviours somewhat independent of each other. With the approach you suggest above, I think you may get into trouble because the container would only get one chance to "push" to its children: at the time set is invoked. However, if child components are added subsequent to the state change, then they would miss the event. As well, if a child component changes its state after the event, then it would be out of sync with the event. The virtue of your reliance on the isDisabled invocation is that it neatly side steps the reliance on state change events, and puts everything into the runtime state evaluations. Otherwise, you would need a kind of "failsafe" set invocation in the lifecycle that forces all components to be in the correct state, which I think isn't the best way to go.
Nonetheless, I think the framework is very good, and my use cases shouldn't impact your design decisions. I should also point out that you did in fact resolve my original issue, and that I am abusing your good nature by expanding the scope of my original defect request. I think the onus is on me to think this out a bit more, and make a better effort to understand the internals of the code.
Thank you,
James
> FieldSet isDisabled and isReadonly methods broken
> -------------------------------------------------
>
> Key: CLK-497
> URL: http://issues.apache.org/click/browse/CLK-497
> Project: Click
> Issue Type: Bug
> Components: core
> Affects Versions: 2.0.1
> Environment: Windows XP, Java 6
> Reporter: James P Brown
> Attachments: FieldSet_isDisabled_isReadonly_bug.zip
>
>
> The FieldSet is supposed to force its child components to be disabled/readonly when it is set to disabled/readonly. I did not observe this when I attempted to create a FieldSet with a child component.
> I believe the code to support this is not working as anticipated. The Field class has modified methods for isDisabled/isReadonly that specifically check if the parent component (i.e. container) is an instanceof FieldSet (or Form, which is working AFAIK). The problem is that the design of FieldSet relies on an instance of its private inner class FieldSet.InnerContainerField for managing those child elements. When I step through the code in debug mode, the class instance is of this inner class type (InnerContainerField) not FieldSet. Since InnerContainerField is not a type of FieldSet, the subsequent logic is ignored.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[JIRA] Updated: (CLK-497) FieldSet isDisabled and isReadonly
methods broken
Posted by "James P Brown (JIRA)" <ji...@apache.org>.
[ http://issues.apache.org/click/browse/CLK-497?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
James P Brown updated CLK-497:
------------------------------
Attachment: FieldSet_isDisabled_isReadonly_bug.zip
I am attaching source code changes I made to the Field and FieldSet classes to address this defect (and the svn patch files for a project level diff), and modified FieldSetTest and FormTest unit test classes to test bug and fix.
I do not believe this to be the best way to address the problem, as I turned the private inner class into a public one, and added two methods to expose the isDisabled and isReadonly data of the outer class FieldSet. However, this is a drastic measure that clearly breaks the intended design of the inner class. I believe someone more familiar with this code will have to sort this out.
Nonetheless, I believe my changes to the unit test classes to be of value going forward, regardless of the true solution.
> FieldSet isDisabled and isReadonly methods broken
> -------------------------------------------------
>
> Key: CLK-497
> URL: http://issues.apache.org/click/browse/CLK-497
> Project: Click
> Issue Type: Bug
> Components: core
> Affects Versions: 2.0.1
> Environment: Windows XP, Java 6
> Reporter: James P Brown
> Attachments: FieldSet_isDisabled_isReadonly_bug.zip
>
>
> The FieldSet is supposed to force its child components to be disabled/readonly when it is set to disabled/readonly. I did not observe this when I attempted to create a FieldSet with a child component.
> I believe the code to support this is not working as anticipated. The Field class has modified methods for isDisabled/isReadonly that specifically check if the parent component (i.e. container) is an instanceof FieldSet (or Form, which is working AFAIK). The problem is that the design of FieldSet relies on an instance of its private inner class FieldSet.InnerContainerField for managing those child elements. When I step through the code in debug mode, the class instance is of this inner class type (InnerContainerField) not FieldSet. Since InnerContainerField is not a type of FieldSet, the subsequent logic is ignored.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[JIRA] Resolved: (CLK-497) FieldSet isDisabled and isReadonly
methods broken
Posted by "Bob Schellink (JIRA)" <ji...@apache.org>.
[ http://issues.apache.org/click/browse/CLK-497?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Bob Schellink resolved CLK-497.
-------------------------------
Resolution: Fixed
The test cases and fix have been checked in. The FieldSet is now correctly set as the parent.
Closing the issue for now as we can always revisit the cascade/block issue at a later stage.
> FieldSet isDisabled and isReadonly methods broken
> -------------------------------------------------
>
> Key: CLK-497
> URL: http://issues.apache.org/click/browse/CLK-497
> Project: Click
> Issue Type: Bug
> Components: core
> Affects Versions: 2.0.1, 1.5.1
> Environment: Windows XP, Java 6
> Reporter: James P Brown
> Assignee: Bob Schellink
> Fix For: 2.1.0, 2.0.2, 1.5.2
>
> Attachments: FieldSet_isDisabled_isReadonly_bug.zip
>
>
> The FieldSet is supposed to force its child components to be disabled/readonly when it is set to disabled/readonly. I did not observe this when I attempted to create a FieldSet with a child component.
> I believe the code to support this is not working as anticipated. The Field class has modified methods for isDisabled/isReadonly that specifically check if the parent component (i.e. container) is an instanceof FieldSet (or Form, which is working AFAIK). The problem is that the design of FieldSet relies on an instance of its private inner class FieldSet.InnerContainerField for managing those child elements. When I step through the code in debug mode, the class instance is of this inner class type (InnerContainerField) not FieldSet. Since InnerContainerField is not a type of FieldSet, the subsequent logic is ignored.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[JIRA] Updated: (CLK-497) FieldSet isDisabled and isReadonly
methods broken
Posted by "Bob Schellink (JIRA)" <ji...@apache.org>.
[ http://issues.apache.org/click/browse/CLK-497?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Bob Schellink updated CLK-497:
------------------------------
Fix Version/s: 1.5.2
2.0.2
2.1.0
Assignee: Bob Schellink
Affects Version/s: 1.5.1
> FieldSet isDisabled and isReadonly methods broken
> -------------------------------------------------
>
> Key: CLK-497
> URL: http://issues.apache.org/click/browse/CLK-497
> Project: Click
> Issue Type: Bug
> Components: core
> Affects Versions: 2.0.1, 1.5.1
> Environment: Windows XP, Java 6
> Reporter: James P Brown
> Assignee: Bob Schellink
> Fix For: 2.1.0, 2.0.2, 1.5.2
>
> Attachments: FieldSet_isDisabled_isReadonly_bug.zip
>
>
> The FieldSet is supposed to force its child components to be disabled/readonly when it is set to disabled/readonly. I did not observe this when I attempted to create a FieldSet with a child component.
> I believe the code to support this is not working as anticipated. The Field class has modified methods for isDisabled/isReadonly that specifically check if the parent component (i.e. container) is an instanceof FieldSet (or Form, which is working AFAIK). The problem is that the design of FieldSet relies on an instance of its private inner class FieldSet.InnerContainerField for managing those child elements. When I step through the code in debug mode, the class instance is of this inner class type (InnerContainerField) not FieldSet. Since InnerContainerField is not a type of FieldSet, the subsequent logic is ignored.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[JIRA] Commented: (CLK-497) FieldSet isDisabled and isReadonly
methods broken
Posted by "Bob Schellink (JIRA)" <ji...@apache.org>.
[ http://issues.apache.org/click/browse/CLK-497?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=11716#action_11716 ]
Bob Schellink commented on CLK-497:
-----------------------------------
Its true that the Field#isDisabled checks with the parent first if it should be disabled. However if the parent (Form or FieldSet) is not disabled it falls back to its own disabled property. So it would be possible to create a custom Container which pushes the disabled property to child fields:
public class MyContainer extends AbstractContainer {
public void setDisabled (boolean disabled) {
List<Field> fields = ContainerUtils.getFields(this);
for (Field field : fields) {
field.setDisabled(disabled);
}
}
}
The code above would push the state to the child fields. Granted a disabled Form or FieldSet would override the state MyContainer pushed to its fields.
Hmm perhaps we should remove the Field#isDisabled logic and let the Form/FieldSet push the state to the child fields? Would that help?
kind regards
bob
> FieldSet isDisabled and isReadonly methods broken
> -------------------------------------------------
>
> Key: CLK-497
> URL: http://issues.apache.org/click/browse/CLK-497
> Project: Click
> Issue Type: Bug
> Components: core
> Affects Versions: 2.0.1
> Environment: Windows XP, Java 6
> Reporter: James P Brown
> Attachments: FieldSet_isDisabled_isReadonly_bug.zip
>
>
> The FieldSet is supposed to force its child components to be disabled/readonly when it is set to disabled/readonly. I did not observe this when I attempted to create a FieldSet with a child component.
> I believe the code to support this is not working as anticipated. The Field class has modified methods for isDisabled/isReadonly that specifically check if the parent component (i.e. container) is an instanceof FieldSet (or Form, which is working AFAIK). The problem is that the design of FieldSet relies on an instance of its private inner class FieldSet.InnerContainerField for managing those child elements. When I step through the code in debug mode, the class instance is of this inner class type (InnerContainerField) not FieldSet. Since InnerContainerField is not a type of FieldSet, the subsequent logic is ignored.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[JIRA] Commented: (CLK-497) FieldSet isDisabled and isReadonly
methods broken
Posted by "James P Brown (JIRA)" <ji...@apache.org>.
[ http://issues.apache.org/click/browse/CLK-497?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=11706#action_11706 ]
James P Brown commented on CLK-497:
-----------------------------------
Thank you for you prompt reply to my issue. Your way is clearly the right way to solve this particular issue.
However, since you've opened the door to considering how this is implemented, I would like to make a couple of observations that might improve the current design.
I thought that the Field class use of instanceof to determine whether the parent was of type Form of FieldSet to be limiting in the long-term. If someone were to create a component that required this behaviour, but was not of either of those types, they would be unable to take advantage of the code in Field. I believe FieldSet and Form to be too specific a set of types to model this behaviour.
My second point, which may solve the previous one, is that I think there would be value in modifying the design to treat the container behaviour to "push" down its disabled/readonly state as configurable, AND allow components to block the push. Perhaps container-like components implement an interface supporting a method called "cascade" and all components support a method called "block".
The Field class (or any other applicable class) would check the parent component for the cascade state, and if it is true then check the applicable disabled/readonly flag. However, if the component has it's block state set to true, it would never check the parent component.
By default I would make Form and FieldSet casade true and all Fields (or similar) block false. This would produce the current implementation behaviours.
I believe the advantage of an approach such as this is that if I never want the container to push its state to its children, I can turn it off. More importantly, if I want some, but not all, elements of the container to be disabled/readonly, the components will be able to block the parent behaviour.
For example, If one has a two FieldSets, and a Form all contained in one Parent FieldSet, but the Form should not be disabled, one could enable the block flag to true, while setting the parent state dsiabled to true. Since the default behaviour would be to cascade, the two child FieldSets would be disabled, but the Form would be enabled.
I think it addresses some of my concerns in the first point because the parent cascade behaviour could be tied to a new interface, or base type, which any component could implement or inherit from. However, there would also need to be some consideration for the isDisabled/isReadonly methods to ensure that a suitable downcast from Control could be done in all circumstances.
Regards,
James
> FieldSet isDisabled and isReadonly methods broken
> -------------------------------------------------
>
> Key: CLK-497
> URL: http://issues.apache.org/click/browse/CLK-497
> Project: Click
> Issue Type: Bug
> Components: core
> Affects Versions: 2.0.1
> Environment: Windows XP, Java 6
> Reporter: James P Brown
> Attachments: FieldSet_isDisabled_isReadonly_bug.zip
>
>
> The FieldSet is supposed to force its child components to be disabled/readonly when it is set to disabled/readonly. I did not observe this when I attempted to create a FieldSet with a child component.
> I believe the code to support this is not working as anticipated. The Field class has modified methods for isDisabled/isReadonly that specifically check if the parent component (i.e. container) is an instanceof FieldSet (or Form, which is working AFAIK). The problem is that the design of FieldSet relies on an instance of its private inner class FieldSet.InnerContainerField for managing those child elements. When I step through the code in debug mode, the class instance is of this inner class type (InnerContainerField) not FieldSet. Since InnerContainerField is not a type of FieldSet, the subsequent logic is ignored.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[JIRA] Commented: (CLK-497) FieldSet isDisabled and isReadonly
methods broken
Posted by "Bob Schellink (JIRA)" <ji...@apache.org>.
[ http://issues.apache.org/click/browse/CLK-497?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=11702#action_11702 ]
Bob Schellink commented on CLK-497:
-----------------------------------
Thanks James, your analysis is correct. The problem is that FieldSet is not set as the true parent of the child control.
As a quick fix we can set the FieldSet as the parent like so:
private class InnerContainerField extends AbstractContainer {
...
public Control insert(Control control, int index) {
super.insert(control, index);
control.setParent(FieldSet.this); // <-- new line
...
}
Still the FieldSet implementation is quite complex because of all the delegation to the inner class. Perhaps it would be better if we implement the Container interface from scratch instead of delegating to the inner class. This will introduces some code duplication but is a lot simpler to read and understand.
And yes, will definitely add your tests to protect against regressions.
kind regards
bob
> FieldSet isDisabled and isReadonly methods broken
> -------------------------------------------------
>
> Key: CLK-497
> URL: http://issues.apache.org/click/browse/CLK-497
> Project: Click
> Issue Type: Bug
> Components: core
> Affects Versions: 2.0.1
> Environment: Windows XP, Java 6
> Reporter: James P Brown
> Attachments: FieldSet_isDisabled_isReadonly_bug.zip
>
>
> The FieldSet is supposed to force its child components to be disabled/readonly when it is set to disabled/readonly. I did not observe this when I attempted to create a FieldSet with a child component.
> I believe the code to support this is not working as anticipated. The Field class has modified methods for isDisabled/isReadonly that specifically check if the parent component (i.e. container) is an instanceof FieldSet (or Form, which is working AFAIK). The problem is that the design of FieldSet relies on an instance of its private inner class FieldSet.InnerContainerField for managing those child elements. When I step through the code in debug mode, the class instance is of this inner class type (InnerContainerField) not FieldSet. Since InnerContainerField is not a type of FieldSet, the subsequent logic is ignored.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[JIRA] Commented: (CLK-497) FieldSet isDisabled and isReadonly
methods broken
Posted by "Bob Schellink (JIRA)" <ji...@apache.org>.
[ http://issues.apache.org/click/browse/CLK-497?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=11711#action_11711 ]
Bob Schellink commented on CLK-497:
-----------------------------------
Hi James,
Though about this a bit today, but ultimately such a change is moving too far away from Click's design and the 80/20 rule. The ability to block/cascade state seems like an edge case, not the norm.
The current impl covers the common case where disabling the parent will disable the child controls. However if there is a need for a more advanced approach, one can create a custom Form / FieldSet / Container that overrides setDisabled/setReadonly and push/block the state:
kind regards
bob
> FieldSet isDisabled and isReadonly methods broken
> -------------------------------------------------
>
> Key: CLK-497
> URL: http://issues.apache.org/click/browse/CLK-497
> Project: Click
> Issue Type: Bug
> Components: core
> Affects Versions: 2.0.1
> Environment: Windows XP, Java 6
> Reporter: James P Brown
> Attachments: FieldSet_isDisabled_isReadonly_bug.zip
>
>
> The FieldSet is supposed to force its child components to be disabled/readonly when it is set to disabled/readonly. I did not observe this when I attempted to create a FieldSet with a child component.
> I believe the code to support this is not working as anticipated. The Field class has modified methods for isDisabled/isReadonly that specifically check if the parent component (i.e. container) is an instanceof FieldSet (or Form, which is working AFAIK). The problem is that the design of FieldSet relies on an instance of its private inner class FieldSet.InnerContainerField for managing those child elements. When I step through the code in debug mode, the class instance is of this inner class type (InnerContainerField) not FieldSet. Since InnerContainerField is not a type of FieldSet, the subsequent logic is ignored.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.