Community
Participate
Working Groups
It would be nice if the MultiValidator class would support asynchronous validations. To that end, an adequate API should be added to allow for subclasses to notify the MultiValidator about a validation being executed asynchronously. Ideally, whenever an asynchronous validation is pending, the MultiValidator should set the validation status observable to be stale.
Created attachment 105515 [details] proposed patch along with some unit tests and a snippet The proposed patch introduces the pair of methods enterStale()/exitStale(IStatus) on the MultiValidator class. enterStale() should be called by subclasses whenever an asynchronous validation is initiated and leads to the validation status observable of the MultiValidator to become stale. As soon as the asynchronous validation terminates, subclasses should call exitStale(IStatus) and pass the result of the validation to the MultiValidator which in turn sets the validation status observable to be not stale anymore. For synchronous validations, there is no need to use the above methods. When starting an asynchronous validation, the result of the already existing validate method can be used as a temporary validation result which is used while performing the actual validation. Note: The patch also includes the trivial fix for bug #237163 which is required to get the staleness state right. If wanting to move this fix out of the patch, please just remove the single tiny change in the UnmodifiableObservableValue class.
By the way, the proposed patch only contains adaptations to the MultiValidator class. Of course, the ValidatedObservable(Value|Set|List|Map)s should also be adapted to track the staleness of the validation status observable. Matthew Hall has already done so in a patch contributed to the predecessor of this bug, bug #233191, where he is providing an alternative patch to the one posted here.
Created attachment 106506 [details] updated patch with tests and a snippet The original patch did not apply anymore due to some recent commits so I have merged the code and I'm providing an updated version of the patch. I've also dropped the fix for bug #237163 which was contained in the previous patch since that fix has already been released to HEAD in the meanwhile.
Created attachment 106541 [details] Latest patch using status callback interface I am still of the mind that a callback is preferable to an enterStale() / exitStale() structure, for the following reasons: * The validate() method as it exists currently is expected to return the current validation status. In the enterStale() / exitStale() paradigm the returned result may or may not be relevant depending on whether the status is calculated asynchronously. In the callback paradigm, the user is expected to either override the validate() method for synchronous validation or validate(Callback) for asynchronous validation. I am very uncomfortable with diluting the contract of existing API, and the change I have advocated avoids this. * The revalidate() routine, as of this patch, makes the validation status observable stale only if the callback hasn't been called by the time the validate method returns. So the client doesn't have to worry about staleness. * The callback implementation executes the status update on the correct realm, so clients don't have to worry about realms either. * Using a separate callback object allows us to introduce a cancel option on the callback, in the case where a dependency observable changes while validation is still pending. The enterStale() / exitStale() procedure has no context attached from validation to validation--it is even conceivable, without a cancel option, that the current validation process could complete before an obsolete validation process, and that the obsolete process wipes out the correct validation status.
Created attachment 106542 [details] Latest patch using status callback interface I am still of the mind that a callback is preferable to an enterStale() / exitStale() structure, for the following reasons: * The validate() method as it exists currently is expected to return the current validation status. In the enterStale() / exitStale() paradigm the returned result may or may not be relevant depending on whether the status is calculated asynchronously. In the callback paradigm, the user is expected to either override the validate() method for synchronous validation or validate(Callback) for asynchronous validation. I am very uncomfortable with diluting the contract of existing API, and the change I have advocated avoids this. * The revalidate() routine, as of this patch, makes the validation status observable stale only if the callback hasn't been called by the time the validate method returns. So the client doesn't have to worry about staleness. * The callback implementation executes the status update on the correct realm, so clients don't have to worry about realms either. * Using a separate callback object allows us to introduce a cancel option on the callback, in the case where a dependency observable changes while validation is still pending. The enterStale() / exitStale() procedure has no context attached from validation to validation--it is even conceivable, without a cancel option, that the current validation process could complete before an obsolete validation process, and that the obsolete process wipes out the correct validation status.
(In reply to comment #5) > Created an attachment (id=106542) [details] > Latest patch using status callback interface > > I am still of the mind that a callback is preferable to an enterStale() / > exitStale() structure, for the following reasons: > * The validate() method as it exists currently is expected to return the > current validation status. In the enterStale() / exitStale() paradigm the > returned result may or may not be relevant depending on whether the status is > calculated asynchronously. In the callback paradigm, the user is expected to > either override the validate() method for synchronous validation or > validate(Callback) for asynchronous validation. I am very uncomfortable with > diluting the contract of existing API, and the change I have advocated avoids > this. You're right here regarding the validation status returned by validate() while a validation is pending since in practice you will probably to keep the last validation status while performing a validation asynchronously. This could also be done with my implementation by checking the stale flag and not setting the validation if the flag is true but this is obviously not so nice. > * The revalidate() routine, as of this patch, makes the validation status > observable stale only if the callback hasn't been called by the time the > validate method returns. So the client doesn't have to worry about staleness. That's great as I that was my main concern about the callback implementation! > * Using a separate callback object allows us to introduce a cancel option on > the callback, in the case where a dependency observable changes while > validation is still pending. The enterStale() / exitStale() procedure has no > context attached from validation to validation--it is even conceivable, without > a cancel option, that the current validation process could complete before an > obsolete validation process, and that the obsolete process wipes out the > correct validation status. The canceling in my implementation was implicit in the sense that whenever you trigger the validate() method any potentially pending validation was canceled which is illustrated in the snippet but with the callback approach you may be able to do this in the MultiValidator which is good. So, all in all, you seem to have convinced me of the callback approach :-).
Matthew, I would suggest to move the part of your patch related to the staleness of the ValidatedObservable(Value|Set|List|Map) to a separate bug as this is a different issue. What do you think? BTW, if you want me to contribute something to your patch like e.g. some unit tests for the staleness of the validated observables or the like, just let me know.
(In reply to comment #7) > BTW, if you want me to contribute something to your patch like e.g. some unit > tests for the staleness of the validated observables or the like, just let me > know. Consider this a standing invitation to submit any patches for any bug, which you think may be useful. Unit tests are most definitely welcome.
Planned for 3.5M2
Matthew, sorry for my rather long absence but I have been a bit busy in the last few weeks. In any case, I have written some unit tests for the validated observables to track the staleness of the validation status observable. I've found some minor issues which prevented the validated observables from becoming non-stale. This should be fixed in the new patch and captured by the unit tests. I've attached the patch to a new bug: bug #248868.
Created attachment 114899 [details] extension of Matthew's patch to support cancellation along with unit tests Matthew, I have extended your patch for the asynchronous validation in order to make it support the cancellation of obsolete validations. I had also to adapt a few things due to bug #238909 which requires that the dependencies are up-to-date before setting a new validation status. Please see the code documentation for more details. The patch also includes some unit tests.
Created attachment 114900 [details] extension of Matthew's patch to support cancellation along with unit tests Matthew, I have extended your patch for the asynchronous validation in order to make it support the cancellation of obsolete validations. I had also to adapt a few things due to bug #238909 which requires that the dependencies are up-to-date before setting a new validation status. Please see the code documentation for more details. The patch also includes some unit tests.
Taking ownership, retargeting for 3.5M3
Ovidio: All your improvements are great and I would commit them as is, however the patch is now over the LOC threshold so unless we can determine how many lines of code are yours and how many are mine, we will have to submit this to IPZilla for review first. I took a stab at this but got lost quickly :-/
Matthew, if I simply remove the IStatusCallback interface and your new validate(IStatusCallback) method (along with its extensive JavaDoc) from the patch, Eclipse already reports 253 added and 21 removed lines, including all the unit tests (these alone make up 158 lines). So here I'd say that the patch as a whole (including tests) would definitely fall below the 250 LOC threshold if also removing other code I've taken over from your patch.
(In reply to comment #15) > Matthew, if I simply remove the IStatusCallback interface and your new > validate(IStatusCallback) method (along with its extensive JavaDoc) from the > patch, Eclipse already reports 253 added and 21 removed lines, including all > the unit tests (these alone make up 158 lines). So here I'd say that the patch > as a whole (including tests) would definitely fall below the 250 LOC threshold > if also removing other code I've taken over from your patch. Okay, I see that you renamed StatusCallback in my patch to IStatusCallback (a good change, but it still accounts for at least a few LOC). To be safe I'm going to have to do my diligence on this and scrutinize each line to determine who owns what. If we come in below the threshold then I'll commit the patch as is (but will add attribution comments to the patch). If we're above the threshold then I'll submit a contribution review to IPZilla and we'll go from there. Matthew
I've been thinking that IStatusCallback should have an isCanceled() method so that long-running validators can abort on their own, instead of just having MultiValidator ignore the status when they complete.
I had to merge my patch with recent changes to HEAD before I could run a comparison. Here is my tally: Lines of code tally: * MultiValidator - 85 lines different from my merged version (determined this renaming my version to MultiValidator2 and side by side with MultiValidator from Ovidio's patch). * MultiValidatorTest - 158 new lines * IStatusCallback - 3 lines For a total of 246 lines--just under the wire. :) I will submit my post-merge patch just for good measure. I've also been thinking that we might want to allow a validator to call handleStatus multiple times. This would allow an asynchronous validator to inform the user show the current status of the validator as it is running: public void validate(final IStatusCallback callback) { new Thread(new Runnable() { public void run() { callback.handleStatus(ValidationStatus.cancel("Reticulating splines")); Spline[] reticulated = reticulateSplines(); IStatus status = validateSplines(reticulated); callback.handleStatus(status); } }).start(); } Perhaps there should be a boolean parameter indicating whether validation is complete, or a separate method for intermediate "progress" statuses. Thoughts?
Created attachment 115064 [details] Patch merging my last patch with HEAD
Created attachment 115065 [details] mylyn/context/zip
There's a problem with the validated observables when you use asynchronous validators. Run Snippet021MultiFieldValidation and do the following: 1. Edit the fields on the left so they are both even 2. Add some addends on the right so they equal 5 (the default sum) 3. Note that the status is ok. Note also that all model fields are now equal to their respective target fields 4. Type a large number into the sum field. 5. After the 1-second delay the status line changes to an error message, as expected. 6. Note that the bad data from the sum field has been copied to model. This is because the validation status is still OK by the time ValidatedObservableValue receives the change event. The problem is that the ValidatedObservableValue listeners are getting notified before MultiValidator about the change. So we do not have a chance to stale the validation status before the validated observables update themselves. The fix I'm attempting is to trigger a revalidation any time the observeValidated* observable observable a change in its target, and to use asyncExec() to avoid redundant revalidations.
Created attachment 115099 [details] Fixed validated observable taking invalid values This patch does not yet incorporate Ovidio's changes
Created attachment 115100 [details] mylyn/context/zip
(In reply to comment #17) > I've been thinking that IStatusCallback should have an isCanceled() method so > that long-running validators can abort on their own, instead of just having > MultiValidator ignore the status when they complete. That's definitely a good idea. (In reply to comment #18) > I had to merge my patch with recent changes to HEAD before I could run a > comparison. Here is my tally: > [...] > For a total of 246 lines--just under the wire. :) :-) > I've also been thinking that we might want to allow a validator to call > handleStatus multiple times. This would allow an asynchronous validator to > inform the user show the current status of the validator as it is running: > [...] > Perhaps there should be a boolean parameter indicating whether validation is > complete, or a separate method for intermediate "progress" statuses. I think that it would be good to be able to at least (optionally) provide one temporary validation status which is used while the validation is being performed asynchronously. This simpler use case could e.g. be supported by letting the new MultiValidator#validate(IStatusCallback) method return an IStatus which, if non-null, could be used as a temporary validation status for asynchronous validations: public abstract class MultiValidator { protected IStatus validate(IStatusCallback); } What you're proposing is even more flexible since then you would even be able to pass back multiple temporary validation statuses which might also be useful in practice. In that case, I think I would introduce a subinterface of IStatusCallback declaring an additional method for that purpose since the IStatusCallback interface might be usable for other things in future in which having intermediate statuses does not make sense. So, I would do something like: public interface IProgressStatusCallback extends IStatusCallback { public void handleProgressStatus(IStatus); } public abstract class MultiValidator { public void validate(IProgressStatusCallback); } However, I think that both APIs above have the weakness that clients may mis-use them by e.g. returning a temporary validation status even though the validation is not asynchronous (case 1) or by calling the different callback methods in the wrong order (case 2).
Created attachment 115289 [details] Update In the previous patch, MultiValidator was doing revalidation in a Realm.asyncExec(). This had the side effect that you had to pump the event loop to get the validator to run. This patch fixes the problem mentioned in comment #21 without doing an asyncExec().
Created attachment 115290 [details] mylyn/context/zip
(In reply to comment #21) > [...] > The problem is that the ValidatedObservableValue listeners are getting notified > before MultiValidator about the change. So we do not have a chance to stale > the validation status before the validated observables update themselves. It seems to me as if this problem is not limited to the new staleness feature but it could also happen with the normal change events. The reason why the problem didn't arise so far probably is that the MultiValidator registers itself as a plain change listener to its dependencies. Since the validated observables attach themselves as more specific value/list/set/map change listeners and those listeners are always notified about changes after the normal change listeners, nothing bad ever happened. Actually, it was interesting to debug this through. In any case, it's good that you have noticed this, Matthew.
Created attachment 115313 [details] Update Added IStatusCallback.isCanceled(). The snippet has been overhauled and split into two pages: the first for synchronous validation, and the second for asynchronous validation
Created attachment 115314 [details] mylyn/context/zip
Created attachment 115315 [details] Merged with Ovidio's patch Merged included 30-40 lines of Ovidio's code in MultiValidator (mostly comments), and 158 lines of code from MultiValidatorTest. Boris, +1?
Created attachment 115316 [details] mylyn/context/zip
I have a p(In reply to comment #27) > (In reply to comment #21) > > [...] > > The problem is that the ValidatedObservableValue listeners are getting notified > > before MultiValidator about the change. So we do not have a chance to stale > > the validation status before the validated observables update themselves. > It seems to me as if this problem is not limited to the new staleness feature > but it could also happen with the normal change events. The way I got around this was to trigger a revalidation on the multivalidator when the validated observable detects a change to its target observable. If the validation status is stale by the time revalidation returns then an asynchronous validation is taking place and the validated observable is made dirty.
Boris, +1 for latest patch?
Matthew, I had a brief look at your patch an here are a few comments: 1. Just a detail: In the method ValidationCallback#handleStatus() (line 280) you should probably be using the "realmSnapshot" variable instead of "realm" in the expression "realm.isCurrent()". 2. Regarding the intermediate status' in your snippet, it seems as if after the first intermediate status returned to the callback, the MultiValidator's validation already becomes non-stale which should not happen. I guess there's really need for a second method or a flag to differentiate between intermediary and the final status. 3. Why do you -always- make the MultiValidator's validation stale inside the MultiValidator#revalidate() method?
(In reply to comment #34) > 1. Just a detail: In the method ValidationCallback#handleStatus() (line 280) > you should probably be using the "realmSnapshot" variable instead of "realm" in > the expression "realm.isCurrent()". I'll fix that, thanks for catching that. > 2. Regarding the intermediate status' in your snippet, it seems as if after the > first intermediate status returned to the callback, the MultiValidator's > validation already becomes non-stale which should not happen. I guess there's > really need for a second method or a flag to differentiate between intermediary > and the final status. My thought was that an intermediate status should always be of severity IStatus.CANCEL (I should probably document this in validate(IStatusCallback) javadocs). This ensures that the validation status is not considered valid by the validated observables, and thus they will not update from their targets yet. If you think this is not explicit enough then let's discuss what the API would look like. > 3. Why do you -always- make the MultiValidator's validation stale inside the > MultiValidator#revalidate() method? Good point. We could change it to be stale only if a status has not been received before returning.
Created attachment 115418 [details] Update Fixed problems mentioned in points 1 and 3 of comment #34. Also added fix to avoid duplicate revalidations when a dependency is also the target of a validated observable
Created attachment 115419 [details] mylyn/context/zip
Running the snippet causes a NPE on the first page: Exception in thread "Thread-0" java.lang.NullPointerException at org.eclipse.jface.examples.databinding.snippets.Snippet021MultiFieldValidation$6.run(Snippet021MultiFieldValidation.java:320) at java.lang.Thread.run(Thread.java:619) Also, I am not happy with isCanceled() on IStatusCallback, and the fact that you use callback.handleStatus(ValidationStatus.cancel("Revalidating in " + count)) to report progress. It looks like validate() should take an IProgressMonitor and an IStatusCallback (without the isCanceled), or a subclass of IProgressMonitor that adds the handleStatus() method.
Created attachment 115438 [details] Update Fixed NPE in snippet--it was actually being thrown from the asynchronous validator's thread.
Created attachment 115439 [details] mylyn/context/zip
Created attachment 115630 [details] Update Modified snippet to use wizardPage.getContainer().exec(true, false, runnable) to spawn async validation instead of new Thread(runnable).start(). I've also changed the timings to make the progress bar more fluid. One major drawback is that WizardDialog automatically disables all controls on the page while the forked task is running. The behavior we'd like to see is for the next and finish buttons to disable and to show the progress bar, but otherwise leave the UI intact so the user can continue to interact with the form. Any changes to the UI while asynchronous validation is running should cancel the current validation request and spawn a new one. The goal with all this is to let *PageSupport users to get free progress monitor updates when the using asynchronous validation in MultiValidator.
Created attachment 115631 [details] mylyn/context/zip
Created attachment 115720 [details] Experiment Reverted to using WizardDialog.run(). Adding IStatusCallback.handleMessage(String)
Created attachment 115721 [details] mylyn/context/zip
(In reply to comment #38) > [...] > Also, I am not happy with isCanceled() on IStatusCallback, and the fact that > you use callback.handleStatus(ValidationStatus.cancel("Revalidating in " + > count)) to report progress. It looks like validate() should take an > IProgressMonitor and an IStatusCallback (without the isCanceled), or a subclass > of IProgressMonitor that adds the handleStatus() method. Boris, how did you intend letting clients report progress during validation? Would you use the task methods on the IProgressMonitor or simply allow clients to call IStatusCallback#handleStatus(IStatus) multiple times until IProgressMonitor#done() is finally called to terminate the validation? I personally think that the IProgressMonitor interface has a few methods which do not naturally apply to the MultiValidator context which might be a bit confusing so I think I would rather try to define some subinterface of IStatusCallback which introduces an isCanceled() method as well as a second method for explicitly reporting intermediary validation statuses. What seems important to me is: * If introducing an IStatusCallback interface, I would try to only give it a handleStatus(IStatus) method. Other things like progress statuses should be placed in a subinterface since a plain IStatusCallback interface may be used at other points in future where such "special features" do not make sense. * If supporting intermediary statuses in the MultiValidator during an asynchronous validation, the MultiValidator should remain stale as long as the final status is not received, even if the intermediary status is of type CANCEL/ERROR since I think that the distinction between "invalid" and "stale" is important. This implies that we need an explicit distinction between some intermediary status and the final one.
(In reply to comment #45) > Boris, how did you intend letting clients report progress during validation? > Would you use the task methods on the IProgressMonitor or simply allow clients > to call IStatusCallback#handleStatus(IStatus) multiple times until > IProgressMonitor#done() is finally called to terminate the validation? The client would be expected to pass progress messages to the IProgressMonitor, and pass the final result to the IStatusCallback. The idea is to translate status messages in the IProgressMonitor into stale events (with the progress information embedded) on the validation status observable. With progress information integrated into StaleEvents, we could then change WizardPageSupport so that stale events on the validation status observables translate into automatic management of the wizard's progress bar. > I personally think that the IProgressMonitor interface has a few methods which > do not naturally apply to the MultiValidator context which might be a bit > confusing so I think I would rather try to define some subinterface of > IStatusCallback which introduces an isCanceled() method as well as a second > method for explicitly reporting intermediary validation statuses. I can see it both ways. However the possibility to get progress bars for free in the wizard dialog is very encouraging, and one that would benefit many Eclipse projects. So we're investigating to see how well it would work. > What seems important to me is: > * If introducing an IStatusCallback interface, I would try to only give it a > handleStatus(IStatus) method. Other things like progress statuses should be > placed in a subinterface since a plain IStatusCallback interface may be used at > other points in future where such "special features" do not make sense. I'm actually looking at two possibilities: * Pass both an IStatusCallback (with a single handleStatus method) and an IProgressMonitor to the validate() method * Make IStatusCallback a subinterface of IProgressMonitor (IStatusMonitor?) and just pass that one object in. > * If supporting intermediary statuses in the MultiValidator during an > asynchronous validation, the MultiValidator should remain stale as long as the > final status is not received, even if the intermediary status is of type > CANCEL/ERROR since I think that the distinction between "invalid" and "stale" is > important. This implies that we need an explicit distinction between some > intermediary status and the final one. We are planning to put intermediate status in the StaleEvent, so intermediate status messages would implicitly stale the validation status observable.
Created attachment 115840 [details] Experimental: automatic progress bars The recent changes in this page are somewhat sloppy, but they work. Boris, I need a critical eye on everything in this patch related to IProgressMonitor, particularly in WizardPageSupport. I had to do a lot of weird stuff to compensate for the fact that WizardDialog.run() doesn't give you an IProgressMonitor to send updates to until the forked thread calls my run() method. A few hiccups with the current state of this patch: * The first time asynchronous validation runs, only the subtask label of the progress monitor part is visible. On all subsequent validations the part is fully visible as expected. * Still no way to keep the UI enabled while asynchronous validation is running.
Created attachment 115841 [details] mylyn/context/zip
Created attachment 115861 [details] Experiment Removed observable-to-progress monitor mapping. This is simpler but has its own set of problems (i.e. what if there are two multivalidators running concurrently)
Created attachment 115862 [details] mylyn/context/zip
Moving to M5
Retargeting to 3.5M6
We are out of time for 3.5. Sorry.
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. If the bug is still relevant, please remove the stalebug whiteboard tag.
