Community
Participate
Working Groups
Hello, Following the discussion that started on that thread <fosgu3$qgc$1@build.eclipse.org>, I'd like to propose an improvement of the MarkerUtil class. For now, this class is only used to create markers and the user must take care of deleting them. This kinda shocked me that the object responsible for creating something is not the one that actually deletes it. I think it would be better if this class had a more general role. I mean, it could simply handle adding/deleting/updating markers on the set of selected resources. It just needs the validation status to do so. If you give me the go, I can try and attach some code by the end of the day. Best regards,
Thanks for volunteering, Damien! I look forward to your contribution.
Current EMF Validation component version is 1.2.
(In reply to comment #0) > Hello, > > Following the discussion that started on that thread > <fosgu3$qgc$1@build.eclipse.org>, I'd like to propose an improvement of the > MarkerUtil class. For now, this class is only used to create markers and the > user must take care of deleting them. This kinda shocked me that the object > responsible for creating something is not the one that actually deletes it. > > I think it would be better if this class had a more general role. I mean, it > could simply handle adding/deleting/updating markers on the set of selected > resources. It just needs the validation status to do so. > > If you give me the go, I can try and attach some code by the end of the day. > > Best regards, > (In reply to comment #2) > Current EMF Validation component version is 1.2. > Sorry for the mistake about the component version. Also I've never used bugzilla before, so I'm not really sure if I should leave "leave as new"' checked or accept the bug. Anyway, I am going on a 1 week trip for work, so I will post a patch during this week-end but I can't promise anything..
No worries, and there's no rush. Regarding the bugzilla work-flow, it's usual to change status of a largish bug or enhancement to ASSIGNED when you're actually working on it. Otherwise, the fact that it isn't still assigned to the inbox means that it's been assigned to a real person to address.
Created attachment 92583 [details] Improved MarkerUtil Class I completely changed the API. I mean I removed every createMarkers function and replaced them by updateMarkers functions. I did that in order to keep the patch as minimal as possible, I think it might be better to have both APIs (updateMarkers + createMarkers).
Created attachment 92584 [details] Helper class for MarkerUtil
Hi, Damien, Sorry that it has taken so long to review your contribution. It looks good, but we will need two changes: - can you provide a patch in the usual CVS format? Using the "Team -> Create Patch..." menu action. It's difficult to ensure consistency with replacement files - we will need JUnit tests covering the new marker-update capability, in addition to ensuring that existing tests of the marker-add (there are some) still pass Thanks!
(In reply to comment #7) > Hi, Damien, > Hello :) > Sorry that it has taken so long to review your contribution. It looks good, > but we will need two changes: > > - can you provide a patch in the usual CVS format? > Using the "Team -> Create Patch..." menu action. It's > difficult to ensure consistency with replacement files I will provide you with that. I didn't send a patch before because I thought it was easier to review. > - we will need JUnit tests covering the new marker-update > capability, in addition to ensuring that existing tests of > the marker-add (there are some) still pass Alright, I'll see what needs to be added/updated. Best regards,
Super. I don't want to rush you, but the M6 API Freeze milestone is coming up on 31 March, so there not much time remaining for API changes such as this.
(In reply to comment #9) > Super. I don't want to rush you, but the M6 API Freeze milestone is coming up > on 31 March, so there not much time remaining for API changes such as this. > I'll do that tonight or tomorrow. I just have one last question. In the end, do we have both createMarkers and updateMarkers API or just updateMarkers?
(In reply to comment #10) > (In reply to comment #9) > > Super. I don't want to rush you, but the M6 API Freeze milestone is coming up > > on 31 March, so there not much time remaining for API changes such as this. > > > I'll do that tonight or tomorrow. I just have one last question. In the end, do > we have both createMarkers and updateMarkers API or just updateMarkers? hello Damien, I tried to use your updated files (BatchValidationDelegate, MarkerUtil and FileStatusListPair) to avoid persistence of markers after a successful validation. But the problem persists. I think that it comes from the fact that method updateFileMarkers which calls file.deleteMarkers is not called when the status is OK. so, when I validate a first time with errors, errors are shown in the problems view, and when I respect constraints and validate a second time, errors persist cause status is OK (updateFileMakers is not called). To avoid this, I call this method even if status is ok. I wait for your comment or changes. Asma
(In reply to comment #11) > (In reply to comment #10) > > (In reply to comment #9) > > > Super. I don't want to rush you, but the M6 API Freeze milestone is coming up > > > on 31 March, so there not much time remaining for API changes such as this. > > > > > I'll do that tonight or tomorrow. I just have one last question. In the end, do > > we have both createMarkers and updateMarkers API or just updateMarkers? > > hello Damien, > > I tried to use your updated files (BatchValidationDelegate, MarkerUtil and > FileStatusListPair) to avoid persistence of markers after a successful > validation. > But the problem persists. > I think that it comes from the fact that method updateFileMarkers which calls > file.deleteMarkers is not called when the status is OK. > so, when I validate a first time with errors, errors are shown in the problems > view, and when I respect constraints and validate a second time, errors persist > cause status is OK (updateFileMakers is not called). > > To avoid this, I call this method even if status is ok. It's exactly what the patch does. Before, the call to createMarkers was inside the else clause of if (status.isOK()). Now it is after the closing brace of the else clause so it should be called everytime a validation finishes, regardless of the status. I checked the file I uploaded and it's the same as the one I have on my computer and that corrects the problem. Are you using the BatchDelegationDelegate file I uploaded here?
(In reply to comment #12) > (In reply to comment #11) > > (In reply to comment #10) > > > (In reply to comment #9) > > > > Super. I don't want to rush you, but the M6 API Freeze milestone is coming up > > > > on 31 March, so there not much time remaining for API changes such as this. > > > > > > > I'll do that tonight or tomorrow. I just have one last question. In the end, do > > > we have both createMarkers and updateMarkers API or just updateMarkers? > > > > hello Damien, > > > > I tried to use your updated files (BatchValidationDelegate, MarkerUtil and > > FileStatusListPair) to avoid persistence of markers after a successful > > validation. > > But the problem persists. > > I think that it comes from the fact that method updateFileMarkers which calls > > file.deleteMarkers is not called when the status is OK. > > so, when I validate a first time with errors, errors are shown in the problems > > view, and when I respect constraints and validate a second time, errors persist > > cause status is OK (updateFileMakers is not called). > > > > To avoid this, I call this method even if status is ok. > It's exactly what the patch does. Before, the call to createMarkers was inside > the else clause of if (status.isOK()). Now it is after the closing brace of the > else clause so it should be called everytime a validation finishes, regardless > of the status. > I checked the file I uploaded and it's the same as the one I have on my > computer and that corrects the problem. Are you using the > BatchDelegationDelegate file I uploaded here? it is right that MarkerUtil.updateMarkers(status) is always called in the BatchValidationDelegate but I am talking about the updateFileMarkers in the MarkerUtil class. it is called only if it.hasnext() is true. while (it.hasNext()){... updateFileMarkers(fileStatusList.getFile(), fileStatusList.getStatusList(), markerType, configurator);...} (when the status is Ok it.hasNext()==false, so updateFileMarkers is not called that's mean that deleteMarkers is not called too, and that's why markers persists even validation is passed.)
(In reply to comment #10) > > I'll do that tonight or tomorrow. I just have one last question. In the end, do > we have both createMarkers and updateMarkers API or just updateMarkers? Great! Yes, we must retain all of the existing createMarkers() API for compatibility with previous releases. However, it can certainly be deprecated if the new API obsoletes it.
(In reply to comment #13) -----8<----- > (when the status is Ok it.hasNext()==false, so updateFileMarkers is not called > that's mean that deleteMarkers is not called too, and that's why markers > persists even validation is passed.) This sounds like a good unit test. It is important that a validation that produces no statuses (all constraints pass) actually remove all existing markers from the resources that were validated.
(In reply to comment #9) > Super. I don't want to rush you, but the M6 API Freeze milestone is coming up > on 31 March, so there not much time remaining for API changes such as this. > Désolé... I had a crazy week-end and yesterday I spend the day in the plane and the bus... Anyway, I missed the deadline :(
(In reply to comment #16) > Désolé... > > I had a crazy week-end and yesterday I spend the day in the plane and the > bus... Anyway, I missed the deadline :( Not necessarily. I, for one, wouldn't want to waste the effort that you have put into this enhancement so far. A freeze is what we make of it, and Canadians in particular are accustomed to working in cold temperatures :-) This is the kind of API addition that is easily demonstrated to have no impact on existing clients. I see no reason why we can't commit this into M7.
(In reply to comment #13) > (In reply to comment #12) > > (In reply to comment #11) > > > (In reply to comment #10) > > > > (In reply to comment #9) > > > > > Super. I don't want to rush you, but the M6 API Freeze milestone is coming up > > > > > on 31 March, so there not much time remaining for API changes such as this. > > > > > > > > > I'll do that tonight or tomorrow. I just have one last question. In the end, do > > > > we have both createMarkers and updateMarkers API or just updateMarkers? > > > > > > hello Damien, > > > > > > I tried to use your updated files (BatchValidationDelegate, MarkerUtil and > > > FileStatusListPair) to avoid persistence of markers after a successful > > > validation. > > > But the problem persists. > > > I think that it comes from the fact that method updateFileMarkers which calls > > > file.deleteMarkers is not called when the status is OK. > > > so, when I validate a first time with errors, errors are shown in the problems > > > view, and when I respect constraints and validate a second time, errors persist > > > cause status is OK (updateFileMakers is not called). > > > > > > To avoid this, I call this method even if status is ok. > > > It's exactly what the patch does. Before, the call to createMarkers was inside > > the else clause of if (status.isOK()). Now it is after the closing brace of the > > else clause so it should be called everytime a validation finishes, regardless > > of the status. > > I checked the file I uploaded and it's the same as the one I have on my > > computer and that corrects the problem. Are you using the > > BatchDelegationDelegate file I uploaded here? > > it is right that MarkerUtil.updateMarkers(status) is always called in the > BatchValidationDelegate but I am talking about the updateFileMarkers in the > MarkerUtil class. it is called only if it.hasnext() is true. > > while (it.hasNext()){... > updateFileMarkers(fileStatusList.getFile(), fileStatusList.getStatusList(), > markerType, configurator);...} > > > (when the status is Ok it.hasNext()==false, so updateFileMarkers is not called > that's mean that deleteMarkers is not called too, and that's why markers > persists even validation is passed.) > When I run it, even if the status is Ok, there is still at least one item in the map (the file on which the validation succeeded associated with the ok status). That's really strange... Note I am using eclipse 3.3 with EMF Validation 1.2, what about you?
(In reply to comment #7) > Hi, Damien, > > Sorry that it has taken so long to review your contribution. It looks good, > but we will need two changes: > > - can you provide a patch in the usual CVS format? > Using the "Team -> Create Patch..." menu action. It's > difficult to ensure consistency with replacement files > - we will need JUnit tests covering the new marker-update > capability, in addition to ensuring that existing tests of > the marker-add (there are some) still pass > > Thanks! > I tried to run the existing test for MarkerUtil. I checked out all the org.eclipse.emf.validation plugins in my workspace and configured the MarkerUtil test so that it runs with the emf.validation plugins from my workspace rather than the ones installed in my plugins directory. In the end the test fails on the first assert line 69 where it checks the status of the validation (which is expected to have a severity greater than 'warning'). Did I do something wrong?
(In reply to comment #18) > When I run it, even if the status is Ok, there is still at least one item in > the map (the file on which the validation succeeded associated with the ok > status). That's really strange... Yes, I think that is a placeholder that informs clients that this resource was validated but no problems were found. Otherwise, without any entries in this map, there would be no indication of which resources were validated. > Note I am using eclipse 3.3 with EMF Validation 1.2, what about you? The component build will be running the unit tests with Eclipse 3.4 and EMF 2.4 ...
(In reply to comment #19) > I tried to run the existing test for MarkerUtil. I checked out all the > org.eclipse.emf.validation plugins in my workspace and configured the > MarkerUtil test so that it runs with the emf.validation plugins from my > workspace rather than the ones installed in my plugins directory. > > In the end the test fails on the first assert line 69 where it checks the > status of the validation (which is expected to have a severity greater than > 'warning'). > > Did I do something wrong? Did you run the test as a JUnit Plug-in test, or as a simple JUnit Test? The tests need the Eclipse run-time. Also, it may be that the test doesn't work in isolation because it depends on statically configured constraints and other data ... try running the entire org.eclipse.emf.validation.tests.AllTests suite if you didn't already.
(In reply to comment #20) > (In reply to comment #18) > > When I run it, even if the status is Ok, there is still at least one item in > > the map (the file on which the validation succeeded associated with the ok > > status). That's really strange... > > Yes, I think that is a placeholder that informs clients that this resource was > validated but no problems were found. Otherwise, without any entries in this > map, there would be no indication of which resources were validated. > > > > Note I am using eclipse 3.3 with EMF Validation 1.2, what about you? > > The component build will be running the unit tests with Eclipse 3.4 and EMF 2.4 > ... > I'm using Eclipse 3.4 and EMF 2.4 now :) I found where the problem was coming from. I was running the plugin with the adapter example plugin installed. Whenever I would do a validation on an EXTLibrary model, there would be an info validation status generated by the adapter example. Good thing Asma noticed that there's a problem. So I removed the adapter example from the list of enabled plugins and now the same problem appears for me. The problem lies here: } else if ((validationStatus.matches(severityMask) && (validationStatus instanceof IConstraintStatus)) { With the adapter example disabled, this evaluates to false because matches() never matches for an ok status and in the case of a successful validation, the evaluateConstraints() method in AbstractValidator does not add an ok status to the list of validation statuses and as a consequence, the createStatus() method in AbstractValidator receives an empty list of statuses and creates a simple Status that does not contain any information about the ressource on which the validation was made. I could add an else if clause: } else if ((validationStatus.isOK() && (validationStatus instanceof Status)) { and add the status to the "ressourcesToVisit" variable. But now, I have a problem. A simple Status doesn't have any information about the ressource on which the validation occured so I wouldn't be able to delete the validation markers. So.. Why does the AbstractValidator creates a simple Status and not a ConstraintStatus when a validation is succcessful? I mean, if a constraint evaluation gives true, it doesn't mean that no contraint was evaluated and the resource information is very useful for deleting the markers. I know it's easier to just delete the markers in the BatchValidationDelegate class but this would be pretty awkward to delete on the user side something that should be updated by the same classe that created it.
(In reply to comment #21) > (In reply to comment #19) > > I tried to run the existing test for MarkerUtil. I checked out all the > > org.eclipse.emf.validation plugins in my workspace and configured the > > MarkerUtil test so that it runs with the emf.validation plugins from my > > workspace rather than the ones installed in my plugins directory. > > > > In the end the test fails on the first assert line 69 where it checks the > > status of the validation (which is expected to have a severity greater than > > 'warning'). > > > > Did I do something wrong? > > Did you run the test as a JUnit Plug-in test, or as a simple JUnit Test? The > tests need the Eclipse run-time. Also, it may be that the test doesn't work in > isolation because it depends on statically configured constraints and other > data ... try running the entire org.eclipse.emf.validation.tests.AllTests suite > if you didn't already. > It worked when I ran the entire test suite, thanks :)
(In reply to comment #22) > So.. Why does the AbstractValidator creates a simple Status and not a > ConstraintStatus when a validation is succcessful? I mean, if a constraint > evaluation gives true, it doesn't mean that no contraint was evaluated and the > resource information is very useful for deleting the markers. I know it's > easier to just delete the markers in the BatchValidationDelegate class but this > would be pretty awkward to delete on the user side something that should be > updated by the same classe that created it. Hi, Damien, The problem with the IConstraintStatus is that clients would legitimately expect such a status to refer to some constraint that was evaluated. A batch validator can be configured to return a status for each and every constraint evaluation, even the successful ones, but that generally results in more clutter than it is worth. I see at least two options. 1) Create another kind of status (call it ResourceStatus) that the BatchValidator can include in its results for every resource that was validated. The utility can then look for this status and ensure that it knows about resources that reported no problems, in order to clean the markers. 2) Create a dummy IModelConstraint implementation whose ID is private to the framework, and which is not registered in the constraint registry. An OK IConstraintStatus can be created for this constraint on the first root EObject in each resource that is validated. Knowing the private constraint ID, the marker utility can use it as the indicator. Personally, I'm partial to the second approach, because it doesn't introduce any new language (another kind of status object). What do you think? Do you feel comfortable implementing this?
(In reply to comment #24) > (In reply to comment #22) > > So.. Why does the AbstractValidator creates a simple Status and not a > > ConstraintStatus when a validation is succcessful? I mean, if a constraint > > evaluation gives true, it doesn't mean that no contraint was evaluated and the > > resource information is very useful for deleting the markers. I know it's > > easier to just delete the markers in the BatchValidationDelegate class but this > > would be pretty awkward to delete on the user side something that should be > > updated by the same classe that created it. > > Hi, Damien, > Hello, > The problem with the IConstraintStatus is that clients would legitimately > expect such a status to refer to some constraint that was evaluated. A batch > validator can be configured to return a status for each and every constraint > evaluation, even the successful ones, but that generally results in more > clutter than it is worth. > > I see at least two options. > > 1) Create another kind of status (call it ResourceStatus) that the > BatchValidator can include in its results for every resource that was > validated. The utility can then look for this status and ensure that it knows > about resources that reported no problems, in order to clean the markers. > > 2) Create a dummy IModelConstraint implementation whose ID is private to the > framework, and which is not registered in the constraint registry. An OK > IConstraintStatus can be created for this constraint on the first root EObject > in each resource that is validated. Knowing the private constraint ID, the > marker utility can use it as the indicator. I didn't understand. Where would the OK IConstraintStatus be created? I'm afraid I got lost somewhere in the explanation :( Nonetheless, this makes me wonder. Why would an OK IConstraintStatus be created for this dummy IModelConstraint implementation when it is not created for usual model constraints. In AbstractValidator, any OK status is discarded: if ((status != null) && (isReportSuccesses() || !status.isOK())) { if (status.getSeverity() > resultStatus.getSeverity()) { resultStatus = status; if (resultStatus.matches(IStatus.CANCEL)) { // cancel the current validation operation throw new ValidationCanceledException(resultStatus); } } results.add(status); > Personally, I'm partial to the second approach, because it doesn't introduce > any new language (another kind of status object). > > What do you think? Do you feel comfortable implementing this? I think I could do it, it's really good practice to learn my way around Eclipse. I'm just a bit confused as to how to proceed.
(In reply to comment #25) > > I didn't understand. Where would the OK IConstraintStatus be created? I'm > afraid I got lost somewhere in the explanation :( Well, that's the $64,000 question, isn't it? ;-) Clearly, the validator would have to create these OK statuses because any of its clients would expect to be able to use the marker-update utility, and these clients generally have no control over the constraint providers and other extensions. The trick is, how to do it efficiently. The simplest approach would be to query each object for its eResource() as it is validated, and ensure that each resource is represented by one of these special OK statuses. However, this would be an extra overhead, which could be considerable in cases of large, deeply-nested resources. It also isn't a certainty that querying objects for their eDirectResource() will find *any* resources at all, because a large selection of objects may not include any resource roots. In general, finding the resources that are covered by a validation operation is not a simple matter, further complicated by cross-resource containment and even the possibility of non-hierarchical traversal via ITraversalStrategy extensions. It's beginning to look like this would require, in the average case, some performance degradation in order to support the enhanced marker management. It might be necessary to introduce a new setting in the IBatchValidator interface that enables marker updating (off by default), which would be a more difficult API change to introduce in M7 than the addition of static operations in the MarkerUtil. I know of some clients that do implement the IBatchValidator interface despite that they aren't supposed to ... > Nonetheless, this makes me wonder. Why would an OK IConstraintStatus be created > for this dummy IModelConstraint implementation when it is not created for usual > model constraints. Well, somehow we have to communicate from the validator to the marker utility, and the statuses currently are the carrier for such communication. > In AbstractValidator, any OK status is discarded: So, some OK statuses would have to be recognized and handled differently. That's not difficult. > > Personally, I'm partial to the second approach, because it doesn't introduce > > any new language (another kind of status object). > > > > What do you think? Do you feel comfortable implementing this? > > I think I could do it, it's really good practice to learn my way around > Eclipse. I'm just a bit confused as to how to proceed. And, now I'm a bit hesitant to proceed in the M7 milestone at all. Your thoughts? Perhaps on reflection, you can see a simpler way.
I think I can work with Damien's patches to complete a solution for the 1.3 release.
Committed new marker-update API in MarkerUtil, based on Damien's patch and completed with handling of problem of updating markers for resources that reported no problems. This uses a new IValidator.Option API, with the IBatchValidator providing an OPTION_TRACK_RESOURCES option to enable marker updating.
Restore original target after milestones were deranged.
