Community
Participate
Working Groups
As of 3.3 M6, the public DataBinding API supplies converters only between String and Number and no validators at all. More implementations for common cases would be great. Specific possible improvements: Implement parameterized DateToString-/StringToDateConverters following the model of NumberToStringConverter. Note that this would be useful for read-only views using standard SWT widgets and also for full read-write bindings with custom controls, such as Nebula FormattedText or CDateTime. Implement a StringToBooleanConverter that supports custom true and false strings and/or more defaults. ("Y" and "N", for example.) Character-/IntegerToBoolean would also be nice, to handle common data storage formats. A publicly-accessible ObjectToStringConverter would also be nice, for debugging purposes and as a last-resort default.
Peter, can you please reevalutate against HEAD? This has changed a lot in 3.3M7.
I finally checked out HEAD and took a look. I don't see any relevant changes. I don't think that I _should_ see anything new, since 3.3 is under API freeze. What are you referring to? P.S. Please set the target to 3.4, since a fix would require new API.
(In reply to comment #2) > I finally checked out HEAD and took a look. I don't see any relevant changes. > I don't think that I _should_ see anything new, since 3.3 is under API freeze. > What are you referring to? The changes in 3.3M7 weren't exposed as API. Now that I reread the description I understand I misunderstood your intent as you're asking for API. My comments were that the default converters and valdiators have been enhanced quite a bit in 3.3M7 and I was asking that you reevaluate to determine if we provide what is being requested. It's not API but I think some of this now exists.
*** Bug 187262 has been marked as a duplicate of this bug. ***
In bug 187262, Peter says that he believes our current IConverter / Converter APIs are inadaquate. If this is true, then this *must* be fixed by 3.3 final. Once we're confident in IConverter/Converter, I see no reason not to expose all of the existing Converter implementations as API. Peter, can you elaborate on what is wrong with IConverter/Converter along with some specific use-cases that would trigger those situations. If you're right, I'm really anxious to get this fixed.
Dave, I think you're misunderstanding my request. I have no problem with the current API, but I would like to expand it. I see no reason why this should be a breaking change. It's my understanding that the API for 3.3 has already been frozen. That would make this a request for 3.4 API. Since my request can be satisfied without breaking 3.3 API compatibility, there's no reason to rush. I thought that the specific use cases were self-evident from my original request. I sometimes want a DateToStringConverter that renders only the date and not the time, or vice versa. I sometimes want to map an arbitrary pair of strings to true and false. (An existing data model may already use some non-standard representation of true and false, or I may want to map a two-state field to a boolean for convenience.) I sometimes want to render an arbitrary Object into a text field or Table column, but my framework wants an explicit reference to the converter for some other purpose. Since the existing DateToStringConverter and BooleanToStringConverter do not satisfy these use cases, I would prefer to improve them before they are published (and therefore harder to change). Brad, the converters in 3.3 M7 can only be configured globally. I have two bindings in the same application that must render Dates differently. It should be possible to pass in a DateFormat as a constructor param or a bean property. Ditto for the true and false strings in BooleanToStringConverter.
Mass update - removing 3.4 target. This was one of the bugs I marked for investigation (and potential fixing) in 3.4 but I ran out of time. Please ping on the bug if fixing it would be really important for 3.4, and does not require API changes or feature work.
I was wondering whether this bug is still considered relevant since there was no activity for a long time? In any case, I do also think that in real world projects you very rapidly end up having to implement your own set of validators and converters, mainly due to the following limitations: * There is no way to parameterize the internal validators/converters (see Peter's examples). * There is no way to customize the error message of a validator which I think is also important. Extending the existing validators/converters and promoting them to API would certainly be a good thing in order to avoid clients having to implement their own ones. However, I think that beyond that, setting up the validators/converters for a single binding still requires too much code, mainly because configuring the two update strategies of a binding often requires some data to be specified redundantly. Here are a few examples: * If you want to read-in an integer in hexadecimal format, you most likely also want to display it in hexadecimal format but you still have to configure both update strategies to do so. * If you want to convert a custom string "Y"/"N" to a boolean true/false, you have to set up an afterGetValidator which only accepts "Y"/"N" as input and a converter which maps "Y"/"N" to true/false, so you have to specify the string pair "Y"/"N" twice for the target2model strategy. In addition, you have to specify it again for the model2target strategy to convert true/false back to "Y"/"N". That's why I think that it would be nice to provide some API which facilitates the overall configuration of validators/converters for a binding. Something I could think about is providing a builder style API which reduces the above redundancy. The idea would be to have a single object which allows to configure the validators/converters for a binding (for both update strategies) but more in terms of the constraints to apply than by explicitly creating validators/converters. E.g. to edit a non-null integer value which lies in the range [1,100], one could do something like the following: IntegerEditing.withDefaults() .required() .valueRange(1, 100); The returned IntegerEditing object would then internally create the appropriate afterGetValidator and converters (in both directions) and it would also add model validators to check that some integer is specified and that it lies in the desired range. Custom error messages could also be supported: IntegerEditing.withDefaults() .valueRange(1, 100, "The value must lie in the range [{0}, {1}]"); The editing of a hexadecimal number could then be easily configured by a single method: IntegerEditing.forHexadecimal(); Likewise, for mapping "Y"/"N" to true/false, a single step would be enough: BooleanEditing.forInputStrings("Y", "N"); This would already set up all the validators/converters without any redundancy. Custom validators could also be added: DateEditing.forFormats(new String[] { "dd.mm.yyyy", "dd/mm/yyyy" }) .addModelValidator(new WeekdayDateValidator()); When creating the actual binding, such an editing object could be used to either configure existing update strategies with the accumulated validators/converters or creating them from scratch (this would also apply to list/set bindings): IntegerEditing editing = ...; // Create new update strategies from scratch. dbc.bindValue( targetObservable, modelObservable, // use custom update policy editing.createTargetValueStrategy(UpdateValueStrategy.POLICY_CONVERT), editing.createModelValueStrategy()); // Configure existing update strategies. dbc.bindValue( targetObservable, modelObservable, editing.applyToTargetStrategy(t2mStrategy), editing.applyToModelStrategy(m2tStrategy); IMO, using this kind of editing objects or something similar would greatly help in configuring bindings by reducing the amount required code to do so and by also supporting a richer set of validations/conversions out of the box. That way, you could maybe even afford making the validators/converters public API which might be more flexible since internally you are free to decide whether you e.g. use different validators for different constraints or whether you rather want to use a single, parameterized validator.
(In reply to comment #8) > I was wondering whether this bug is still considered relevant since there was > no activity for a long time? I think it is still relevant, we just haven't had someone push on it. If you have time to help drive this forward for 3.6, should we try to set up a call with the interested parties? Eric Rizzo seems to be interested, too.
(In reply to comment #9) > I think it is still relevant, we just haven't had someone push on it. If you > have time to help drive this forward for 3.6, should we try to set up a call > with the interested parties? Eric Rizzo seems to be interested, too. I think it would be good to give this a try for 3.6 and I believe that I will have some time to devote to this. What would be great is getting some feedback from people having practical experience in using non-trivial validations/conversions with databinding. I think that the following bugs are also related to this one: https://bugs.eclipse.org/bugs/show_bug.cgi?id=203492 https://bugs.eclipse.org/bugs/show_bug.cgi?id=197807
Created attachment 139097 [details] demo for integer and date editing with snippet I think that Java code is always a good basis for discussion so I have tried to code up something. The patch includes classes for configuring the editing of integers and dates and it illustrates what the whole thing might look like. There is also a simple snippet which illustrates the usage of the API. Please note that this is really just a draft implementation (especially the changes on the internal validators/converters are not really clean for now). Essentially, the editing classes support the following: * Configuring the overall editing strategy by e.g. defining the set of date patterns to support. * Applying a set of constraints (which are typically applied as model validators). * Defaulting/Customizing of validation messages. In the snippet, you can see an example of how I think those editing strategies could be used in practice. There is an EditingFactory which defines the default, project specific editing for individual object types. In addition, the EditingFactory also defines the default validation messages so you don't have to specify them all over the place.
What might be an issue of the builder style editing API is that it does not play too well with inheritance which breaks the method chaining. As an example, in the IntegerEditing class there are constraints (required, range, positive, ...) which are also applicable to shorts, longs, BigIntegers, and the like but moving those to an abstract NumberEditing class would not work without breaking method chaining (at least in the lack of generics) so one would probably have to duplicate the API defining those constraints (this is mostly relevant for the editing of numbers where you have many number types to which the same constraints apply). Maybe, that's not too bad since even if you want to implement e.g. a NumberRangeValidator which works for different numeric types, you would probably also need specific factory methods for the individual types (something like NumberRangeValidator.forXxx(Xxx min, Xxx max)) so the amount of API would be about the same. However, it's certainly something to take into consideration.
Created attachment 142074 [details] ongoing implementation including snippets This new patch now includes Editing implementations for Strings, Integers, and Dates and it supports some common conversions/validations for those models. While the current implementation is not yet really elaborated, I think that it already pretty much illustrates how the whole thing would look like. In the Snippet035Editing snippet, you can see some basic usage of the API for setting up value and list bindings and for defining different conversions and validations for different models. In the Snippet036EditingTable snippet, you can also see an example of how an Editing object might be used for defining how a CellLabelProvider displays a model attribute or how an EditingSupport drives the editing inside a table. For that purpose, I've added the following methods: * Editing#convertToTarget(Object modelValue) * Editing#convertToModel(Object targetValue, MultiStatus aggregateStatus) While in general an Editing object is intended to configure the validators/converters and then create appropriate update strategies for a binding, I think that there are also situations in which you don't want to (or actually don't need to) set up a binding because some other framework (e.g. JFace) already drives the whole workflow. In such cases, the above methods might be useful to simply do a conversion between a target and a model value. See the ObservableMapEditingCellLabelProvider and ObservableMapEditingSupport classes in the second snippet for a simple example (it's just for illustration). Even though I think that this kind of API pretty much facilitates setting up a customized binding, I'm still not sure whether it is the right way to go (see also my previous comments) so feedback is very welcome since maybe I'm simply running into the wrong direction :).
I am swamped right now with the e4 0.9 release and traveling to Germany on Thursday. Ovidio, if nobody provides feedback within a week, would you mind pinging again on this bug? Thanks!
Actually, I wanted to post the second patch much earlier but unfortunately I have not found the time to do so and on Saturday I will start my vacation (I'll be away for 4 weeks). That's a bit unfortunate. At least, I'll try to write up some comments about possible restrictions of the proposed API before I leave southwards...
Ok, here are a few comments about the general intent of the API as it is right now: * An Editing object is intended to be used for configuring the validators/converters of both update strategies of a binding. For now, for the target-to-model (t2m) direction, it supports all the possible validators/converters while in the model-to-target (m2t) direction it only supports a converter but no validators since I guess that in this direction there is usually no need for validators. * In the t2m direction, the following validators/converters can be configured: target-validators => target-converter => model-validators => before-set-model-validators. For every validation stage, multiple validators can be chained. * In the m2t direction, only a model-converter can be configured. * Typically, every Editing class will have static factory methods for setting up the basic editing of an object, i.e. both converters and possibly a target-validator (with custom validation messages). Examples: IntegerEditing#forFormat(NumberFormat), DateEditing#forFormats(DateFormat[] inputFormats, DateFormat displayFormat). * Once you have created an editing object, you can add a set of arbitrary model-validators or target-validators (see Editing#add(Target|Model|BeforeSetModel)Validator(IValidator)). * Some common model-validators are directly supported by the Editing classes (e.g. IntegerEditing#range(int min, int max)). Custom validation messages are supported here as well. * From an Editing object, you can finally create appropriate update strategies or configure existing ones to be passed to a binding as usual. * The Editing class also supports directly converting between target and model values wiithout the need for update strategies/bindings (see Editing#convertTo(Target|Model)(..)). See comment #13 for examples as of when this might be useful. And here are some open questions/possible problems: * Should an Editing class also support configuring validators for the m2t direction? I think that in general, validations are not needed in this direction but maybe we should support this for completeness? * Is the naming of the validators/converters OK? So far, I've used more concise names which do not include the t2m/m2t part and the names are more based on the type of value (target/model) to which they are applied rather than when they are applied as is done on the update strategies. * Since the Editing class can be used for configuring value bindings but also for list and set bindings, it's difficult to state in the JavaDoc when exactly a converter/validator is applied since the different update strategies do not all define the same update pipeline. So e.g. when configuring an UpdateListStrategy, for now, only a converter will be set on the update strategies since no validations are supported on list bindings so far. * The Editing classes currently have built-in support for common model-validations like integer-range-validations or regex-string-validations. Here, an option could be to drop this API and say that clients must implement their own model-validators which are often not hard to write. However, I personally think that it would be nice to provide some kind of support for the more common model-validations.
Well, I guess my vacation is over... :( It would be great if someone would maybe have some time to look at the last patch. The interesting parts so far are actually only the new Editing classes and the snippets (the changes to the internal validators/converters are not really clean for now and IMO they are less relevant for the time being).
I've been thinking that the model constraints which are currently directly supported by the individual Editing classes (like IntegerEditing#range(), StringEditing#matches()) might also be moved to separate *Constraints classes. This would help separating the model validations from the actual parsing/displaying of the model values since those are actually two (mostly) independent things. You would then e.g. have an IntegerConstraints class which supports aggregating multiple validations to apply to an integer value. This would look like the following: IValidator validator = new IntegerConstraints() .required() .rangeMessage("Value is outside [{0}, {1}].") // custom validation message .range(1, 100) .addValidator(new MyIntegerValidator()) .aggregationPolicy(Constraints.AGGREGATION_MAX_SEVERITY) .createValidator(); In the *Editing classes you would then configure the model validations on such a Constraints object instead of on the Editing object itself, something like: IntegerEditing intEditing = IntegerEditing.withDefaults(); intEditing.modelConstraints() // returns an IntegerConstraints instance .required() .range(1, 100);
My notes while reviewing the patch: * There is a lot of noise at the beginning of the patch due to autoformatting of existing files. It would be lots easier to review without that noise. As this feature evolves it would be helpful if you could revert the whitespace-only changes before attaching each patch. It's fairly simple to do this in the compare editor: open the synchronize view, synchronize the sources with all CVS so all changed files show up, then use Ctrl-. and Ctrl-, to navigate forward and backware between changes. Saving in the compare editor does not invoke the auto-format save actions. This will make the patches easier to review, and will also speed up IP review once we get to that point. After IP approval is granted I will go ahead and autoformat each changed file before committing, just so we don't have this problem in the future on these same files. Eventually we'll get all the files reformatted and this won't be a problem any more. * The default date formatters / parsers were changed from static to per instance--can we make these ThreadLocals instead? I'm guessing these are a bit too expensive to be creating new ones for every instance. * BindingMessages.formatStringValue is not documented correctly--it references the argument as the "key" but it is actually a message pattern. * The formatStringValue method needs a name more distinct from formatString. Maybe formatPattern? It seems to me that the existing formatString method itself could have been named better to indicate that the argument is a message key rather than a pattern. Maybe we should deprecate it and add a formatMessage (formatKeyedMessage?) method to replace it? * StringToIntegerValidator.new(NumberFormatConverter converter, String parseErrorMessage, String outOfRangeMessage): are these string arguments message keys, patterns, or strings to present directly to the user? Please phrase your answer in javadoc form. :) * I don't like all these final fields and constructor overloading in AbstractStringToNumberValidator--maybe these should be changed to mutable properties with setters returning <this>? * We need to settle on a set of nouns to represent each type of string wrt internationalization and message formatting, and document everything precisely: e.g. key (translation key), message pattern (unformatted message), message (formatted pattern). Variable names and javadocs should be updated to reflect whatever nomenclature we decide to use. * DateRangeValidator: "// TODO: Externalize" on line 25 or so needs to be done before we can commit this. Right now we only have an English translation anyway so it won't be difficult. Note to self: the current patterns in messages.properties are not very friendly looking to non-computer people. * Fix your copyright attribution comments. It looks like all new files in the patch are attributed to IBM Corporation instead of to you. * StringEditing: suggest we rename stripped method to trimmed to match String.trim() semantics. * StringEditing.requiredMessage: rename to requiredWithMessage? Also, is this a message key, a pattern or a raw message? * StringEditing.matchesMessage: ditto comments for requiredMessage * NonNullValidator: externalize message strings * StringRegexValidator: use the word "pattern" instead of "regular expression" to avoid scaring typical users? * StringRegexValidator: externalize message strings * CompositeValidator: "TODO: Define an aggregation strategy" needs to be looked at. Also I think there is a bug somewhere where we discuss implementing an "and-validator" and an "or-validator". Just mentioning since there is some overlap. * IntegerEditing.withDefaults you've documented that the argument "outOfRangeMessage" can be parameterized with Integer.MIN_VALUE and Integer.MAX_VALUE but I have no idea what you mean here. * IntegerEditing: s/(something)Message/(something)WithMessage/ (?) * StringStripConverter: is there a reason you're not using String.trim()? Character.isWhitespace() is more exact I suppose but String.trim() is a lot faster. * MANIFEST.MF: We don't import the com.ibm.icu bundle directly, this needs to be changed to an Import-Package and reference the minimum package version. * Snippet035Editing: creating a binding based solely on an Editing seems common enough to warrant adding binding support directly to Editing, e.g. Editing.bindValue(DataBindingContext, IObservableValue targetObservable, IObservableValue modelObservable) This looks really nice! It's apparent you've put a lot of care and thought into this, and you've been quite thorough. The problem domain appears to be quite well factored--the EditingClass hierarchy is particularly impressive. Well done! Boris and I have discussed this with you before, but I wanted to ask again: would you be interested in joining the DataBinding project as a committer? (In reply to comment #18) > I've been thinking that the model constraints which are currently directly > supported by the individual Editing classes (like IntegerEditing#range(), > StringEditing#matches()) might also be moved to separate *Constraints classes. > This would help separating the model validations from the actual > parsing/displaying of the model values since those are actually two (mostly) > independent things. Could you outline which responsibilities would remain with the Editing classes instead of being moved to the *Constraints classes?
(In reply to comment #19) Matthew, thanks a lot for your extensive review of the patch! As a general remark, the patch was more about getting the Editing API right so that's the reason why some changes to the internal validators/converters were not always clean (actually, you have found all the bad things :) ). In any case, all the validators/converters (also the new ones) are still internal for now. Before making them public API (if it's necessary), IMO there would still be some clean up to do. Here are some comments about your feedback (more will follow): > * The default date formatters / parsers were changed from static to per > instance--can we make these ThreadLocals instead? I'm guessing these are a bit > too expensive to be creating new ones for every instance. Actually, the formats were already per instance before and there was a TODO indicating to share them. I think that making the default formats ThreadLocals or synchronizing the access to them would be a good idea. In fact, the StringToNumberParser already synchronizes the access to the NumberFormat so we could do it analogously for dates. What I also wanted to do in this respect is introducing some caching of validators/converters in the Editing classes (at least for the withDefaults() methods) in order to achieve similar caching as in the UpdateStrategy class. > * The formatStringValue method needs a name more distinct from formatString. > Maybe formatPattern? It seems to me that the existing formatString method > itself could have been named better to indicate that the argument is a message > key rather than a pattern. Maybe we should deprecate it and add a formatMessage > (formatKeyedMessage?) method to replace it? Indeed, this might be a bit confusing. Already the ResourceBundle#getString(String key) method is not particularly expressive but if we want to be consistent with that, I could think of the following naming: BindingMessages#getString(String key) BindingMessages#getFormattedString(String key, Object[] arguments) BindingMessages#formatMessage(String pattern, Object[] arguments) BTW, this is no public API so we could simply rename the methods. > * I don't like all these final fields and constructor overloading in > AbstractStringToNumberValidator--maybe these should be changed to mutable > properties with setters returning <this>? I think that having immutable validators/converters has its advantages but I agree that mainly the possibility to customize the validation message might lead to too many constructors. So maybe we could introduce such setter methods for the validation messages. > * We need to settle on a set of nouns to represent each type of string wrt > internationalization and message formatting, and document everything precisely: > e.g. key (translation key), message pattern (unformatted message), message > (formatted pattern). Variable names and javadocs should be updated to reflect > whatever nomenclature we decide to use. I think that in the public API, everything should be a message pattern (unformatted message) as it is right now in the Editing classes. Internally, the distinction should indeed be clearer than it is right now. I think that your proposed naming is good. > * StringRegexValidator: use the word "pattern" instead of "regular expression" > to avoid scaring typical users? Yes, StringPatternValidator might be better but note that this class is internal for now (it's "accessed" through StringEditing#matches()). > * CompositeValidator: "TODO: Define an aggregation strategy" needs to be looked > at. Also I think there is a bug somewhere where we discuss implementing an > "and-validator" and an "or-validator". Just mentioning since there is some > overlap. With aggregation strategy I was more thinking about the option to merge the individual validation statuses into a MultiStatus or to only take the most severe one (MERGED/MAX_SEVERITY). However, the OR-ing of validations might also be useful in practice so we might try to support this as well (I'm not sure yet how to do this). > * IntegerEditing.withDefaults you've documented that the argument > "outOfRangeMessage" can be parameterized with Integer.MIN_VALUE and > Integer.MAX_VALUE but I have no idea what you mean here. Since outOfRangeMessage is intended to be a message pattern, this means that in the pattern you might use {0} and {1} in order to include the MIN_VALUE and MAX_VALUE in your validation message, if desired. > * IntegerEditing: s/(something)Message/(something)WithMessage/ (?) The <constraint>Message methods are simply setter methods for the validation message (a message pattern) to use for subsequent applications of the corresponding <constraint>. Why do you think that the methods should be called <constraint>WithMessage? > * StringStripConverter: is there a reason you're not using String.trim()? > Character.isWhitespace() is more exact I suppose but String.trim() is a lot > faster. Actually, I'm used to the StringUtils class of the apache-commons library which provides both a strip() and a trim() method (the latter simply calls String#trim()). I've implemented the semantics of StringUtils#strip() since that's what I use in practice but trim() seems fine as well. My original intent was to support both but maybe trim() would be enough. > * Snippet035Editing: creating a binding based solely on an Editing seems common > enough to warrant adding binding support directly to Editing, e.g. > Editing.bindValue(DataBindingContext, IObservableValue targetObservable, > IObservableValue modelObservable) I think that's a good idea. I have also been thinking about providing a BindingBuilder similar to the one in bug #203492 which works more in terms of Editing objects than of UpdateStrategies and which also allows you to keep some state like the default update policies. However, I'm not sure whether something like that should be provided as part of the DataBinding framework or whether clients should write it themselves (it shouldn't be too difficult).
Created attachment 145997 [details] patch without the noise due to auto formatting Here's the patch again but without the noise due to auto formatting.
(In reply to comment #19) > (In reply to comment #18) > > I've been thinking that the model constraints which are currently directly > > supported by the individual Editing classes (like IntegerEditing#range(), > > StringEditing#matches()) might also be moved to separate *Constraints classes. > > This would help separating the model validations from the actual > > parsing/displaying of the model values since those are actually two (mostly) > > independent things. > > Could you outline which responsibilities would remain with the Editing classes > instead of being moved to the *Constraints classes? Only the methods which apply the actual model constraints (IntegerEditing#range(), StringEditing#matches(), ...) and their respective <constraint>Message methods would be moved to the Constraints classes. That way, the Editing classes would only contain API for setting up the parsing/displaying of a model value based on a String (i.e. they essentially configure the t2m-afterGetValidator and both converters). In addition, the individual Editing classes would expose properly configured Constraints classes which would then be used to apply the model validations instead of doing so directly on the Editing instance. E.g. the IntegerEditing class would provide the following methods exposing adequate Constraints objects: StringConstraints targetStringConstraints(); IntegerConstraints modelIntegerConstraints(); IntegerConstraints beforeSetModelIntegerConstraints(); The reason why I think that the Editing classes should expose adequate Constraints objects instead of totally decoupling those two classes is that in model validators you often need to include some model value in the validation message (think of the bounds of a range constraint) so you need to convert a model to a string. Since the Editing class already knows how to convert the model to a string, the same converter can be used for the Constraints object as well without having to specify this conversion again. As an example, using the Editing classes only, you would configure an Editing object as follows: IntegerEditing editing = IntegerEditing.forFormat(myIntFormat).required().range(1, 100); With the Constraints classes, you would achieve the same as follows: IntegerEditing editing = IntegerEditing.forFormat(myIntFormat); editing.modelIntegerConstraints().required().range(1, 100); The drawback of the second version is that it is a bit more verbose and that you cannot configure the Editing object along with model constraints in a single run using method chaining anymore. The reason why I have been thinking about separating the model validations from the parsing/displaying of models is that currently the only way to e.g. access the integer validations is through an IntegerEditing object. However, the IntegerEditing object is specific to a string-integer binding and cannot be used e.g. for integer-integer bindings for which you might also want to specify some validations.
Created attachment 146005 [details] patch for Editing/Constraints API Here's a patch illustrating how the Editing/Constraints API might look like.
Bug 197807 has a patch with similar features in it
Snippet035Editing: EditingFactory.forDate() should use the default Locale to determine date parsing / formatting instead of hard coding the date formats. I'm in the USA where we use "MM.dd.yyyy" as the date format (it's backwards, I know) so any dates I typed past the 12th of any month (in my locale-specific format) were changing to the wrong month/year when interpreted in "dd.MM.yyyy" format. There should also be a EditingFactory.forDate(Locale) method as well so apps can adapt to different locales per user. Regarding separating out the constraints API from the Editing class: * Without method return type covariance in Java 5, we have to duplicate several methods for type safety e.g. Editing.modelConstraints() : Constraints vs DateEditing.modelDateConstraints() : DateConstraints, these two methods return the same object but we need the second for type safety. Depending on what we decide on the next item we may want to put this patch in a new bundle that requires Java5. * In the older patch we could say EditingFactory.forDate().afterEqual(today) and pass that directly to some API. Now we have to store the DateEditing in a local variable, and call dateEditing.modelDateConstraints() to add constraints, then we need a new statement in which to pass the editing to the API. Everyday usage has gotten more verbose than it needs to be. I like the separation of concerns, but in this case I would still favor duplicating the constraint methods in the corresponding *Editing classes--the Editing class would just parlay the constraint to modelConstraints(). This way we can keep the 99% case boilerplate-free. Now that I understand the *Message methods better (e.g. StringConstraints.requiredMessage(String)) I'm not liking the separation of the message configuration from the constraint configuration, and the fact that the message has to be set before adding the constraint: // Right new StringConstraints().requiredMessage(REQUIRED_EMAIL_MESSAGE).required() // Wrong -- constraint will use preprogrammed message instead new StringConstraints().required().requiredMessage(REQUIRED_EMAIL_MESSAGE) Logically speaking I think the constraint and the message for that constraint should be provided together in the same method call. This is how I originally assumed that the API worked, hence my suggestion to change the method to requiredWithMessage(String) to make client code more self-explanatory without having to look at javadocs: new StringConstraints().requiredWithMessage(REQUIRED_EMAIL_MESSAGE) StringConstraints could probably use a max/min length constraint e.g. for binding to columns in a database table. new StringConstraints().maxLength(15); We also need to think about attaching a severity level to each constraint, so that soft constraints can be displayed as a warning or info message instead of a blocking error.
One more: Snippet036EditingTable doesn't execute in my workspace because the Person class is private and can't be reflected. There are also several "unused method" errors until Person is made public.
(In reply to comment #25) > Regarding separating out the constraints API from the Editing class: > * Without method return type covariance in Java 5, we have to duplicate several > methods for type safety e.g. Editing.modelConstraints() : Constraints vs > DateEditing.modelDateConstraints() : DateConstraints, these two methods return > the same object but we need the second for type safety. Depending on what we > decide on the next item we may want to put this patch in a new bundle that > requires Java5. Indeed, this is not particularly nice and Java5 would help at this point. > * In the older patch we could say EditingFactory.forDate().afterEqual(today) > and pass that directly to some API. Now we have to store the DateEditing in a > local variable, and call dateEditing.modelDateConstraints() to add constraints, > then we need a new statement in which to pass the editing to the API. Everyday > usage has gotten more verbose than it needs to be. I like the separation of > concerns, but in this case I would still favor duplicating the constraint > methods in the corresponding *Editing classes--the Editing class would just > parlay the constraint to modelConstraints(). This way we can keep the 99% case > boilerplate-free. The ability to configure an Editing instance in a single run using method chaining is indeed very handy and, to me, this is the main drawback of the Editing/Constraints approach so adding those methods on the Editing classes seems a good idea to me. > Now that I understand the *Message methods better (e.g. > StringConstraints.requiredMessage(String)) I'm not liking the separation of the > message configuration from the constraint configuration, and the fact that the > message has to be set before adding the constraint: The reason why I have separated the message configuration from the constraint configuration is that if you're not happy with a default validation message and you want to customize it, you can easily do so the way it's done in the EditingFactory of the snippets. There, you basically configure your custom validations messages and then pass out the Editing object on which the necessary constraints are applied in a second step. If the configuration of the message and the actual constraint would be coupled, you would have to specify custom validation messages every time you apply a constraint to an Editing object which I think would be rather cumbersome. > StringConstraints could probably use a max/min length constraint e.g. for > binding to columns in a database table. Yes, I would also add the constraints minLength, maxLength, and maybe also lengthRange. There might also be other common constraints which we want to support. > We also need to think about attaching a severity level to each constraint, so > that soft constraints can be displayed as a warning or info message instead of > a blocking error. I guess that for the type of constraints supported so far you will typically want to issue an error but if we want to support this, we could simply overload the constraint-method and pass the severity level as well (as opposed to configuring the severity level separately). Something like: new IntegerConstraints().range(1, 100, IStatus.WARNING);
(In reply to comment #20) > > * I don't like all these final fields and constructor overloading in > > AbstractStringToNumberValidator--maybe these should be changed to mutable > > properties with setters returning <this>? > I think that having immutable validators/converters has its advantages but I > agree that mainly the possibility to customize the validation message might > lead to too many constructors. So maybe we could introduce such setter methods > for the validation messages. I actually spotted one more place that might become a problem for future API evolution: The Editing constructor accepts a Constraint for model, target and modelBeforeSet. If in the future we decided to add targetBeforeSet then we have to add a new constructor, and deprecate the old one so that clients are encouraged to port to the new one and to provide a properly typed Constraint object. I was thinking it might be appropriate to have createTargetConstraint() and createModelConstraint() template methods and make change Editing.new() to a no-arg constructor. > > * StringRegexValidator: use the word "pattern" instead of "regular expression" > > to avoid scaring typical users? > Yes, StringPatternValidator might be better but note that this class is > internal for now (it's "accessed" through StringEditing#matches()). Actually I was talking about the regex validator's error message; the StringRegexValidator class name itself is fine. > > * CompositeValidator: "TODO: Define an aggregation strategy" needs to be > looked > > at. Also I think there is a bug somewhere where we discuss implementing an > > "and-validator" and an "or-validator". Just mentioning since there is some > > overlap. > With aggregation strategy I was more thinking about the option to merge the > individual validation statuses into a MultiStatus or to only take the most > severe one (MERGED/MAX_SEVERITY). However, the OR-ing of validations might also > be useful in practice so we might try to support this as well (I'm not sure yet > how to do this). With OR-ing of validators I'd say you take the status with the lower severity. Although let's hold off on this until somebody asks for it--I haven't seen a use case presented yet so let's not worry about it for now. > > * IntegerEditing.withDefaults you've documented that the argument > > "outOfRangeMessage" can be parameterized with Integer.MIN_VALUE and > > Integer.MAX_VALUE but I have no idea what you mean here. > Since outOfRangeMessage is intended to be a message pattern, this means that in > the pattern you might use {0} and {1} in order to include the MIN_VALUE and > MAX_VALUE in your validation message, if desired. In the javadoc you specifically reference **Integer**.MIN_VALUE and **Integer**.MAX_VALUE (which have precise, fixed values), rather than a generic min and max value of a configurable range. The javadoc should explicitly set forth how the message pattern may be parameterized--your explanation quoted above would probably be sufficient if we just put it in the javadoc. > > * StringStripConverter: is there a reason you're not using String.trim()? > > Character.isWhitespace() is more exact I suppose but String.trim() is a lot > > faster. > Actually, I'm used to the StringUtils class of the apache-commons library which > provides both a strip() and a trim() method (the latter simply calls > String#trim()). I've implemented the semantics of StringUtils#strip() since > that's what I use in practice but trim() seems fine as well. My original intent > was to support both but maybe trim() would be enough. Now that I've had a chance to look into the matter, I can see an argument in favor of both trim() and strip(). It works fine for those of us who only deal with the ASCII subset of Unicode, but is lame for almost everybody else. So we'd probably better implement both trimmed() and stripped(). > > * Snippet035Editing: creating a binding based solely on an Editing seems > common > > enough to warrant adding binding support directly to Editing, e.g. > > Editing.bindValue(DataBindingContext, IObservableValue targetObservable, > > IObservableValue modelObservable) > I think that's a good idea. I have also been thinking about providing a > BindingBuilder similar to the one in bug #203492 which works more in terms of > Editing objects than of UpdateStrategies and which also allows you to keep some > state like the default update policies. However, I'm not sure whether something > like that should be provided as part of the DataBinding framework or whether > clients should write it themselves (it shouldn't be too difficult). Let's just start with some new methods on Editing and see how far that takes us: public Binding bindValue(DataBindingContext, IObservableValue target, IObservableValue model) public Binding bindList(DataBindingContext, IObservableList target, IObservableList model) public Binding bindSet(DataBindingContext, IObservableSet target, IObservableSet model) With these methods it's possible that binding builders would actually become moot.
(In reply to comment #28) > Let's just start with some new methods on Editing and see how far that takes > us: > > public Binding bindValue(DataBindingContext, IObservableValue target, > IObservableValue model) > public Binding bindList(DataBindingContext, IObservableList target, > IObservableList model) > public Binding bindSet(DataBindingContext, IObservableSet target, > IObservableSet model) > > With these methods it's possible that binding builders would actually become > moot. I was thinking about also providing bind(Value|List|Set) methods where you can customize the update policies and also pass-in update strategies which are then configure by the editing object. So for value bindings, you would have the following methods (the same for list/set bindings): public final Binding bindValue(DataBindingContext dbc, IObservableValue targetObservableValue, IObservableValue modelObservableValue); public final Binding bindValue(DataBindingContext dbc, IObservableValue targetObservableValue, IObservableValue modelObservableValue, int t2mUpdatePolicy, int m2tUpdatePolicy); public final Binding bindValue(DataBindingContext dbc, IObservableValue targetObservableValue, IObservableValue modelObservableValue, UpdateValueStrategy t2mUpdateStrategy, UpdateValueStrategy m2tUpdateStrategy); I think that at least the one with the custom update policies may be common in practice. What do you think?
Sounds good, but let's be very explicit in the javadoc about what changes the Editing object may make to a passed-in UpdateValueStrategy. e.g. will it override the converter if one is already in place?
(In reply to comment #28) > I actually spotted one more place that might become a problem for future API > evolution: The Editing constructor accepts a Constraint for model, target and > modelBeforeSet. If in the future we decided to add targetBeforeSet then we > have to add a new constructor, and deprecate the old one so that clients are > encouraged to port to the new one and to provide a properly typed Constraint > object. I was thinking it might be appropriate to have > createTargetConstraint() and createModelConstraint() template methods and make > change Editing.new() to a no-arg constructor. Actually, I had already tried to do something similar but the problem I encountered was that e.g. the IntegerEditing class passes on the display NumberFormat to the IntegerConstraints object it creates. If you do the creation through a createModelConstraint() method, you must have stored the NumberFormat as an instance field of the IntegerEditing class to have it available. However, the problem is that the createModelConstraint() must probably be called inside the constructor of the Editing class and the IntegerEditing class is not totally initialized at this point (you would not have stored the NumberFormat yet).
*** Bug 197807 has been marked as a duplicate of this bug. ***
(In reply to comment #31) > Actually, I had already tried to do something similar but the problem I > encountered was that e.g. the IntegerEditing class passes on the display > NumberFormat to the IntegerConstraints object it creates. If you do the > creation through a createModelConstraint() method, you must have stored the > NumberFormat as an instance field of the IntegerEditing class to have it > available. However, the problem is that the createModelConstraint() must > probably be called inside the constructor of the Editing class and the > IntegerEditing class is not totally initialized at this point (you would not > have stored the NumberFormat yet). Maybe lazy-init the constraints fields? You'd just have to be careful to direct all modifications through constraints through the modelConstraints() or targetConstraints() fields so they are guaranteed to be non-null.
(In reply to comment #33) > (In reply to comment #31) > > Actually, I had already tried to do something similar but the problem I > > encountered was that e.g. the IntegerEditing class passes on the display > > NumberFormat to the IntegerConstraints object it creates. If you do the > > creation through a createModelConstraint() method, you must have stored the > > NumberFormat as an instance field of the IntegerEditing class to have it > > available. However, the problem is that the createModelConstraint() must > > probably be called inside the constructor of the Editing class and the > > IntegerEditing class is not totally initialized at this point (you would not > > have stored the NumberFormat yet). > > Maybe lazy-init the constraints fields? You'd just have to be careful to > direct all modifications through constraints through the modelConstraints() or > targetConstraints() fields so they are guaranteed to be non-null. You would also have to be careful to initialize the Editing class (e.g. setting the mentioned NumberFormat in the IntegerEditing class) before you add any constraint. It's maybe not very nice but it should work.
Great to see the progress on this. Let me know when you've converged and need another reviewer.
Created attachment 146551 [details] new Editing/Constraints API patch Here's an updated patch which includes the following relevant changes: * Support trim() in addition to strip() on StringEditing. * Added the following StringConstraints: minLength, maxLength, lengthRange, nonEmpty. * Added the Editing#bind(Value|List|Set) methods. * Added the Editing#create(Target|Model|BeforeSetModel)Constraints() methods along with a lazy initialization of those constraints objects instead of passing them directly to the constructor. * The default Editng() constructor is now protected and clients instead use the new static Editing#create() method to instantiate a plain Editing object. That way, it's more "consistent" with subclasses which also use static factory methods to instantiate the objects. * Added the static creation method Editing#withConverters(IConverter t2mConverter, IConverter m2tConverter) which can be used to create fully configured custom Editing objects without the need for subclassing. * Added the convenience methods for directly applying model validators on the *Editing classes (the calls are delegated to the *Constraints classes) without loosing the ability to use method chaining. I've only added the actual constraints methods to the *Editing classes but not the methods for setting the corresponding validation messages in order to keep the number of convenience methods somewhat limited. Since the validation messages should typically only be called in the EditingFactory, I guess that's fine that way. Note also that the convenience methods always apply the validators as model constraints (and not as before-set model constraints). I guess that's fine as well since before-set validations are typically only used for expensive validations if at all.
I have been a bit busy last week but here we go again. (In reply to comment #28) > With OR-ing of validators I'd say you take the status with the lower severity. > Although let's hold off on this until somebody asks for it--I haven't seen a > use case presented yet so let's not worry about it for now. Actually, when OR-ing validators you will typically also need a custom validation message along with a custom parameterization so I don't think that we should try to support something like this as a whole. However, the actual OR-ing of validators might be useful for some constraints which are not directly supported, e.g. if you want to check that a number lies either within [a,b] or within [c,d] or that a number is either <x or >y. So if we supported a simple OR-ing, clients could easily write simple validators which simply delegate the actual validation to the OR-ed Constraints object and then returns the custom validation message in case of error. However, I agree that this is not that important for now so let's think about this a bit later. > > > * IntegerEditing.withDefaults you've documented that the argument > > > "outOfRangeMessage" can be parameterized with Integer.MIN_VALUE and > > > Integer.MAX_VALUE but I have no idea what you mean here. > > Since outOfRangeMessage is intended to be a message pattern, this means that > in > > the pattern you might use {0} and {1} in order to include the MIN_VALUE and > > MAX_VALUE in your validation message, if desired. > > In the javadoc you specifically reference **Integer**.MIN_VALUE and > **Integer**.MAX_VALUE (which have precise, fixed values), rather than a generic > min and max value of a configurable range. The javadoc should explicitly set > forth how the message pattern may be parameterized--your explanation quoted > above would probably be sufficient if we just put it in the javadoc. I'm not sure I fully understand what you mean here. The "outOfRangeMessage" is the already existing error message issued by the AbstractStringToNumberValidator class in case a parsed integer is outside the value range of integers. It's not about the general IntegerConstraints#range(int min, int max) constraint or the like but it's really about the integer range [Integer.MIN_VALUE, Integer.MAX_VALUE]. In the JavaDoc, I meant that you may reference Integer.MIN_VALUE and Integer.MAX_VALUE by parameterizing the message with {0} and {1}, respectively. By default, the validation message including the range bounds looks as follows: "Please enter a value between [-2'147'483'648] and [2'147'483'647] and with a similar format."
Created attachment 147262 [details] ongoing Editing/Constraints API patch It's still work in progress but here are some relevant changes since the last patch: * Externalized all the validation strings. * Added Editing/Constraints classes for booleans. * Reduced constructor overloading in internal validator classes by using setter methods for custom validation messages. * Synchronize access to NumberFormat/DateFormat instances in validators/converters (only for the new ones for now).
Created attachment 149314 [details] ongoing Editing/Constraints API patch Further changes: * EditingFactory.forDate() now respects the locale for the date component separators and the order of the date components. * Added locale specific methods EditingFactory.forDate(Locale) and EditingFactory.forInteger(Locale). * Clearly state what parameterization is supported on validation messages (see *Constraints classes). * The new validators are now thread safe. * JavaDoc extension/clean up. One big question I have is the following: For now, there is support for the editing/validation of int's (IntegerEditing/IntegerConstraints). Since the same editing/constraints support should also be added for long's, short's, char's, byte's, I'm not sure how to do this, i.e. whether to copy the IntegerEditing/IntegerConstraints classes for every type of integer number or to try to use the same class for all of them? If trying to use the same Editing/Constraints class for all integer types, I think that you would still need type-specific factory methods (withDefaults(), forFormat(), etc.) on the Editing class and you would have to change all the types in the Constraints class to the most general integer type "long". I personally would be fine with providing separate classes for every integer type since the actual implementation of the Editing/Constraints classes are very simple (most methods are just one-liners) i.e. copying them would not be too bad. Thoughts?
Created attachment 149326 [details] new patch including support for multiple integer/decimal types This patch now includes support for the individual integer types (long, int, short, byte) and also for the decimal types double and float. So far, I've taken the approach to support multiple types with the same pair of Editing/Constraints classes (instead of having dedicated classes for every single numeric type) to see what it would look like. More concretely, the following new API classes are included in the patch: * IntegerEditing/IntegerConstraints: Supports all the integer number types (long, int, short, byte). The IntegerEditing class now has factory methods for the individual integer types (with(Long|Integer|Short|Byte)Defaults(), ...) and the IntegerConstraints class uses long's in its API in order to accommodate all the integer types. * DecimalEditing/DecimalConstraints: Supports the decimal types double and float. Analogously, the DecimalEditing class has dedicated factory methods for doubles/floats and the DecimalConstraints class works in terms of doubles. Such a grouping of integer and decimal types has the advantage that you end up with less classes/code as opposed to having a pair of *Editing/*Constraints classes for every numeric type. On the other hand, the drawback of this approach is that sometimes the API is a bit loose. I'm not yet sure which approach is better.
Created attachment 151361 [details] cleaned up version of the previous patch I've done some cleanup of the previous patch and I think that it is now in a state in which we could discuss whether the general approach taken so far might the right one or not. I think it's better to discuss the current set of Editing/Constraints classes before adding more since implementing additional ones should be rather straightforward once we take a specific direction. The API classes included in the current patch are the following: * Editing/Constraints: Base classes for editing/validation * IntegerEditing/IntegerConstraints (supports the types long, int, short, byte) * DecimalEditing/DecimalConstraints (supports the types double, float) * StringEditing/StringConstraints * BooleanEditing/BooleanConstraints * DateEditing/DateConstraints Other new classes (mainly additional validators/converters) are all internal. From my point of view, there are the following main points which may require some further discussion: * The Editing classes support the full update pipeline (all validators, converter) in the target-to-model direction but only a converter (no validators) in the model-to-target direction. I personally have never encountered a situation in which I needed a validator in the m2t update pipeline. Does anyone think that there is some need to support validators in the m2t direction as well? * The Editing class defines the methods (target|model|beforeSetModel)Constraints() which are declared to return a plain Constraints object. Since subclasses need to expose more specific Constraints types in their interfaces, additional methods need to be introduced which delegate to the methods of the supertype and which cast the return type accordingly (e.g. IntegerEditing#modelIntegerConstraints() returning an IntegerConstraints instance). This is a limitation of Java 1.4 which might not be really nice (but maybe also not too bad?). * The individual integer types (long, int, short, byte) are supported by the same pair of IntegerEditing/IntegerConstraints classes by defining the interface in terms of long's which accomodate all the integer types (the same is done for doubles and floats in the DecimalEditing/DecimalConstraints classes, see comment #40 for more details). This is done in order to avoid having to define basically identical editing/constraints for the individual integer/decimal types. * The Editing subclasses offer convenience methods for directly applying model validators on the Editing object using method chaining (see the last point in comment #36 for more details). This was done since this probably greatly simplifies the most typical usage of the API by supporting the full configuration of an Editing object and its Constraints using method chaining. The possible drawback is that code you end up with a larger code footprint. Feedback on the above points (or others) is welcome.
Created attachment 152082 [details] added basic support for the OR'in of constraints I've just added an additional AGGREGATION_MIN_SEVERITY aggregation policy to the Constraints class which results in returning the validation status with the *lowest* severity. Using this new aggregation policy, clients can easily write their own validators which do an OR'ing of a set of validations accumulated in a Constraints object since taking the validation status with the minimum severity has the effect of OR'ing the validators. I think that this could be a very simple way to provide basic support for the OR'in of validations to clients. As an example, I've included a RangesValidator in the Snippet035Editing snippet which checks that a given value is within one of two given ranges. Beyond the addition of that aggregation policy (and the RangesValidator in the snippet), the patch is the same.
Created attachment 153890 [details] added support for BigDecimals, BigIntegers, Characters, Number-To-Number-Editing This patch now also includes Editing/Constraints classes supporting the editing of BigDecimals, BigIntegers, and Characters based on a string. I have also added a NumberToNumberEditing class which supports the validation/conversion between different combinations of numbers. This is something which is already supported by the existing UpdateStrategies.
Boris, do you mind if I take over this bug?
Ovidio, congratulations on your committer election, and welcome to the project! Assigning this bug to you now. :) I finally finished reviewing the patch! Here are my notes: Methods with parameterized message strings should document specifically that {0} refers to X, {1} refers to Y, etc. e.g. IntegerConstraints.rangeMessage(String) should document that {0} is the minimum value in the range, and that {1} is the maximum value in the range. ObservableMapEditingSupport: Consider ValuePropertyEditingSupport in addition to or in place of this class. ObservableMapEditingSupport.handleValidation: internationalize the string "Validation Error" Snippet035Editing: Awesome. EditingFactory: Internationalize static final strings Not sure if the value for DATE_DISPLAY_PATTERN should be the default pattern. Static factory methods that take an optional locale--very nice I had a thought that EditingFactory could change static methods to instance methods, and by doing so we make it possible to dynamically configure an editing factory. This allows standard configurations to be passed around instead of enforced statically everywhere. forDouble(Locale) unexpected uses the static fields INTEGER_PRASE_ERROR_MESSAGE and INTEGER_OUT_OF_RANGE_METHODS. Integer vs double configure(DecimalEditing) unexpected uses static field INTEGER_RANGE_MESSAGE NumberToNumberValidator: I'm seeing a common duality between e.g. outOfRangeMessage and formattedOutOfRangeMessage. This is begging for some kind of abstraction, to simplify every case where we have two fields that are related like this. StringEditing: stripped(boolean): Introduce a separate "strippedToNull" method instead of boolean "toNull" argument. I don't care for boolean arguments to methods unless the method name makes it self-explanatory e.g. setVisible(boolean) is obvious, whereas layout(boolean) makes no sense unless you look at the docs. trimmed(boolean): Ditto for separate "trimmedToNull" method. CharacterConstraints: We need some kind of pattern or abstraction around all these private message fields. A lot of concepts are duplicated in many place because of the validation message/constraint separation. Rename "letter" method to "alpha"? Rename "digit" method to "numeric"? Rename "letterOrDigit" method to "alphaNumeric"? Ditto the above for CharacterEditing StringLengthValidator: "length" in method names is redundant, it is already implied by the class name. Therefore minLength => min, maxLength => max, lengthRange => range DecimalConstraints: rename decimalDisplayFormat() to just "decimalFormat" or "numberFormat" rename integerDisplayFormat() to just "integerFormat" General comment: the decoupling of validation messages from the constraints is making the API huge. BooleanConstraints: Wondering if the required() and requiredMessage(String) methods should just be pulled up to the Constraints class. BigIntegerRangeValidator: private constructor: replace integer value for LESS, LESS_EQUAL, etc with enum. Extract the messages (or message keys for internationalization) into the enum class. compare() method should check for null. Does the use of compare() imply a required() constraint? Constraints: AGGREGATION_MERGED, AGGREGATION_MAX_SEVERITY, AGGREGATION_MIN_SEVERITY should be extracted to an enum. Will need a method validate(Object value, IValidator[] validators) Class should be documented as not thread safe. Probably all of the *Constraints classes need this. StringToBooleanValidator: DEFAULT_TRUE_VALUES, DEFAULT_FALSE_VALUES: Be careful we don't try to encompass too many values by default here--this is not necessarily the desired behavior. These fields should probably be localized, which means they can't be static nor initialized in a static initializer block. validate(): I have the sense that the strategy taken here is too simple. Consider for example an expression which can be parsed and evaluated to true or false. In such a case we can't possible provide all possible true or false values as simple strings. So the true values / false values should be treated as a single use case and extracted to a string matcher validator Editing: Possibly rename the "applyTo*Strategy" methods to follow the pattern set by UI forms: applyToM2TListStrategy => adaptModelToTargetStrategy applyConstraints should use a status aggregation policy StringConstraints: missing method "matches(java.util.regex.Pattern)" DecimalScaleValidator: Consider using ThreadLocals instead of synchronized blocks IntegerEditing: If/else chain in constructor should be extracted out into static factory methods--make these objects arguments to the constructor forLongFormat(NumberFormat, String, String) -- I'm not liking having to pass two strings into this method, in my mind these should be configurable with subsequence method calls e.g. forLongFormat(numberFormat).parseErrorMessage("foo").outOfRangeMessage("bar"). This is slightly more verbose but makes the code self-explanatory. I'm seeing methods like range(long, long) but no rangeMessage(String) which was a little surprising. In my mind we lose a lot of the benefit of shadowing the APIs from the *Constraints classes if we don't shadow the ability to configure messages as well. StringRegexValidator: Either getFormattedValidationMessage() should be synchronized, or we should use ThreadLocals, or we should just calculate the validation message in the constructor. For simplicity I'm rather leaning toward the constructor in all cases for the moment. DecimalEditing: beforeSetModelDecimalConstraints() (and Editing.beforeSetModelConstraints() in general): this should probably be targetting the after-convert phase instead of before-set, since the before-set validator is not executed with ValueUpdatePolicy.POLICY_CONVERT update strategies, unless Binding.updateTargetToModel() is called explicitly. Generally, before-set is reserved for expensive/long-running validators that should not be run until all other possible validation has passed. BigDecimalEditing: As a general rule internal classes should have protected constructors, but document them as @noreference, and tag the class as @noextend. This allows outside code to extend our implementations without requiring us to commit to long-term support. In general we've been trying to push API restrictions by policy rather than constraint. range(): in general, range validators should verify that min<=max so we on't create a constraint that's impossible to satisfy. BigIntegerConstraints: Why not combined with IntegerConstraints? bigIntegerDisplayFormat: why not just name this "numberFormat"? CharacterValidator.isValid(char) is begging for a strategy enum to replace int constants. BooleanToStringConverter FIXME: "should/may we change this to convert to a localized string?" -- why not? NumberToNumberEditing: can you provide a use case for this class? NonEmptyStringValidator.validate: check input.length()==0 instead of "".equals(input) StringStripConverter: int stripEnd = string.length() - 1; - while (stripEnd >= 0) { + while (stripEnd >= stripStart) { And there you have it. These notes are very rough, please don't take any "you should do this" type statements literally. Everything here is just suggestions for improvements based on my reactions while reading the all 132 (!!) printed pages of your patch. Boris, there's a lot of API in here that could be cleaned up by putting this patch in a separate Java5 bundle e.g. use covariant return with Editing.modelConstraints() instead of DateEditing having to introduce dateModelConstraints(), etc. Is this feasible? Ovidio: awesome work. We're almost there.
(In reply to comment #45) > Ovidio, congratulations on your committer election, and welcome to the project! > Assigning this bug to you now. :) Cool, my very own bug :) Here is some feedback for a handful of the points you have mentioned. More will follow. > I finally finished reviewing the patch! Here are my notes: Thanks a lot for the effort! > ObservableMapEditingSupport: Consider ValuePropertyEditingSupport in addition to > or in place of this class. I couldn't find the ValuePropertyEditingSupport class? > ObservableMapEditingSupport.handleValidation: internationalize the string > "Validation Error" Since the class is only part of the snippets I guess there's no need to externalize the strings? > StringLengthValidator: "length" in method names is redundant, it is already > implied by the class name. Therefore minLength => min, maxLength => max, > lengthRange => range Good idea. I've renamed them and I also renamed DecimalScaleValidator#maxScale => #max accordingly. > DecimalConstraints: > rename decimalDisplayFormat() to just "decimalFormat" or "numberFormat" > rename integerDisplayFormat() to just "integerFormat" The "Display" part seems really superfluous so I removed it. > EditingFactory: > forDouble(Locale) unexpected uses the static fields > INTEGER_PRASE_ERROR_MESSAGE and INTEGER_OUT_OF_RANGE_METHODS. Integer vs double > configure(DecimalEditing) unexpected uses static field INTEGER_RANGE_MESSAGE The error messages should equally fit for integer and for doubles but the names were a bit unfortunate. I've renamed them to NUMBER_*. Thanks for the hint. > CharacterConstraints: > Rename "letter" method to "alpha"? > Rename "digit" method to "numeric"? > Rename "letterOrDigit" method to "alphaNumeric"? I've named them that way since there are equally named methods on the Character class so I think it's better to keep the names consistent. > BigIntegerRangeValidator: > compare() method should check for null. Does the use of compare() imply a > required() constraint? The operands should never be null since the compare() is only called for non-null input numbers and the input number is only checked against existing range bounds. > Constraints: > AGGREGATION_MERGED, AGGREGATION_MAX_SEVERITY, AGGREGATION_MIN_SEVERITY > should be extracted to an enum. Will need a method validate(Object value, > IValidator[] validators) Do you mean as a static validate method? Something I had been thinking about was to add a method Constraints#applyConstraints() which applies all the accumulated validators without having to explicitly create an intermediate validator. I'm not sure whether you mean something in this direction? > StringToBooleanValidator: DEFAULT_TRUE_VALUES, DEFAULT_FALSE_VALUES: > Be careful we don't try to encompass too many values by default here--this > is not necessarily the desired behavior. > These fields should probably be localized, which means they can't be static > nor initialized in a static initializer block. Those default representations for true/false were already present in the StringToBooleanPrimitiveConverter and externalized in the messages.properties file. I've used the same in the StringToBooleanValidator to keep them the same even though I do also not like having those pre-defined defaults too much. > validate(): I have the sense that the strategy taken here is too simple. > Consider for example an expression which can be parsed and evaluated to true or > false. In such a case we can't possible provide all possible true or false > values as simple strings. So the true values / false values should be treated > as a single use case and extracted to a string matcher validator This could be a good point. Would you use the matcher as the sole mechanism or would you still keep the existing String[] approach as an option? > Editing: > Possibly rename the "applyTo*Strategy" methods to follow the pattern set by > UI forms: applyToM2TListStrategy => adaptModelToTargetStrategy > applyConstraints should use a status aggregation policy I would say that "adapt*Strategy" is just as fine as "applyTo*Strategy" so I renamed them accordingly since making the names consistent with other API is definitely good. > StringConstraints: missing method "matches(java.util.regex.Pattern)" Good point, added the necessary methods. > StringRegexValidator: > Either getFormattedValidationMessage() should be synchronized, or we should > use ThreadLocals, or we should just calculate the validation message in the > constructor. For simplicity I'm rather leaning toward the constructor in all > cases for the moment. For now, I've made the method synchronized since that's what I've done in all other validators. Doing the formatting in the constructor would simplify the code a bit but you have the slight disadvantage that MessageFormat#format(...) typically instantiates a set of temporary objects which you would then always create even if you never encounter a validation error (However, I'm not sure how much this matters...). > DecimalEditing: > beforeSetModelDecimalConstraints() (and Editing.beforeSetModelConstraints() > in general): this should probably be targetting the after-convert phase instead > of before-set, since the before-set validator is not executed with > ValueUpdatePolicy.POLICY_CONVERT update strategies, unless > Binding.updateTargetToModel() is called explicitly. Generally, before-set is > reserved for expensive/long-running validators that should not be run until all > other possible validation has passed. The method Editing#modelConstraints() already targets the after-convert validation phase and is what you will usually want to use. Note that also all the convenience methods like IntegerConstraints#required() delegate to the modelConstraints() method and *not* to beforeSetModelConstraints(). > BooleanToStringConverter > FIXME: "should/may we change this to convert to a localized string?" -- why > not? I only added the FIXME since changing this would result in a change for clients. I just wasn't sure whether this would be an undesired change of behavior for clients? > NumberToNumberEditing: can you provide a use case for this class? The only reason why I added this class is that the defaulting of validators/converter in the UpdateStrategy classes also support conversions between different number types. I personally do not have a particularly relevant use case so we could also remove that class for the time being if you like (I would be fine with that). > NonEmptyStringValidator.validate: check input.length()==0 instead of > "".equals(input) Good point since a null value would not have triggered any error with the existing code! Thanks. > StringStripConverter: > > int stripEnd = string.length() - 1; > - while (stripEnd >= 0) { > + while (stripEnd >= stripStart) { Fixed, thanks.
Created attachment 156294 [details] made some adaptations based on Matthew's review Here's an updated patch containing the changes I've mentioned in the previous comment.
Matthew, here's a second round of feedback to your review comments... (In reply to comment #45) > Methods with parameterized message strings should document specifically that {0} > refers to X, {1} refers to Y, etc. e.g. IntegerConstraints.rangeMessage(String) > should document that {0} is the minimum value in the range, and that {1} is the > maximum value in the range. What I have tried to do so far is actually stating in the JavaDoc how the messages can be parameterized. I've done this by always adding a sentence "Can be parameterized with ... XXX and YYY." where XXX and YYY are always listed in the order in which they are included in the message format. E.g. for IntegerConstraints#range(long, long), this looks like: "Can be parameterized with the min and max values of the range in which the parsed integer must lie." Matthew, do you think that it is enough to use a similar sentence while including the message format indexes in order to make the whole thing more explicit? I.e. something like the following: "Can be parameterized with the min ({0}) and max ({1}) values of the range in which the parsed integer must lie." Do you think that this would be explicit enough? > StringEditing: > stripped(boolean): Introduce a separate "strippedToNull" method instead of > boolean "toNull" argument. I don't care for boolean arguments to methods unless > the method name makes it self-explanatory e.g. setVisible(boolean) is obvious, > whereas layout(boolean) makes no sense unless you look at the docs. > trimmed(boolean): Ditto for separate "trimmedToNull" method. Having dedicated methods is indeed clearer so I have added them. BTW, something I had been thinking in this context was to support a way of chaining multiple conversions from target=>model on the StringEditing class. E.g. strippedToNull() could also be interpreted as applying two separate conversions consecutively. This could be supported by a StringConversions class by doing something like new StringConversions().stripped().emptyToNull() and then passing those StringConversions to the StringEditing class. I don't think that we should do something like this for now but if we e.g. find the need to support more conversions like e.g. converting an input string to upper case, this kind of conversions could always be combined with the existing ones. So if you e.g. wanted to convert the input string to upper case and also strip to null, you would do something like new StringConversions().stripped().toUpperCase().emptyToNull(). This would allow for a combination of conversions without resulting in an explosion of methods in the StringEditing class. But again, I would only consider something like that in case need arises. > IntegerEditing: > I'm seeing methods like range(long, long) but no rangeMessage(String) which > was a little surprising. In my mind we lose a lot of the benefit of shadowing > the APIs from the *Constraints classes if we don't shadow the ability to > configure messages as well. I had decided to only shadow the actual constraints methods partly in order to reduce the amount of shadowed API but mainly also because I think that if you use something like an EditingFactory for using the Editing API, most of the time you will actually not need to set any validation messages when actually configuring an Editing instance. IMO, the API for configuring the validation messages and display formats on a Constraints object is kind of the "boring stuff" which will very often be the same within a project. So I think that in a real project, you should really set up something like an EditingFactory which provides you with pre-configured Editing instances on which you only need to apply the necessary constraints. Matthew, do you think this makes sense? > BigDecimalEditing: > As a general rule internal classes should have protected constructors, but > document them as @noreference, and tag the class as @noextend. This allows > outside code to extend our implementations without requiring us to commit to > long-term support. In general we've been trying to push API restrictions by > policy rather than constraint. Thanks for the information. In that case, I have also removed all the "final" modifiers to the *Editing/*Constraints classes and I have applied the changes you mentioned.
Created attachment 156446 [details] further adaptations based on Matthew's review Updated patch which includes the changes mentioned in the previous comment.
(In reply to comment #45) Matthew, here is some feedback on the last few points of your review where I think that especially the last one is important so if you have any idea regarding it, I would be very glad. I hope that the explanations below are clear enough to express what I mean... As a next step, I think that I will have a closer look at the possible benefits of using Java5 for implementing the functionality provided by the patch. > IntegerEditing: > forLongFormat(NumberFormat, String, String) -- I'm not liking having to pass > two strings into this method, in my mind these should be configurable with > subsequence method calls e.g. > forLongFormat(numberFormat).parseErrorMessage("foo").outOfRangeMessage("bar"). > This is slightly more verbose but makes the code self-explanatory. I agree with you that this would be more readable but the problem I've encountered is that you need to pass those validation messages to the target-to-model validator. This is currently done directly in the constructor and the t2m validator is immediately added to the set of targetConstraints() without having to care about it anymore. However, if you had methods like the ones above which allow you to change the validation messages after having created the IntegerEditing instance, you would also have to replace the existing t2m validator with a new one which uses the changed validation messages. Since I couldn't find any clean way to do this, I've decided that the validation messages should be specified at construction time. > BigIntegerConstraints: > Why not combined with IntegerConstraints? The main reason was that without generics, you cannot have a common class for Integer and BigInteger constraints without sacrificing type safety. The same holds for BigDecimalConstraints/DecimalConstraints. What I've grouped together are the individual primitive integer types (byte, short, int, long) by having a single IntegerEditing/IntegerConstraints class which uses longs (the same for float/double). > BigIntegerConstraints: > bigIntegerDisplayFormat: why not just name this "numberFormat"? Actually, there might really be something which is not ideal regarding the xxxFormat() methods on some of the *Constraints classes. What I'm doing right now is that if you create an IntegerEditing instance like IntegerEditing.forFormat(new MyIntegerFormat()).range(1, 100) the IntegerEditing class passes the MyIntegerFormat() instance to the IntegerConstraints#integerFormat(NumberFormat) method. This has the nice side-effect that integers included in the validation messages of the IntegerConstraints class are automatically formatted using the same format as is used for actually displaying integers in the UI which IMO is what you usually want. However, if you look e.g. at the DecimalConstraints class, it has two format methods, #decimalFormat() and #integerFormat() (the latter is only needed for the formatting of the scale specified in the DecimalConstraints#maxScale(int) constraint). Here again, if you create a DecimalEditing instance like DecimalEditing.forFormat(new MyDecimalFormat()).range(1.0, 100.0) the DecimalEditing class passes the MyDecimalFormat() instance to the DecimalConstraints#decimalFormat(NumberFormat) method. However, the DecimalEditing class does not really know what to pass to DecimalConstraints#integerFormat(NumberFormat) so the integer format remains unconfigured which is not so nice. That's why I'm not totally sure whether it is a good idea having *Editing classes configure (part of) their associated *Constraints classes? I just found it very convenient to do so in most cases but there might be some problems doing so. Any ideas regarding this point?
Created attachment 157017 [details] updated patch without the NumberToNumberEditing class Again, here's an updated version of the patch which now does not include the NumberToNumberEditing class anymore. I think we should wait for a real use case before adding this API.
Created attachment 157180 [details] cleanup of format specifications for constraints and new maxPrecision constraint I have renamed the method DecimalConstraints#integerFormat(NumberFormat) to #scaleFormat(NumberFormat) since the specified format was only used for formatting the scale value of the scale constraint in validation messages (the same for StringConstraints#integerFormat() => StringConstraints#lengthFormat()). I have also adapted the JavaDoc of different *Constraints classes in order to state more clearly which formatting state they capture when they are applied. In addition, this new patch also includes a new DecimalConstraints#maxPrecision(int) constraint which allows to enforce a maximum number of significant digits on a decimal number. I think this is very handy e.g. in conjunction with the #maxScale() constraint in order to specify the overall precision and scale of a number (like is e.g. done in Oracle where you can define a number as having up to 5 significant digits and 2 fractional digits like NUMBER(5,2)).
(In reply to comment #48) > Matthew, do you think that it is enough to use a similar sentence while > including the message format indexes in order to make the whole thing more > explicit? I.e. something like the following: > "Can be parameterized with the min ({0}) and max ({1}) values of the range > in which the parsed integer must lie." > Do you think that this would be explicit enough? Yes, the important part is being explicit about which parameters are at which index. > I had decided to only shadow the actual constraints methods partly in order to > reduce the amount of shadowed API but mainly also because I think that if you > use something like an EditingFactory for using the Editing API, most of the time > you will actually not need to set any validation messages when actually > configuring an Editing instance. IMO, the API for configuring the validation > messages and display formats on a Constraints object is kind of the "boring > stuff" which will very often be the same within a project. So I think that in a > real project, you should really set up something like an EditingFactory which > provides you with pre-configured Editing instances on which you only need to > apply the necessary constraints. > Matthew, do you think this makes sense? I'm starting to wonder again whether it's a good idea to shadow all that API. It seems like we're doing it to get around Java limitations (e.g. closures would make this a non-issue) but in the process we're royally violating the Don't-Repeat-Yourself principle. I like the separation of Editing and Constraints as it currently stands. As an experiment I'd like to take the snippets you have now and see if modifying them to register all constraints through e.g. modelIntegerConstraints() instead of through IntegerEditing methods, and see if it's not as horrible as it sounds. :) (In reply to comment #50) > > BigIntegerConstraints: > > Why not combined with IntegerConstraints? > The main reason was that without generics, you cannot have a common class for > Integer and BigInteger constraints without sacrificing type safety. The same > holds for BigDecimalConstraints/DecimalConstraints. What I've grouped together > are the individual primitive integer types (byte, short, int, long) by having a > single IntegerEditing/IntegerConstraints class which uses longs (the same for > float/double). I we go the Java5 route, this could be unified by changing the method to accept a Number and let Java do the autoboxing. > That's why I'm not totally sure whether it is a good idea having *Editing > classes configure (part of) their associated *Constraints classes? I just found > it very convenient to do so in most cases but there might be some problems doing > so. Any ideas regarding this point? Dependency injection to the rescue! :) Let's allow the user to provide their own preconfigured Constraints objects to the *Editing constructors. (In reply to comment #52) > I have renamed the method DecimalConstraints#integerFormat(NumberFormat) to > #scaleFormat(NumberFormat) since the specified format was only used for > formatting the scale value of the scale constraint in validation messages (the > same for StringConstraints#integerFormat() => StringConstraints#lengthFormat()). > I have also adapted the JavaDoc of different *Constraints classes in order to > state more clearly which formatting state they capture when they are applied. In > addition, this new patch also includes a new > DecimalConstraints#maxPrecision(int) constraint which allows to enforce a > maximum number of significant digits on a decimal number. I think this is very > handy e.g. in conjunction with the #maxScale() constraint in order to specify > the overall precision and scale of a number (like is e.g. done in Oracle where > you can define a number as having up to 5 significant digits and 2 fractional > digits like NUMBER(5,2)). +1 This is coming along very nicely, we're almost there!
Created attachment 157378 [details] Remove all methods in *Editing that shadows *Constraints methods, experiment with changes in snippets Here are my observations from this experiment: * Most of the *Editing classes were reduced in size by anywhere from 1/3 to 1/2. Not to mention that a lot of duplicate API was removed * It only takes one more line of code per *Editing instance to put the constraints on the editing.model*Constraints() instead of tagged on the end of the Editing expression. In every instance the editing object was stored to a local variable, so accessing the model constraints object was trivial. So I think I've changed my mind from my suggestion several months ago to duplicate these methods: it seems better to keep the API clean and leave the client to type a little bit more code, than to implement the same API in two places.
Created attachment 157469 [details] removed API in the *Editing classes shadowing API from the *Constraints classes Thanks for bringing up the topic about the shadow API again, Matthew! I think you're right that using the *Editing classes without the shadowed API is nevertheless concise so I've remved the shadowed API, including the Editing#add(Target|Model|BeforeSetModel)Validator() methods which followed the same pattern. This new patch also includes the message indexes in the JavaDoc for the parameterization of the validation messages.
One point I had (intentionally) ignored until now is the handling of primitive types in the (Integer|Decimal|Boolean)Editing classes. For those classes, I'm setting up validators/converters which assume *non*-primitive types without letting the user configure this behavior through the *Editing class. Actually, I don't like that e.g. a StringToNumberConverter is aware of whether the number it is converting to is of a primitive type or not. Rather, I think the check for primitive types should be done on the model side (after-convert validator) by checking whether the number is null (using a NonNullValidator). However, we might still want to give clients the possibility to explicitly state whether they are editing a primitive type or not? If so, I would suggest to add a "primitive" boolean parameter to all the factory methods of the above *Editing classes in order to support this (analogous to e.g. StringToNumberConverter.forLong(boolean primitive)).
(In reply to comment #56) > One point I had (intentionally) ignored until now is the handling of primitive > types in the (Integer|Decimal|Boolean)Editing classes. For those classes, I'm > setting up validators/converters which assume *non*-primitive types without > letting the user configure this behavior through the *Editing class. This could be a built-in conditional behavior when the Editing class creates the binding. In a nutshell, we could check whether the model observable is a primitive type, and if so add an implicit required() validator.
(In reply to comment #57) > (In reply to comment #56) > > One point I had (intentionally) ignored until now is the handling of primitive > > types in the (Integer|Decimal|Boolean)Editing classes. For those classes, I'm > > setting up validators/converters which assume *non*-primitive types without > > letting the user configure this behavior through the *Editing class. > > This could be a built-in conditional behavior when the Editing class creates > the binding. In a nutshell, we could check whether the model observable is a > primitive type, and if so add an implicit required() validator. I think I would prefer doing so explicitly rather than adding such a validator implicitly since the Editing API in general is thought for explicitly configuring your bindings rather than defaulting anything like is e.g. done by the update strategies. I personally would probably always use a required() constraint rather than using a primitive flag but if we want to support primitive types explicitly I think we should do so now.
Boris, in the last few days Matthew and I have tried to fine tune the patch and I think that we have now reached a point in which it would be good to have another person reviewing the patch so if you think that you can spare some time, that would be great. One point which is still a bit open is the handling of primitive types (see the previous two comments) and the more general point of seeing what benefits it would bring to implement the same functionality using Java5. I think that the comments starting at comment #45 illustrate the relevant discussion points pretty well. Right now, only the *Editing/*Constraints classes are API, all the other stuff is internal so looking at those classes as well as at Snippet035Editing/EditingFactory from the snippets should provide a good picture about the API and its intended use.
How about consolidating the methods for primitives/wrappers into one method: IntegerEditing intWrapperEditing = IntegerEditing.withDefaults(Integer.class); IntegerEditing intPrimitiveEditing = IntegerEditing.withDefaults(int.class); // automatically add a required() validator when numberClass.isPrimitive()
(In reply to comment #60) > How about consolidating the methods for primitives/wrappers into one method: > > IntegerEditing intWrapperEditing = IntegerEditing.withDefaults(Integer.class); > IntegerEditing intPrimitiveEditing = IntegerEditing.withDefaults(int.class); // > automatically add a required() validator when numberClass.isPrimitive() This could be an option which would also reduce the overall number of required factory methods but you would also loose some type safety. However, assuming that primitive types should be handled by adding a required() constraint on the model side rather than letting the target-to-model validator check for primitive types (see AbstractStringToNumberValidator) I'm wondering whether we should provide special support at all for primitive types? After all, adding a required() constraint is something which clients can already do. They could either do so explicitly or if they use something like an EditingFactory they could easily provide any kind of convenience method for setting up an Editing instance for primitive types. Clients could then easily use an explicit class type like above or a boolean parameter or whatever they prefer on the EditingFactory rather than requiring us to directly support this on the *Editing classes.
Sorry guys, only seeing this now.. I am swamped today and tomorrow. Will try to remember this bug on Wednesday :-)
I've finally found some time to give this a try using Java5 and here is my first rough impression. I would say that there are some obvious parts where Java5 would indeed improve the quality of the API, namely: * The extra (target|model|beforeSet)*Constraints() methods on the *Editing classes could be avoided by using covariant return types (e.g. IntegerEditing#modelConstraints() vs. IntegerEditing#modelIntegerConstraints()). * Subclasses of the Constraints class could overwrite the Constraints#addValidator() and Constraints#aggregationPolicy() methods by returning the subclass type in order not to break method chaining. * Classes like Constraints or NumberRangeValidator (internal) could use enums instead of integer constants at different points. However, where I had some more problems was when trying to take advantage of Java5 in order to reduce the duplicated API in the *Constraints classes for the different integer types. E.g. one might be tempted to let the DecimalConstraints class extend the IntegerConstraints class in order to avoid the duplicated API among the two classes. However, in order not to break method chaining when introducing such a type hierarchy, you would have to do one of the following: * Overwrite all the methods coming from IntegerConstraints in the DecimalConstraints subclass in order to return the subtype. * Make the *Constraints classes self-referential generic classes (in the same spirit as the Enum<E extends Enum<E>> class) and let the methods of the superclass return the self-referential type for method chaining. Matthew and I had already discussed those two points some time ago and I personally think that neither of them is really satisfactory. The first solution essentially re-introduces the duplicated API and the second approach somehow results in a rather ugly parameterization of the classes.
(In reply to comment #63) > However, where I had some more problems was when trying to take advantage of > Java5 in order to reduce the duplicated API in the *Constraints classes for the > different integer types. E.g. one might be tempted to let the > DecimalConstraints class extend the IntegerConstraints class in order to avoid > the duplicated API among the two classes. However, in order not to break method > chaining when introducing such a type hierarchy, you would have to do one of > the following: > * Overwrite all the methods coming from IntegerConstraints in the > DecimalConstraints subclass in order to return the subtype. > * Make the *Constraints classes self-referential generic classes (in the same > spirit as the Enum<E extends Enum<E>> class) and let the methods of the > superclass return the self-referential type for method chaining. If I'm not mistaken, this approach allows you to declare methods only once in the superclass: class Constraints<T extends Constraints<T>> { public T required() { add(new RequiredValidator()); return (T) this; } } I'm also thinking that there should just be a single NumberConstraints for all numeric constraints and have that class parameterized with the number type: class NumberConstraints<N extends Number> extends Constraints<T> { public NumberConstraints<N> lessThan(N number) { add(new LessThanValidator(number)); return this; } } The downside is that you end up with some ugly generic type arguments all over the place. Although this is probably a minor thing since presumably most clients will prefer method chaining over storing the constraints object in a local variable. It occurs to me that we could also use generics to parameterize the Editing class e.g. with modelConstraints(). All this mucking around with generics just because Java lacks a "This" return type like in Scala. ;-) > Matthew and I had already discussed those two points some time ago and I > personally think that neither of them is really satisfactory. The first > solution essentially re-introduces the duplicated API and the second approach > somehow results in a rather ugly parameterization of the classes. If it's ugly on the client side then I'm totally with you. But if it's only ugly on the implementation side and beautiful on the client side, I can live with that.
(In reply to comment #64) > If I'm not mistaken, this approach allows you to declare methods only once in > the superclass: > > class Constraints<T extends Constraints<T>> { > public T required() { > add(new RequiredValidator()); > return (T) this; > } > } Yes, using a cast in the methods of the superclass would already be enough. This will of course lead to an unchecked exception but that's no particular problem (the only workaround for the unchecked exception would be the getThis() trick we had discussed some time ago but that was also not particularly nice). > I'm also thinking that there should just be a single NumberConstraints for all > numeric constraints and have that class parameterized with the number type: > > class NumberConstraints<N extends Number> extends Constraints<T> { > public NumberConstraints<N> lessThan(N number) { > add(new LessThanValidator(number)); > return this; > } > } Yes, I would say that, ideally, we should parameterize the Constraints class (and subclasses) with the model type and probably also the Editing class with the target and model types. > It occurs to me that we could also use generics to parameterize the Editing > class e.g. with modelConstraints(). What we might want to keep in mind here is whether we might have additional constraints in future in case the update pipeline is extended in future (maybe not very probable?). Actually, in an early version of the patch, I was passing the different constraints objects directly in the constructor of the *Editing classes when we decided to rather define create*Constraints() methods for them in order to be more flexible when something changes in the validation pipeline. This might also be relevant here. > All this mucking around with generics just because Java lacks a "This" return > type like in Scala. ;-) That might have helped a tiny little bit... :) > > Matthew and I had already discussed those two points some time ago and I > > personally think that neither of them is really satisfactory. The first > > solution essentially re-introduces the duplicated API and the second approach > > somehow results in a rather ugly parameterization of the classes. > > If it's ugly on the client side then I'm totally with you. But if it's only > ugly on the implementation side and beautiful on the client side, I can live > with that. In order to keep the client's code clean, I think we must make sure that all the self-referential generic types need not be directly instantiated by clients but that they are only used inside the framework or used as base classes (the latter eventually also by clients). After all, the self-referential types are merely used in order to get around some of the deficiencies of Java generics. In any case, I think I will try to rewrite the patch using Java5. It's a pitty that I have been a bit busy in the last two weeks and that I couldn't make any progress on this patch during that time...
BTW, Boris, did you have any time to have a look at the patch?
Created attachment 160440 [details] first draft of the patch using Java5 Here's a very first draft of the patch using Java5. Here are the main points I've done so far: * Make the NumberRangeValidator generic and get rid of the subclasses. * Merge the individual *Constraints classes for numbers into a single, generic NumberConstraints class. * Merge the individual *Editing classes for numbers into a single, generic NumberEditing class. * Parameterize the base Constraints class with the type it operates on. * Parameterize the base Editing class with the target and model types it operates on. * Use covariant return types in order to avoid the additional (target|model|beforeSetModel)Constraints() methods on the *Editing classes. * Use an enum for the aggregation policies on the Constraints class. * Introduce a BaseObjectConstraints class which extends Constraints and which serves as the (artificial) base class which defines a self-referential type parameter for not breaking method chaining. BaseObjectConstraints also includes the required() constraint and the other *Constraints classes inherit from this one. What's not working for now are the positive() and nonNegative() constraints on NumberConstraints (see the FIXMEs) since the way it was implemented, a representation for the numbers 0 and 1 was required which must have the correct number type.
Created attachment 160542 [details] ongoing Java5 version of the patch Updated patch. For the problem of the positive() and nonNegative() number constraints described in my previous comment, I've added explicit NumberConstraints#forXXX() factory methods for creating a NumberConstraints object for a specific number type. That way, I can instantiate the constants 0 and 1 having the correct number type. That's actually already a bit ugly. What makes it even a bit worse is that now, in the NumberEditing#createModelConstraints() method, I need to explicitly dispatch on the number type for creating a NumberConstraints object of the correct type. Instead of taking this approach, it would also be possible to do a series of instanceof checks whenever the positive()/nonNegative() constraints are checked but that's also not much nicer I guess.
Created attachment 179211 [details] Java 1.4 version of the patch for the new release cycle This is an updated version of the Java 1.4 version of the patch for the new release cycle.
Created attachment 179212 [details] Java5 version of the patch for the new release cycle This is an updated version of the Java 5 version of the patch for the new release cycle.
Boris, I wanted to ask whether you might have some time to look at the patch? I think it's now in a state in which it would be good to have a third person looking at it to see whether it makes sense at all what Matthew and I have been doing here :). If you find some time, I think that it would be enough to look at the API classes, i.e. the Editing and Constraints classes along with their subclasses. I would be very interested in hearing what you think about the approach taken so far. I think that the two provided snippets (mainly Snippet035Editing along with the sample EditingFactory class) illustrate pretty well the intended use of the API.
Can we change the subject line to something like "New Editing and Constraints API?" I can never find this bug by the current subject line.
(In reply to comment #72) > Can we change the subject line to something like "New Editing and Constraints > API?" I can never find this bug by the current subject line. Done, the subject definitely didn't fit anymore :).
Boris, do you think you could take a moment to look over this patch? This is a substantial new API and we're interested in your feedback.
Ovidio, making you QA contact since you're already the assignee for this bug
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant.
