Bug 352166

Comment 1 Sami Wagiaalla 2011-07-14 16:55:35 EDT

Created attachment 199710 [details]
adds the needed method

Comment 2 Andrew Gvozdev 2011-07-14 18:21:05 EDT

Please, provide more details why you need a new method. JavaDoc for ProblemMarkerInfo states that the class is not intended to be subclassed. What kind of information do you want to pass and where?

Comment 3 Sami Wagiaalla 2011-07-15 09:31:33 EDT

When a build fails due to a missing dependency we (Autotools plugin) would like to provide the user with the option of installing the missing dependency.
To do so I created AutotoolsProblemMarkerInfo extends ProblemMarkerInfo and added a type parameter to it. The type parameter specifies whether this problem is a missing dependency or otherwise, and what kind of dependency it is (header file, library, executable, etc)

Comment 4 Andrew Gvozdev 2011-07-18 11:07:00 EDT

(In reply to comment #3)
> When a build fails due to a missing dependency we (Autotools plugin) would like
> to provide the user with the option of installing the missing dependency.
> To do so I created AutotoolsProblemMarkerInfo extends ProblemMarkerInfo and
> added a type parameter to it. The type parameter specifies whether this problem
> is a missing dependency or otherwise, and what kind of dependency it is (header
> file, library, executable, etc)
It is not clear to me how the data from ProblemMarkerInfo would get to Autotools plugin. Could you clarify?
AFAICS ProblemMarkerInfo ends in ACBuilder where it gets converted to IMarker and any custom data elements get ignored there.

Comment 5 Sami Wagiaalla 2011-07-18 16:14:46 EDT

Hmm... That is true so there is no point in extending ProblemMarkerInfo.
What would be a way to achieve this then ? I could patch ProblemMarkerInfo to have a misc info field and patch ACBuilder to preserve that if that patch would be acceptable.

Comment 6 Andrew Gvozdev 2011-07-21 11:17:35 EDT

(In reply to comment #5)
> Hmm... That is true so there is no point in extending ProblemMarkerInfo.
> What would be a way to achieve this then ? I could patch ProblemMarkerInfo to
> have a misc info field and patch ACBuilder to preserve that if that patch would
> be acceptable.
I think it might be beneficial to make ProblemMarkerInfo more generic to be able to handle arbitrary IMarker tags. Currently it can deal with just a few hardcoded ones.
How about following constructor?
  public ProblemMarkerInfo(IResource file, int lineNumber, String desciption, int severity, Map<String, String> attributes)

Comment 7 Sami Wagiaalla 2011-07-26 09:45:56 EDT

That would be great! Can you do that or would you like me to prepare a patch ?

Comment 8 Andrew Gvozdev 2011-07-26 10:06:08 EDT

(In reply to comment #7)
> That would be great! Can you do that or would you like me to prepare a patch ?
I have no plans to implement that myself but if you submit a patch I'll work with you to get it to CDT.

Comment 9 Sami Wagiaalla 2011-08-10 14:24:03 EDT

Created attachment 201263 [details]
Add attributes constructor

Comment 10 Sami Wagiaalla 2011-08-10 14:24:05 EDT

Created attachment 201264 [details]
mylyn/context/zip

Comment 11 Sami Wagiaalla 2011-08-10 14:27:29 EDT

Sorry for the delayed reply.

A couple of points about the latest attached patch:

- I added interface for setting the type because the QuickFix in autotools will be set up to
  handle only errors of autotools type.
- I left addProblemMarker because modifying generateExternalMarker to accept the attributes
 map made for very awkward looking code when creating the ProblemMarkerInfo object
- Related to the previous point I think that it would look better if we remove the attributes map
  from the constructor and add setAttribute(String, String) and getAttrbiute(String)
  
What do you think ?

Comment 12 Andrew Gvozdev 2011-08-10 15:34:35 EDT

There is a lot of unrelated stuff in the patch and it does not apply in eclipse. Could you please clean-up the patch and resubmit? Perhaps you need to rebase against the latest master?

Comment 13 Sami Wagiaalla 2011-08-10 16:12:48 EDT

Created attachment 201275 [details]
Create more generic API for setting ProblemMarkerInfo

My bad. I should have looked at that patch before attaching it. How embarrassing...

This is the correct patch. I will attach another one once I have rebased on origin/master just in case.

Comment 14 Andrew Gvozdev 2011-08-10 17:27:07 EDT

The changes look pretty reasonable to me, that's pretty much how I would do it given the existing codebase. I only have a few rather formal comments:

- You need to add @since tags to new API methods to satisfy API tools. Just add "@since 5.4" in JavaDoc, see ErrorParserManager.getWorkingDirectoryURI() for example. 5.4 is the current version of cdt.core plugin.
- Please, add comments to all new public methods you added. ProblemMarkerInfo currently has no JavaDoc at all, could you do an extra and add basic comments to it?
- Watch what you copy-paste - "desciption" is misspelled in ProblemMarkerInfo constructor.

- We are required to update the second date in copyright header to the current year. See http://wiki.eclipse.org/CDT/policy#Copyright.
- I encourage to add your name to the headers of java files. If you do so, it would be good to mention "bug 352166".

Comment 15 Andrew Gvozdev 2011-08-10 17:31:03 EDT

... and please do not introduce unused imports warnings :)

Comment 16 Sami Wagiaalla 2011-08-11 11:24:30 EDT

Created attachment 201328 [details]
Create more generic API for setting ProblemMarkerInfo

Thanks for the review :). This is a revised patch.
I made the suggested fixes, and added setAttribute and getAttribute API.

Comment 17 Andrew Gvozdev 2011-08-12 13:29:39 EDT

Pushed on master, thanks for the patch. I just did a few very cosmetic changes on top, such as in JavaDoc for better readability and fixed the license header. Eclipse Foundation is a bit picky with the format and does not allow 3 years, only 2 - it's a range.

Comment 18 Sami Wagiaalla 2011-08-12 14:00:43 EDT

Great! Thanks for the help :)

Comment 19 CDT Genie 2011-08-12 14:23:02 EDT

*** cdt git genie on behalf of Andrew Gvozdev ***

    bug 352166: Cosmetics

[*] http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=67634ea7a882285671f6cee39ff94504a35f3cb8

Comment 20 CDT Genie 2011-08-12 14:23:04 EDT

*** cdt git genie on behalf of Sami Wagiaalla ***

    bug 352166: ErrorParserManager does not have an API for adding markers

[*] http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=e34011c34b7d18a4021219864bcbc5b9fe638deb

Comment 21 CDT Genie 2011-09-27 12:23:08 EDT

*** cdt git genie on behalf of Andrew Gvozdev ***

    bug 352166: Expressed IMarkerGenerator severity constants via
    corresponding IMarker ones.

[*] http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=fe0611ef330dfd6c08aec5a8eeb8566e844ff990