hugoheden
|
Posted: December 13, 2009 21:44 by hugoheden
|
|
The following should pretty much describe all that has happened on the dev-hh-2 branch (at least up to revision 217, but I don't think there'll be any more major changes). To understand the changes, it's probably easier to read this than the the individual commit messages. EDIT Jan 11 2010: I've continued working on the branch, and revisions 218 and 220 somewhat affects the below. I've updated the text using the color of purple. Work has been done to trim down SwingValidationGroup to something sensible. The convenience methods for adding UI-components (together with Validators and optionally a ValidationStrategy and also optionally a custom ValidationUI for decorating the added component (revision 220)) are still there, but all those weird methods for creating a ValidationListener but not adding it to the group are now gone. The purpose of those "create" methods was originally to support the ValidationBunch stuff, but that has been reworked as well, see below. The ValidationGroup - ValidationListener - ValidationBunch infrastructure has been reworked. The ValidationBunch is no longer a separate class (i.e it has been removed). Instead, its functionality has been merged into ValidationGroup: In order to make a ValidationGroup work as the former ValidationBunch did, an instance of a newly introduced class, AdditionalGroupValidation, is passed to the ValidationGroup constructor. (And support for doing so has been added to the create methods of the subclass SwingValidationGroup). The method AdditionalGroupValidation#performAdditionalValidation can be overriden by client code to perform validation not covered by the individual components. Also, there are two boolean parameters, passed to the AdditionalGroupValidation constructor, that configure how the AdditionalGroupValidation is to behave:
Now, these changes required that a ValidationGroup (the "ValidationBunch") could be "added" to another ValidationGroup, so such support was added thusly: ValidationGroup and ValidationListener now have a common superclass, ValidationItem. The parameter of ValidationGroup#add is now ValidationItem (instead of ValidationListener). Hence, ValidationGroups can now be added and removed from each other using the ValidationGroup#add and ValidationGroup#remove methods. In the add-method, there is a check to detect ancestry-to-self, i.e that makes sure a child validation group cannot have an ancestor of itself added to itself - i.e. preemptively avoiding endless loops. As a consequence, the methods ValidationGroup#addValidationGroup and ValidationGroup#removeValidationGroup were removed, as well as the no-longer-needed GroupSpecificValidationUI. Adding a ValidationGroup (to another ValidationGroup) is similar to adding a ValidationListener: The added ValidationItem, the child, will affect the parent UI if there's a Problem. Note: In the now removed ValidationGroup#addValidationGroup, there was a boolean flag named useUI, indicating whether the added ValidationGroup (the child) should continue to update its own ValidationUI:s as well (in addition to the ones of the parent) or not. This boolean flag has no correspondance in the current API, i.e, it is currently not possible to "turn off" the ValidationUI of the added child. ValidationListener now implements java.util.EventListener again. (Note though that its superclass ValidationItem does not.) In ValidationListener, something very similar to what was before ValidationListener#validate() has been reintroduced: protected abstract void performValidation(Problems problems); The goal is to reduce the conceptual burden for ValidationListener subclasses, and let the SimpleValidation infrastructure (ValidationListener and ValidationGroup etc) do "more work" (as it did before I started messing with all this). The method ValidationGroup#modifyComponents is renamed and pulled up to superclass: ValidationItem#invokeWithValidationSuspended. The isSuspended() method is now back to having package visibility (it's been public for a while) |
The dev-hh-2 branch
22 posts
Replies: 21 - Last Post: January 29, 2010 09:53
by: hugoheden
by: hugoheden
Tim Boudreau
|
Posted: January 13, 2010 07:28 by Tim Boudreau
|
This generally sounds good. Might like to find a different name for AdditionalGroupValidation - it feels a little clunky, but could stay if you feel strongly about it. I don't have any better ideas about it at the moment, but I'm sleepy and my basement is filling with water  > This boolean flag has no correspondance in the current API, i.e, it is currently not > possible to "turn off" the ValidationUI of the added child. This one I think is necessary - there are some specific use-cases - I'm using it now in NetBeans. Example: In Java Card, I have a panel that lets you select an SDK and a Card associated with that SDK. It uses createValidationLabel() to show error messages. It is used is in a dialog - if you open a project with an invalid setting for these, you get a popup inviting you to reset it to valid values, with 2 combo boxes for available SDKs/Cards. But I also use it inside the Project Properties dialog (which has its own API for showing error messages - I use an implementation of ValidationUI which calls the dialog's error showing mechanism). Without the ability to turn off the panel's built-in validation, the error message is going to be shown twice - once at the bottom of the dialog (the built in Project Properties stuff) and one in the middle of the screen inside that panel. That will look very bad. The purpose of being able to disable a ValidationUI is so you can compose together reusable panels which have their own ValidationUI, but only let the appropriate ValidationUI win when one is inside another. This could be done by having the panels just have ValidationGroups, but that ends up being a more code for the client, since every place it is used will need to provide a ValidationUI, and sometimes that's not necessary. Slight preference for runWithValidationSuspended() - invoke makes me think it might be run asynchronously or something - reminds me of EventQueue.invokeLater(). Nonetheless, either one is a great improvement over "modifyComponents". |
|
hugoheden
|
Posted: January 13, 2010 12:06 by hugoheden
|
|
This generally sounds good. Might like to find a different name for AdditionalGroupValidation - it feels a little clunky, but could stay if you feel strongly about it. Hmm, I agree, it does feel clunky, will think about it. ... and my basement is filling with water Ouch..! This one I think is necessary - there are some specific use-cases - I'm using it now in NetBeans. Example:... Understood (I think!), will look at it Slight preference for runWithValidationSuspended() - invoke makes me think it might be run asynchronously or something - reminds me of EventQueue.invokeLater(). Nonetheless, either one is a great improvement over "modifyComponents". OK, sounds good, will fix  |
|
hugoheden
|
Posted: January 13, 2010 23:52 by hugoheden
|
|
Might like to find a different name for AdditionalGroupValidation - it feels a little clunky, but could stay if you feel strongly about it. Ok, switched to "GroupValidator" for now. This one I think is necessary - there are some specific use-cases - I'm using it now in NetBeans. Example:... Introduced a method ValidationGroup#addWithDisabledUI() (as a complement to the regular add-method). Not sure this is clean enough.. or convenient enough. Slight preference for runWithValidationSuspended() - invoke makes me think it might be run asynchronously or something - reminds me of EventQueue.invokeLater(). Nonetheless, either one is a great improvement over "modifyComponents". Ok, done. |
|
Tim Boudreau
|
Posted: January 26, 2010 05:54 by Tim Boudreau
|
|
> This option, revalidate-all-children, was completely removed in revision 218, because I am having trouble > coming up with a usecase for it Not sure if I understand what this means. Is this for re-running validation in other attached groups, or re-running validation within the owning group? I'll just describe a use-case and you can tell me if it is still supported: In the UI that is the screen shot in the FAQ, we have three fields and a checkbox: Port: URL: Card Manager URL: Card is on Remote Machine [x] By default, URL and Card Manager URL are automatically updated if you change the value for Port - *unless* you have manually modified either of the URLs, in which case they are not changed. If they are not all using the same port, the user should get a warning - this is a faintly possible configuration, but most likely user error. So any change in those four items should trigger revalidation of at least all of them. There is also the case where the URLs contain 'localhost' or '127.0.0.1', and the Remote checkbox is checked, in which case the user sees a warning that the URLs look like local ones, so the Remote setting is probably wrong. Are these scenarios still supported? |
|
hugoheden
|
Posted: January 26, 2010 23:52 by hugoheden
|
|
All the above is still supported.. hrm, and if I'm wrong it should be pretty easy to rectify -- the functionality is pretty much there, it's just been removed from the API. > Not sure if I understand what this means. Is this for re-running validation in other attached groups, > or re-running validation within the owning group? Consider the usual case with a few UI components (or rather ValidationListeners) with Validators added to a ValidationGroup. This ValidationGroup was supplied a GroupValidator instance by client code at creation. When a UI component is "changed" (either programmatically or because of user interaction), then validation is triggered. The following will happen:
Yes, this should cover the use-cases. So, what do I mean by saying that the "revalidate-all-children" option is gone? Well, what we could do in *addition* to the above would be to actually re-run the validators of all UI-components within the ValidationGroup, not only the triggering one. We don't do that above. This is the option that is gone. The rationale for this is that there should never be a need for that, because the other UI components would have been revalidated when they were last changed! Changing *one* UI component can only affect the validity of *that* component and the *combination* of the components -- but it can not change the validity of *another* component taken as a singular. Does this make sense? Bah.. maybe it's just easier to write code to verify that the use-cases are supported |
|
hugoheden
|
Posted: January 16, 2010 21:50 by hugoheden
|
|
The following describes what has been done on branch dev-hh-2, revisions 221 to 236 more or less. To sum up: Work was done on Validators. The idea was to introduce common validators for validating JLists and AbstractButton[], and to separate out these validators from the String validators. Here goes: The old Validators enum has been "purified" to only handle Strings, renamed to StringValidators and moved to new sub-package ....api.builtin.stringvalidation. The method mayNotEndWith() was made static (I believe it was non-static by mistake?). The merge() method (together with AndValidator) and the limitSeverity() method (together with CustomLevelValidator) are generic and not specific for Strings, so they were moved to the api package instead (where the Validator interface resides), to a new class called ValidatorUtils. Also, a new Validator package was created, intended for validation of JList and AbstractButton[]: ...api.builtin.indexvalidation. This is similar to package stringvalidaton, but validates Integer[] (the indices of selected items/buttons) instead of String. Converters were also added (ButtonModel[]->Integer[]) and (ListSelectionModel[]->Integer[]). A new enum was added as well, IndexValidators, similar to the StringValidators. IndexValidators does not really contain much though. Currently there is just SelectionMustNotBeEmptyValidator. I'm thinking we could have something like * EXACTLY_ONE_SELECTED_ITEM_REQUIRED * AT_LEAST_ONE_SELECTED_ITEM_REQUIRED * EXACTLY_TWO_SELECTED_ITEMS_REQUIRED * AT_LEAST_TWO_SELECTED_ITEMS_REQUIRED * static Validator<Integer[]> exactNumberOfSelectedItemsValidator(int n) * static Validator<Integer[]> minimumNumberOfSelectedItemsValidator(int n) * static Validator<Integer[]> maximumNumberOfSelectedItems(int n) But I'm not sure this is useful..? Anyway, with these changes, ButtonMustBeSelectedValidator became obsolete and was removed (SelectionMustNotBeEmptyValidator is more generic since it handles Integer[] which can be used for JList/ListSelectionModel as well). ButtonsValidationListener (which was previously a method local class) was factored out, and a JListValidationListener was added. Convenient add-methods in SwingValidationGroup for creating and adding them were added. |
|
hugoheden
|
Posted: January 16, 2010 22:06 by hugoheden
|
|
The following describes what has happened in revision 237 to 258 * Changed AdditionalGroupValidation ==> GroupValidator * Changed invokeWithValidationSuspended() ==>runWithValidationSuspended() * Added ValidationGroup#addWithDisabledUI(). This was slightly complicated and caused some implementation changes in the ...api.ui package to happen. * Removed SwingValidationListener from class hiearchy. It did not do much good, and also could not be used for the new ButtonsValidationListener. Instead JTextComponentValidationListener, JComboBoxValidationListener, JListValidationListener now inherit ValidationListener directly. (These classes are all package private.) SwingValidationListener was renamed to SwingValidationListenerFactory and made public, and create-methods were added for the various ValidationListeners. * Removed four of the add-methods in SwingValidationGroup. They have become somewhat obsolete now that there is a separate SwingValidationListenerFactory class with many factory methods. SwingValidationGroup has become rather clean. * ValidationItem#getCurrentLeadProblem() has been "removed" from public API; is no longer protected but package-private. * Removed a public method in ComponentDecorator: public static final ComponentDecorator createDefault() I don't think it's currently very useful. In the future (post 1.0?), I'd like to add ability to customize the SimpleDefaultComponentDecorator, and then a public creator method may be more useful -- but then probably with a strategy object parameter. * API name change: ComponentDecorator ==> SwingComponentDecorationFactory |
|
hugoheden
|
Posted: January 17, 2010 21:05 by hugoheden
|
|
I think I'm pretty much done with dev-hh-2 now. *If* there's interest, I'd obviously appreciate review of the code and further discussion. I do realize however that nobody has asked me to keep on work with this, and that there may be time constraints (before the next release, 1.0?) etc preventing this work from being of any use. So, no worries! But below, for the sake of discussiom, I'll assume that this branch is of interest after all, and I have some questions and some more details to report. * I'm currently working on unit tests and documentation of ValidationGroup. (There's a lot of logic in that class that needs to be tested.) * Question: - I feel slightly uncomfortable having public fields like ValidationUI.noOpValidationUI, because everybody says that accessor methods is preferrable for many reasons.. But ValidationUI is an interface (and that's good) so it can't have an accessor method. Are there any alternatives, or are we ok as it is? More questions to come (I think) |
|
Tim Boudreau
|
Posted: January 26, 2010 06:04 by Tim Boudreau
|
|
> feel slightly uncomfortable having public fields like ValidationUI.noOpValidationUI, because everybody says that accessor > methods is preferable for many reasons Accessor method might be preferable (really I've always thought the restriction that interfaces cannot have static methods was silly and arbitrary). At the same time, since the instance has no state and is immutable, the only practical consideration is that old objects (ones that have survived enough garbage collections that they now live in the old generation memory area of the heap) can cause processor cache misses and hurt performance - and this is really a non-issue for GUI applications, since the VM's definition of old typically means that an object becomes "old" in less than a minute, and there are so many "old" objects in a GUI that cache misses are the norm. I'm fine with an accessor method if we can find a place to put it where users will find it easily. Would rather not add a class to the visible API with a factory method, if possible. It also might be possible to make the class completely internal - AFAIK it is only used for a few cases where you need an initial value, isn't it? I'll check out the branch and look at it this week - with all the Oracle/Sun stuff going on, and a beta of the Java Card modules built and ready to distribute, I've finally got some time for other things. |
|
hugoheden
|
Posted: January 26, 2010 22:39 by hugoheden
|
|
> It also might be possible to make the class completely internal - AFAIK it is only used for a few cases where you need an initial value, isn't it? Well, it may be *convenient* for client code to have easy access to the ValidationUI.noOpValidationUI -- the demo code in BunchDemo currently uses that to "disable" decoration for a UI component: bunch.add(SwingValidationListenerFactory.createJComboBoxValidationListener(sumComboBox, ValidationUI.noOpValidationUI)); Anyway, no strong opinions here, we can keep it in. Or out..?
|
|
Tim Boudreau
|
Posted: January 26, 2010 06:45 by Tim Boudreau
|
|
Reviewing the javadoc - first pass. Generally all of this looks like really good, careful, meticulous work Initial thoughts - I'm really just giving it a quick read-through right now: Does ValidationItem need to be public? I.e. it provides a common superclass, but do we ever expect subclasses to be created outside the library? If not, perhaps we could either hide it from the API or make its constructor package-private? Vaguely wondering if SwingComponentDecorationFactory should be an optional constructor parameter for SwingValidationUI, rather than a global singleton - that way if someone wants a specific decorator for a specific UI, they don't have to change it for the whole application. Probably not for this rev, but there are a lot of tree views with checkboxes out there. Might be interesting to provide a CheckboxTreeModel and a way to validate that. But probably out of scope. Slight preference for ValidationGroup.add (ValidationGroup, boolean disableUI) - it would be one less method for people to have to know about, and the meaning of the parameter would be pretty clear. Looks good! -Tim |
|
Tim Boudreau
|
Posted: January 26, 2010 19:54 by Tim Boudreau
|
| One other thing - I think for ValidationUI, setProblem(Problem) and clearProblems() is probably clearer. Any objections? |
|
hugoheden
|
Posted: January 26, 2010 22:57 by hugoheden
|
|
One other thing - I think for ValidationUI, setProblem(Problem) and clearProblems() is probably clearer. Any objections? No, that's sounds fine! Edit -- I'll expand just a bit on that. I agree that "showProblem" is not good, since it is to be used for clearing/removing a Problem as well (by passing null), and it is confusing to call "showProblem" to actually not show a Problem. So therefore I think the setProblem(Problem)/clearProblem() is better. The only small issue I have with that is that we will need to specify in the javadoc whether passing null to setProblem(Problem) either
I often try to avoid rules like that (contract enforced by documentation and asserts rather than the compiler). But I can't really come up with a better solution, so let's go for it. |
|
hugoheden
|
Posted: January 26, 2010 22:57 by hugoheden
|
|
> Does ValidationItem need to be public? Yes I think so, because the type is exposed in ValidationGroup#add() > I.e. it provides a common superclass, but do we ever expect subclasses to be created outside the library? If not, perhaps we could either hide it from the API or make its constructor package-private? The constructor is already package private, so we're in sync there > Vaguely wondering if SwingComponentDecorationFactory should be an optional constructor parameter for > SwingValidationUI, rather than a global singleton - that way if someone wants a specific decorator for a > specific UI, they don't have to change it for the whole application. The idea was that the UI-component-decoration would be configurable -- * application wide (hence the global singleton) * for individual ValidationGroups (the create-methods in SwingValidationGroup has an optional SwingComponentDecorationFactory parameter) * for individual UI-components (a custom ValidationUI can be specified when creating the various SwingValidationListeners) -- Any thoughts? Perhaps that's just too much API for too little gain -- i.e is this getting a little bloated? > Slight preference for ValidationGroup.add (ValidationGroup, boolean disableUI) Ok, I agree, that looks better. |
|
Tim Boudreau
|
Posted: January 27, 2010 08:40 by Tim Boudreau
|
|
I notice we have ValidationItem.performValidation(), which is public final. Then there is ValidationListener, a subclass of ValidationItem, which has protected final triggerValidation() that just calls performValidation(). Any need for both? I've been looking for places where we can make the API smaller without losing anything. I think I may have found some: I think I've actually figured out a way to, type-safely, make SwingValidationGroup go away: public final <ComponentType, ValueType> void add (ComponentType comp, Validator<ValueType>... validators) which delegates to SwingValidationListenerFactory's new method static <CType, ModelType> ValidationListener<CType> createValidationListener(final CType component, final ValidationStrategy strategy, ValidationUI validationUI, final Validator<ModelType> validator) That, in turn, will first see if it is any of the types we already have listeners for (using a slightly awful reflection trick to find out the generic type of the validator). If it is an unknown component type, then it will iterate ServiceLoader for any declaratively registered (flat files in META-INF/services on the classpath) SwingValidationListenerFactory (we could take out the word "Swing" at this point) instances, and read an annotation @Retention(RetentionPolicy.RUNTIME) public @interface ListenerFor { Class<?> componentType(); Class<?> modelObjectType(); } and if it finds a match for the component type and validator type, then the add succeds. On the one hand, this eliminates any dependency on Swing classes - CType and ModelType can be anything. On the other hand, it is less intuitive than a method that takes a JComboBox, etc. - that has some value because it's very clear what it's for, and guaranteed that you won't get an exception by adding some combination that there's no listener factory registered for. So I'm wavering between "this is clutter" and "this is ease of use." What do you think? I could move the generic listener generation stuff into ValidationGroup (in which case SwingValidationGroup is *really* just a set of convenience add methods); that, at least, is probably a good idea. The one bit of logic that is Swing-specific is handling the component-decorator - that, along with convenience, might be justification enough to keep it around. -Tim -Tim |
|
hugoheden
|
Posted: January 27, 2010 22:26 by hugoheden
|
|
> I notice we have ValidationItem.performValidation(), which is public final. Then there is ValidationListener, > a subclass of ValidationItem, which has protected final triggerValidation() that just calls performValidation(). > Any need for both? Hmm. It would seem obvious that the answer would be "no, we could get rid of validationTriggered()". But I'm not sure.
On the other hand ... If we think about this and rewrite the documentation for performValidation() a bit, we can *define* performValidation() to mean "a request for revalidation (running the actual validators) of the managed component" (or "all managed components" in the more general case of the super class ValidationItem). This would be exactly the right thing to do when the content of a UI component has been changed.. and .. hmm, Ok, yes, we could probably get rid of validationTriggered() and use performValidation() instead |
|
Tim Boudreau
|
Posted: January 27, 2010 08:42 by Tim Boudreau
|
| One other thing: I haven't tested it yet - in the case of disableUI being true, will that break the child group's component decorator, or cause the one from the child group to be used instead? |
|
hugoheden
|
Posted: January 27, 2010 11:40 by hugoheden
|
|
It will disable the ValidationUI:s of the added child group. The ValidationUI of the parent group will still be enabled. I figured the parent is the one that typically manages an OK button or Next button in a panel, and thus is typically not the one that you would want disabled. (disableUI means that the ValidationUI will remain cleared from Problems, i.e an OK/Next button would typically remain enabled even in the presence of errors). Thoughts? Edit: > It will disable the ValidationUI:s of the added child group. -- by this I mean that ValidationUI:s owned directly by the added child group will be disabled. Not the ValidationUI:s of the children of the added child group (for example the decorations of the individual UI-components in an embedded panel) -- these ValidationUI:s will remain enabled. |
|
Tim Boudreau
|
Posted: January 27, 2010 23:33 by Tim Boudreau
|
|
OK, that sounds good. I'm running into a little issue with the idea of generifying ValidationListener so that factories for other components + validator types can be plugged in: There is no way to type-safely infer the generic type of the Validator. I currently have code that works that looks up the type of the third parameter and uses that to determine the validator's type, but that will not be 100% reliable - you could have an overload of validate() that could be matched against. So the choice is: - Forget about declaratively plugging in additional factories for ValidationListeners (and with it a way to allow a generic add() method on ValidationGroup which is not swing-specific (but could throw an exception), OR - Add a method <Class<T>> type() to Validator. That seems a pity, since Validator is a nice, clean interface (could make it an abstract class and make the type a constructor parameter though), and it would mean we need to pollute the interface due to limitations of the implementation of generics in Java. But we would be able to delete SwingValidationGroup (if we want) and have ValidationGroup be final and handle all possible cases. I'm leaning toward the type() solution, although I don't completely like either one. |
Replies: 21 - Last Post: January 29, 2010 09:53
by: hugoheden
by: hugoheden
