Community
Participate
Working Groups
If you get an error or warning in a system include file (which typically reside outside of the workspace directory) then you can't give a file reference to the marker. The result is that the user cannot double click on the problem and be taken to the location in the editor. ErrorParserManager has a couple of utility functions to get an IFile for you from a path but unless the path resides in the workspace it won't work. Even if the error parser implemetor creates an IFile resource for the file and passes it to generateMarker it still doesn't work. In this case nothing is shown in the problems view at all. I think there is probably an exception being thrown.
You can only create markers on IResources. Since the external files aren't IResources, you simply can't do this. But it is a good requirement that needs a solution. Hopefully someone will think of something innovative and come up with one.
Here is something innovative: - If the error parser cannot create an IFile object for a given path then it attaches the problem marker to the project. That's why the file cannot be opened in a simple way from the problems view - A new marker type could be defined which has an additional attribute like "externalLocation" (derived from the standard C/C++ problem marker) - The error parser could be made to use the the new type, still use the project as resource but store the original path of the error in the marker's externalLocation attribute - An popup menu object contribution for the new marker type could be created, as an analogy of the Delete C/C++ Marker contribution which allows manually deleting C/C++ Markers. The new contribution would say: "Open External File". An action would be provided to open the external file in an editor and place the cursor in the appropriate position. The main drawback of this is the fact that the user must know or guess that while double-clicking does not work, he can still get the file opened via a popup menu action. I would have liked to somehow override the default double-click action for the Problems view but I could not figure out how to do this. The current action which is caused by the double-click is hard-coded into the ProblemView class and it's framework Another drawback is that annotiations probably also won't work for the external file. I'll have to think about this some more, maybe we can configure the CEditors annotation access ... Anyway I would like opinions on whether this is worth doing in spite of the drawbacks, or whether other, more elegant ways of doing this can be imagined.
(In reply to comment #2) > - A new marker type could be defined which has an additional attribute like > "externalLocation" (derived from the standard C/C++ problem marker) I think this could be applied to any text marker. > Another drawback is that annotiations probably also won't work for the external > file. I'll have to think about this some more, maybe we can configure the > CEditors annotation access ... > Anyway I would like opinions on whether this is worth doing in spite of the > drawbacks, or whether other, more elegant ways of doing this can be imagined. Have you looked at ExternalSearchAnnotationModel and DebugMarkerAnnotationModel? They try to solve the same thing, kind of. ExternalSearchAnnotationModel does not provide any annotations for markers currently, but can be quite easily changed to do so. The drawback is that it is only used when the file is opened with ExternEditorInput and ExternalSearchEditor. DebugMarkerAnnotationModel supports breakpoints only, but works generically for all editor inputs and editors. I think we need a common solution for any marker type and any editor input/editor, otherwise it depends on the way a file is opened whether it will show problem markers or breakpoints - but not both.
Created attachment 49345 [details] This patch introduces the new marker type and the action to open the editor Submitting an initial patch to create a sketch of a solution. What this patch does: - Define a new marker type - Change the error parser infrastructure so that markers of this type, including the external location, are generated - An action contribution to open an editor for the external location Second thoughts about this implementation: - Perhaps a new marker type is not required. We could just define the org.eclipse.cdt.core.problem marker to always have the externalLocation attribute. In many cases this attribute would simply not be set. Known defects: - my new externalProblem markers don't have the "Delete C/C++ Marker" popup menu item which is contributed for the standard problem marker. I thought that by setting the super element type to org.eclipse.cdt.core.problem the menu item would be inherited. Any ideas? - The IMarkerGenerator interface must be extended by another method signature. Currently to my knowledge only classes in org.eclipse.cdt.core.tests get broken. Should I go to more trouble to avoid breaking things? Or is the extension of the interface ok? - The way the editor is opened and directed to the correct line is done without much internal knowledge. Items to be done: - Make sure that the ExternalSearchEditor gets opened. - Modify marker support so that the problem markers for the external file are also displayed.
Created attachment 49600 [details] Do the marker definitions and also support marker annotation display in external editor This patch does all the patch before did plus adding an OpenExternalProblemAction which opens the ExternalSearchEditor for the referenced location. By a little tweak to the ExternalSearchAnnotationModel the markers are now being displayed in the correct place even in the external editor. Design considerations: - Decide whether a new marker type is necessary, or a new attribute for the "normal" CDT problemMarker is sufficient - Is it OK to reuse the ExternalSearchEditor for displaying problem markers? (I think it is) - Review and clean-up the implementation - Provide unit tests - Create a new bugzilla which will depend on #155379 and will be resolved when the external editor is opened upon double-click. In this way this current bug can be resolved and we don't forget that for external markers opening the external editor should ideally be triggered from a double-click and not from an extra context menu entry. - Create a nice icon for the action (suggestions, anybody?) I'd love to have opinions on my suggested implementation.
(In reply to comment #5) > By a little tweak to the ExternalSearchAnnotationModel the > markers are now being displayed in the correct place even in the external > editor. The annotation model should filter the markers according to the file location, otherwise it would include markers for all external files associated with the same resource (#isAcceptable). I would also vote for reusing the existing marker type(s).
Created attachment 53293 [details] Reworked implementation which filters by location The implementation has been sleeping (maturing, of course) for two months now. I have reworked it to suit the following requirements: - I am reusing the standard C/C++ marker type and it has an additional attribute which is filled only for problems on external files. - The annotation model does filter by location now which is necessary because markers for multiple files are all stuck on to the containing project. (isAcceptable() implementation) I tried to use existing JUnits to verify that my implementation does not break any existing things but found it was pretty confusing. Numerous tests from the org.eclipse.cdt.core.tests fail, also from tests which are related to external error parsing, such as GCCErrorParserTests. These tests are not in the nightly build test suite so I have no idea whether they should work or not. Btw I also tried in a different workspace to run them with the HEAD from the repository unmodified and they fail even under such circumstances. I felt that the IMarkerGenerator interface was a little clumsy with it's long parameter lists, and I worked up a suggestion to make the ErrorParserManager.Problem class public and top-level under the more descriptive name ProblemMarkerInfo, and to use this wrapper object in the IMarkerGenerator.addMarker() interface. I am leaving the old version in place, but have marked it as deprecated. For the implementation of the filtering in ExternalSearchAnnotationModel.isAcceptable() I had to get information about the project and external location path of the markers into the annotation model. I did it by extending the ExternalFileEditorInput, introducing among others a new constructor. I am confident that it will still be working correctly also in the old contexts, but it would be good to hear some comments from somebody who knows his way around here (Anton?) So, here is the suggested patch, any more comments or opinions?
Looks good to me, just some comments on the ExternalSearchAnnotationModel: - The attribute for the external location is defined different in the plugin.xml (don't know if this attribute declaration has any effect at all). - isAcceptable allows only C problem markers. Rather, I'd suggest to not filter the type. This allows to use the mechanism for other marker types, too (e.g. breakpoints). - retrieveMarkers should not findMarkers with DEPTH_INFINITE, because we don't want to get all markers of all resources inside a project. - The model does not listen to resource changes, ie. it does not update itself if eg. a marker is deleted. Actually, I think we can base the annotation model on the ResourceMarkerAnnotationModel which handles this already. - One last point ;-) The ExternalEditorInput could be a little bit smarter and guess the marker resource from the ITranslationUnit (ie. the IProject it is associated with), thus the markers also appear when the input has been created without explicit marker resource, but there still remains the problem that a file opened with an IStorage only, will not show the problem markers. As a workaround, one could get all markers from the workspace root with DEPTH_ONE to cover all projects...
Anton, thanks for your fast reply. Mine is not so fast. > - The attribute for the external location is defined different in the > plugin.xml (don't know if this attribute declaration has any effect at all). I changed this. The declaration may be important for the persistence of the marker. Even though attributes can apparently created and read without being declared the persistence mechanism may depend on the declarations. I did not try this, however. > - isAcceptable allows only C problem markers. Rather, I'd suggest to not filter > the type. This allows to use the mechanism for other marker types, too (e.g. > breakpoints). Right. Filtering by the existence of the externalLocation attribute is strict enough. > - retrieveMarkers should not findMarkers with DEPTH_INFINITE, because we don't > want to get all markers of all resources inside a project. Sure. DEPTH_ZERO is perfectly enough. > - The model does not listen to resource changes, ie. it does not update itself > if eg. a marker is deleted. Actually, I think we can base the annotation model > on the ResourceMarkerAnnotationModel which handles this already. OK, I changed this too. Made the implementation of the ExternalSearchAnnotationModel smaller and cleaner. I have two new concerns: - Will the original external search stuff still be displayed correctly after my changes. Markers from external search will not have the externalLocation attribute set. They should be filtered out. How can I test this? - I have now figured out how to create a translation unit from an external include file's path and how to open the C editor on the file. This is even better that the external editor because now the outline also is alive. The drawback at the moment is that I need to fiddle with another marker annotation model (the C editor's) and have not yet figured out where to start. (Attempts with the TranslationUnitAnnotationModel did not succeed ...) A few pointers would be greatly appreciated. I'll post the updated patch shortly ...
Created attachment 53779 [details] This patch has learned how to open the C editor This patch can open the C editor on external files which have error markers. It does not yet display the markers in the editor as annotations. I'll have to do some more research for this ...
(In reply to comment #9) > I have two new concerns: > - Will the original external search stuff still be displayed correctly after my > changes. Markers from external search will not have the externalLocation > attribute set. They should be filtered out. How can I test this? Search does not set markers anymore, instead the search adds annotations dynamically when an editor is opened, therefore this is not influenced by the change. > - I have now figured out how to create a translation unit from an external > include file's path and how to open the C editor on the file. This is even > better that the external editor because now the outline also is alive. The > drawback at the moment is that I need to fiddle with another marker annotation > model (the C editor's) and have not yet figured out where to start. (Attempts > with the TranslationUnitAnnotationModel did not succeed ...) A few pointers > would be greatly appreciated. I have committed a change to CDocumentProvider which uses the ExternalSearchAnnotationModel as fallback model. Thus the markers for external files are also shown in the CEDitor (I had to comment out part of the change as it depends on your patch).
(In reply to comment #11) > I have committed a change to CDocumentProvider which uses the > ExternalSearchAnnotationModel as fallback model. Thus the markers for external > files are also shown in the CEDitor (I had to comment out part of the change as > it depends on your patch). Works great. I'll attach the patch including your changes for documentation purposes and commit it now. Thanks a lot, Anton!
Created attachment 54151 [details] Patch with modified CDokumentProvider as suggested by Anton Leherbauer The patch in the form in which I'll commit it.
Patch applied, hope I'm not stepping on anyone's toes ...
Thanks for the fix. I've been hooking it up with our builder and errors parsers and it's working great. One problem I noticed though is when you open problems in source files that are external to the workspace, they are not opened as source files (no c icon in editor tab). The reason appears to be that CoreModel#createTranslationUnitFrom only looks at includes (IIncludeReference's). I guess this is sort of outside of the scope of this particular bug so maybe I should log a new one for it?
(In reply to comment #15) > I guess this is sort of outside of the scope of this particular bug so maybe I > should log a new one for it? Warren, since this bug is now long closed, and your problem is a trifle different, I'd prefer if you'd open a new one. Thx, Norbert
A possible enhancement could be to use the predefined MarkerViewUtil PATH_ and NAME_ATTRIBUTEs. These allow alternative resource and path strings to be shown in the problem view. In case of an external file they could be used to have the resource field refer to the external file, and path to the external file path.
Created attachment 89392 [details] Patch to show external file name and path in Problems view Resource and Path fields.
(In reply to comment #18) > Created an attachment (id=89392) [details] > Patch to show external file name and path in Problems view Resource and Path > fields. > The idea is good, but it introduces a dependency of org.eclipse.cdt.core on org.eclipse.ui.ide.
If the extra dependency is the only issue then may I suggest to just copy the 2 attribute strings into cdt.core. The worst that can happen is the old behaviour.
