Bug 351963

Comment 1 Sebastian Zarnekow 2011-07-13 09:07:30 EDT

We could introduce an optional extension interface for the language specific IResourceValidator to allow for custom marker types per language. It could be used from MarkerUpdaterImpl.addMarkers(IFile, Resource, IProgressMonitor)

What do others think?

Comment 2 Wladimir Safonov 2011-07-26 07:53:01 EDT

I think this would be a quite convenient feature. When domain experts have to work with the tool and resolve problems, "Xtext Check (Fast)" as a problem type appears confusing to them as opposed to a concise problem name from their domain. It would be even better, if one could supply this type not just per language, but also for groups of specific checks. This would allow to further classify all problems generated for a specific language into meaningful groups of domain-related problems.

I think technically this can be solved if org.eclipse.xtext.validation.Issue would not be just constrained by CheckType types, but would allow to specify custom issue types. Check methods can then supply these in a optional parameter to the warning()/error() methods and it will be evaluated when the marker is actually set in MarkerUpdaterImpl.addMarkers(IFile, Resource, IProgressMonitor). Of course the language developer will have to register futher required marker types himself in the language ui-Plugin. 

What do you think, Sebastian?

Comment 3 Michael Clay 2011-07-26 16:53:28 EDT

(In reply to comment #1)
> We could introduce an optional extension interface for the language specific
> IResourceValidator to allow for custom marker types per language. It could be
> used from MarkerUpdaterImpl.addMarkers(IFile, Resource, IProgressMonitor)
> 
> What do others think?

what about the following proposal for this extension interface?


public interface IResourceValidatorUIExtension {

	/**
	 * @param issue
	 *            the issue to map to a corresponding marker identifier
	 * @return the marker identifier (from extension point <code>org.eclipse.core.resources.markers</code> )
	 *         corresponding to the given issue
	 */
	String mapMarkerType(Issue issue);

	/**
	 * @param file
	 *            the file to delete the markers from
	 * @param issues
	 *            the actual list of validation issues for the given file
	 */
	void deleteMarkers(IFile file, List<Issue> issues);

	/**
	 * Removes the remaining markers in case of file deletion
	 * 
	 * @param file
	 *            the file to delete the markers from
	 */
	void deleteAllMarkers(IFile file);

}

Comment 4 Sebastian Zarnekow 2011-07-26 17:13:04 EDT

I think the suggested extension interface is too limited. That's why I had something more general in mind, e.g.

interface IResourceValidatorUIExtension {

 void updateValidationMarkers(IFile file, Resource resource, IProgressMonitor monitor, CheckMode mode);

 void deleteValidationMarkers(IFile file, CheckMode checkMode);

}

#updateValidationMarkers would allow to get full control on the validation step in the build since even the invocation of #validate would be part of the implementation.

Comment 5 Michael Clay 2011-07-26 17:51:42 EDT

i thought of the IResourceValidatorUIExtension as some kind of logic which will be added to the existing MarkerUpdaterImpl#addMarkers only if the configured IResourceValidator implements this interface e.g.

+                       if (resourceValidator instanceof IResourceValidatorUIExtension) {
+                               IResourceValidatorUIExtension resourceValidatorUIExtension = (IResourceValidatorUIExtension) resourceValidator;
+                               resourceValidatorUIExtension.deleteMarkers(file,list);
+                       }
                        for (Issue issue : list) {
-                               markerCreator.createMarker(issue, file, MarkerTypes.forCheckType(issue.getType()));
+                               String markerType = MarkerTypes.forCheckType(issue.getType());
+                               if (resourceValidator instanceof IResourceValidatorUIExtension) {
+                                       IResourceValidatorUIExtension resourceValidatorUIExtension = (IResourceValidatorUIExtension) resourceValidator;
+                                       markerType = resourceValidatorUIExtension.mapMarkerType(issue);
+                               }
+                               markerCreator.createMarker(issue, file, markerType);
                        }

because i assume that marker type creation will not be that different and is already covered with the existing MarkerCreator and regarding the validation impl (collecting and converting syntax and semantic errors to issues) iam not sure if this kind of full control is needed because of the already existing 'validation extension' hooks by means of guice..what kind of validation customization do you expect which can't be tackled with the existing hooks?



(In reply to comment #4)
> I think the suggested extension interface is too limited. That's why I had
> something more general in mind, e.g.
> 
> interface IResourceValidatorUIExtension {
> 
>  void updateValidationMarkers(IFile file, Resource resource, IProgressMonitor
> monitor, CheckMode mode);
> 
>  void deleteValidationMarkers(IFile file, CheckMode checkMode);
> 
> }
> 
> #updateValidationMarkers would allow to get full control on the validation step
> in the build since even the invocation of #validate would be part of the
> implementation.

Comment 6 Sebastian Zarnekow 2011-07-27 01:05:42 EDT

The extension interface should allow to implement the file specific logic MarkerUpdaterImpl.updateMarker(ResourceSet, ImmutableList<Delta>, IProgressMonitor)

The default implementation of the extension would do exactly the same thing as MarkerUpdaterImpl does. However, clients would be able to replace this implementation, e.g. implement client specific guards on the IFile (#readonly is probable not the check that everybody wants to perform), or trigger other implementations that are capable to create markers, e.g. existing compilers. There wouldn't be a need to translate the output of existing infrastructure to Issues and back again for the purpose of marker creation (which could lead to lost information).

Comment 7 Michael Clay 2011-07-27 04:18:48 EDT

(In reply to comment #6)
> The extension interface should allow to implement the file specific logic
> MarkerUpdaterImpl.updateMarker(ResourceSet, ImmutableList<Delta>,
> IProgressMonitor)
> 
> The default implementation of the extension would do exactly the same thing as
> MarkerUpdaterImpl does. However, clients would be able to replace this
> implementation, e.g. implement client specific guards on the IFile (#readonly
> is probable not the check that everybody wants to perform), or trigger other
> implementations that are capable to create markers, e.g. existing compilers.
> There wouldn't be a need to translate the output of existing infrastructure to
> Issues and back again for the purpose of marker creation (which could lead to
> lost information).

i see thanks for the clarification..we also need to call deleteValidationMarkers  in case of file deletion i.e. CheckMode is optional or add another deleteMarker method to the interface without CheckMode...if its ok for you i would like to come up with a first proposal of this extension impl.

Comment 8 Sebastian Zarnekow 2011-08-28 11:18:15 EDT

(In reply to comment #7)
> i see thanks for the clarification..we also need to call
> deleteValidationMarkers  in case of file deletion i.e. CheckMode is optional or
> add another deleteMarker method to the interface without CheckMode...if its ok
> for you i would like to come up with a first proposal of this extension impl.

Hi Michael,

IFile#delete will delete all associated markers. There is no need do a special #deleteValidationMarkers step. A first shot of the optional extension would be great.

Comment 9 Michael Clay 2011-08-29 16:13:34 EDT

Created attachment 202359 [details]
proposed patch

Comment 10 Sven Efftinge 2011-08-30 03:02:14 EDT

looks good. Please apply.

Comment 11 Michael Clay 2011-08-30 13:00:10 EDT

pushed to master

Comment 12 Knut Wannheden 2011-08-30 14:08:08 EDT

Question: I know this is a bit late now, but I was wondering why there is a "UI" in the name and why the service is in the UI plug-in.

Comment 13 Alex Ruiz 2011-08-30 17:08:58 EDT

Thanks so much for the fix. I tried several nightly builds. FYI, The latest ones simply don't work. 

I finally got a good one: https://hudson.eclipse.org/hudson/view/Modeling/job/Xtext-nightly-HEAD/1325/

Cheers,
-Alex

Comment 14 Alex Ruiz 2011-08-30 17:09:40 EDT

Please ignore previous e-mail. Posted on the wrong bug :D

Comment 15 Karsten Thoms 2017-09-19 17:47:26 EDT

Closing all bugs that were set to RESOLVED before Neon.0

Comment 16 Karsten Thoms 2017-09-19 17:58:34 EDT

Closing all bugs that were set to RESOLVED before Neon.0