Bug 119444 - [Markers] Hierarchical Problems view: SubCategory contributions should not load plug-ins
Summary: [Markers] Hierarchical Problems view: SubCategory contributions should not lo...
Status: RESOLVED WORKSFORME
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 3.2   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: ---   Edit
Assignee: Tod Creasey CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 119452
  Show dependency tree
 
Reported: 2005-12-06 12:11 EST by Markus Keller CLA
Modified: 2006-01-25 14:51 EST (History)
6 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Markus Keller CLA 2005-12-06 12:11:52 EST
I20051130-1215

[spawned from bug 114760 comment 8]

Plug-ins can only contribute categories by extending org.eclipse.ui.ide.markerSupport with a subCategoryProvider and providing an ISubCategoryProvider (in the schema, this is wrongly defined as ICategoryProvider). The problems view therefore has to load all plug-ins that contribute such a subCategory. This is IMO not acceptable.

Im not sure what's the best way to solve this problem, but here are two suggestions (I would prefer A):

(A) The subcategories are stored in an additional attribute on IMarker. This would make the ISubCategoryProvider obsolete, since the Problems view could look up the categories without further support. Clients who create markers would just set the category on creation.

(B) Clients specify subCategories declaratively in their plugin.xml based on already existing marker attributes. Since they would typically have to map a markerType-specific id to a category label, the ui.ide plug-in would have to provide support for such a mapping in xml (e.g. something like
<subCategory attribute="id" markerTypeId="org.eclipse.jdt.core.problem">
    <mapping match="1.*" categoryName="%cat_buildpath"/>
    <mapping match="(1[5-9])|20" categoryName="%cat_syntax"/>
 ...
)
Comment 1 Markus Keller CLA 2005-12-07 13:05:30 EST
Since (A) is deemed to be too memory-intensive and (B) would be too hard for clients to implement, we propose C:

(C) Contributed categories are only shown when the contributing plug-in has already been loaded. Otherwise, they are rendered with a gray "not loaded". The categories are loaded if
- the plug-in is activated, or
- the user clicks the category's column header (to group by that category).
Comment 2 Tod Creasey CLA 2005-12-07 13:56:58 EST
Don't you think this would be wierd for the user? For the JDT case these markers are from Core - opening the package explorer would then cause a refresh in the problems view and an addition of categories for no apparent reason. No other contribution we use works this way.

Having said that I don't actually have a better answer just yet so let me think on it. Going this route is setting a precedent that we want to ve sure we are happy with.

JDT is currently our only use case for this support so it might not be worth it at all in the end.
Comment 3 Tod Creasey CLA 2005-12-08 10:38:00 EST
Nick had a good idea that I think will solve both issues (if it is possible in JDT land).
Assuming that you could determine the value of the sub-category based on an attribute then all we would need to declare is what attribute to look for and what the displayed name maps to.

For example lets assume jdt has an attribute "JDTType" which we can use. The markup would look something like

<subCategory attribute = "JDTType" label = "Java category">
    <markerTypeReference id="org.eclipse.jdt.problemmarker"/>
    <attributeMapping value="1" label="NLS">
    <attributeMapping value="2" label="Build ">
    <attributeMapping value="3" label="Syntax">
    <defaultAttributeMapping label="Other Java">
</subcategory>

I recall Philippe saying that you already had the info you need - is it in an attribute that you could define in this way?
Comment 4 Martin Aeschlimann CLA 2005-12-08 11:05:15 EST
Yes, sounds like a good alternative. I think we still keep the possibility to use a ISubCategoryProvider so a UI plugin is free to contribute a category without requiring a core plugin to add a new attributes.

My favourite solution for the activation problem is:
- We don't try to restore the problems view's grouping if the used category
hasn't been loaded yet. A default grouping or no grouping will used instead
- We don't show columns on restore if its category hasn't been loaded yet.
Either it will be removed (the user would have to add it again) or it will be
grayed out, labels will say 'Unspecified'. A click on the header would force the load of the category.

(from bug 119864)
Comment 5 Tod Creasey CLA 2005-12-08 11:14:39 EST
Yes - I will keep the ISubCategoryProvider as well as I suspect this is not as easy for others.

So if I were to put this in is there an attribute on a jdt problem that you could use for this type of specification?
Comment 6 Martin Aeschlimann CLA 2005-12-08 11:20:26 EST
Of course, but let me check with Philippe. Philippe, any objections in introducing a new marker attribute for the Java categories?
Comment 7 Tod Creasey CLA 2005-12-08 11:32:43 EST
Actually we want to avoid this for size reasons. IIRC core already has an attribute they can test. Philippe?
Comment 8 Markus Keller CLA 2005-12-08 11:42:51 EST
> Actually we want to avoid this for size reasons.

Are these < 20 Bytes really relevant? I mean, we easily loose that amount of memory also when e.g. the description message is expressed in a verbose language (it is localized). The attribute approach (which was my (A)) would save us from all the hassle about plugin activation, UI state restoration, etc.
Comment 9 Tod Creasey CLA 2005-12-08 12:04:07 EST
Looking at my workspace I have 6000 markers so lets say it creates an extra 100K - that is not too bad but we really need to be aware of memory issues as is very hard on downstream components.

Having said thatt with the idea we have you could just create a single byte attribute and map the number to a string using this markup (i.e. "1" = NLS, "2" = Build and so on). This would reduce the size greatly/

If it is good for you then we are all set. I was just checking with JDT core as I think they already have an attribute they can use - the initial idea of having an ICategoryProvider was due to JDT being concerned that they couldn't express the logic required simply.

Comment 10 Nick Edgar CLA 2005-12-08 12:12:33 EST
Just FYI: John recently made an optimization to essentially intern Integer attributes with value 0 or 1, and it made a significant improvement.
Comment 11 John Arthorne CLA 2005-12-12 11:59:54 EST
It certainly makes sense to avoid adding an extra attribute to every problem marker if it can be avoided. The interning in core will only help for Integer values 0,1, and 2 (although this can be expanded if necessary).

The theory behind the suggestion in comment #3 is that each Java problem already has an attribute that can be tested (the IJavaModelMarker.ID attribute), so an additional attribute to denote the category is not necessary.  However, this assumes there is a very simple mapping from the value of IJavaModelMarker.ID to the category name that can be expressed declaratively.  I suspect this is not the case, and performing a non-trivial computation to map id->category on the fly for each marker will likely be too expensive. I.e., I suspect you are stuck with adding an integer category id to each marker.
Comment 12 Philipe Mulet CLA 2005-12-12 15:32:08 EST
Actually, a problem ID allows to infer a category. I was waiting to add such an API until categorization when far enough we understand the requirement.

I would add: CategorizedProblem#(static)computeCategoryID(int problemID)
Note this type already defines the following set of categories:
	
/**
 * List of standard category IDs used by Java problems, more categories could be added 
 * in the future.
 */
public static final int CAT_UNSPECIFIED = 0;
public static final int CAT_BUILDPATH = 10;
public static final int CAT_SYNTAX = 20;
public static final int CAT_IMPORT = 30;
public static final int CAT_TYPE = 40;
public static final int CAT_MEMBER = 50;
public static final int CAT_JAVADOC = 60;
public static final int CAT_CODE_STYLE = 70;
public static final int CAT_POTENTIAL_PROGRAMMING_PROBLEM = 80;
public static final int CAT_NAME_SHADOWING_CONFLICT = 90;
public static final int CAT_DEPRECATION = 100;
public static final int CAT_UNNECESSARY_CODE = 110;
public static final int CAT_UNCHECKED_RAW = 120;
public static final int CAT_NLS = 130;
Comment 13 Philipe Mulet CLA 2005-12-12 15:34:15 EST
The only problem is that categorization will force jdt/core to load which doesn't feel appropriate.
Comment 14 Tod Creasey CLA 2005-12-12 16:15:39 EST
JDT core will have had to load to generate the markers anyways correct?

What is the name of this attribute? I wouldn't mind trying to mock this up.
Comment 15 Nick Edgar CLA 2005-12-12 16:27:52 EST
> JDT core will have had to load to generate the markers anyways correct?

Yes, but after restarting it may not be loaded.  One of the original design points of the Tasks/Problems view was to not require contributing plug-ins to be activated just to display the markers.


Comment 16 Tod Creasey CLA 2005-12-12 16:36:41 EST
What we are advocating has nothing to do with JDT core loading (unless I am missing something). Once the markers have the attribute there is nothing further for JDT core (or UI for that matter) to do.

JDT UI will be marking up this mapping declaratively - taking your example below we would have something like:

<subCategory attribute = "JDTType" label = "Java category">
    <markerTypeReference id="org.eclipse.jdt.problemmarker"/>
    <attributeMapping value="130" label="NLS">
    <attributeMapping value="10" label="Build ">
    <attributeMapping value="20" label="Syntax">
    <defaultAttributeMapping label="Other Java">
</subcategory>
Comment 17 Tod Creasey CLA 2005-12-13 10:21:22 EST
So looking at ProblemReporter#getProblemCategory I see the issue with the current support - some conversion of the number has to occur first (bit shifts in this case).

Looking at the code it appears to be too complex to put into any sort of markup so I think these are the options

1) Add a category provider and live with the plug-in activation. It would have to be JDT UI that got activated (due to the fact that the extension point is in the IDE plug-in)

2) Add a new attribute. John figures it will be at least 20 bytes per attribute (based on the size of the value). For my example plug-in with 6000 problems this amounts to 120K of extra space.

3) Go with 1) but consider the lazy loading idea that Martin had. My concern with this is how it would look to the user. If we go and add extra categories as the result of plug-in activation it will be very confusing.

One option I was considering is the idea that we do not apply the category providers until the user does something - i.e. we replace them with marker type initially. This will not cause plug-in activation but won't shock the user either.

So for instance on shutdown your world looks like:

NLS Problem
    prob1
    prob2
Build Problem
    prob3
    prob4
PDE problem
    prob5 
    prob6

On restart it would look like
Java Problem
    prob1
    prob2
    prob3
    prob4
PDE problem
    prob5 
    prob6

until the user selects the column again (activating JDT UI if it hasn't already been) when it would be restored to 

NLS Problem
    prob1
    prob2
Build Problem
    prob3
    prob4
PDE problem
    prob5 
    prob6
Comment 18 Nick Edgar CLA 2005-12-13 11:02:51 EST
I really dislike the idea of introducing an implicit mode based on whether plug-ins are activated or not.  This is not something the end user should be aware of.  If we are unable to avoid plug-in loading to provide a decent user experience, then we should just take the hit.
Comment 19 John Arthorne CLA 2005-12-13 11:04:41 EST
I'm sure I missed part of the discussion, but what is the problem with using the marker type to represent categories, as JDT does already with build path problems? I.e., create sub-types of the java problem type for each category that you want to display. It seems that categorization is the main reason for marker types in the first place, so having unrelated mechanisms of "category" and "type" on markers seems like overload.  If there are multiple purposes for the marker type that are being overloaded, perhaps we could add attributes to the marker type extension that allows you to express that a given sub-type is used for the purpose of categorization rather than other purposes (as we already do for transient markers).
Comment 20 Tod Creasey CLA 2005-12-13 11:26:19 EST
This is what the other products interested in this are doing - they have many marker types and they will just categorize them.

If JDT created a marker type per category this solution could work for them too.
Comment 21 Markus Keller CLA 2006-01-11 06:18:19 EST
Philippe, Martin: Do you think John's suggestion of using the marker type as categorization would be feasible for JDT? If yes, then this would make the whole subCategories support obsolete.
Comment 22 Philipe Mulet CLA 2006-01-11 07:23:17 EST
Would different marker types affect user experience ? i.e. I suspect it would become visible in filter configuration dialog ?

I installed FindBugs (static analysis), and it only contributes one marker types. Not one per category.

This being said, producing new marker types is something we could consider, though I suppose it could be seen as a breaking change for clients watching our generated markers. They suddenly would look different.
Comment 23 Tod Creasey CLA 2006-01-11 08:02:40 EST
So far of everyone I talked to no one else does it like JDT - they all use a lot of markerTypes which of course has it's own problems (and why you can group them now). 

I changed the support last week so that an undefined category defaults to marker type - anything finer than that is done via attributes now as no one wanted the plug-in activation of the category provider.
Comment 24 Martin Aeschlimann CLA 2006-01-11 09:59:14 EST
In the last discussion with Tod berfore X-Mas I tought we agreed to that there is a confusion about the relationship between between category / subcategory. Let's quikly choose better extension point names to clarify:

1. 'Problem view category' (currently called subcategory):
A (contributable) category that defines groups and knows how to sort markers in these groups.

2. 'Marker type group': (currently called category):
One instance of a 'Problem view category', contributed by default by platform.ui. It groups by similar marker types together. Which this extension point each marker type can specify in which marker type group is wants to belong. 

So yes, I think we still need the 'Marker type group' extension point. Inheritance of marker types can help, but a.) should not be enforced, b.) we still need to know what kind of groups there are. Not each marker type should be its own group (Better introduce a 'Undefined' group for markers types that are not using the 'Marker type group' extension point yet. Marker types are an implemetation detail, so don't show it in the UI)
Comment 25 Tod Creasey CLA 2006-01-11 10:20:53 EST
If you look at todays integration build there is no longer a concept of subcategory. Now there are three ways to specify a category

1) By grouping marker types into a category
2) By specifying an attribute to check and a mapping of the value to a displayable string
3) By markerType (the default). You get this if you do not specify either of the other two

I won't ever show "Undefined" or "Unknown" as this is too confusing to the user.
Comment 26 Markus Keller CLA 2006-01-13 12:12:00 EST
Tod, I tried I20060110-1026 with the contribution below, but I did not get the markerTypeCategory nor the markerAttributeCategory to work. Did I do something wrong, or is the Problems view not yet ready for this?

   <extension
         point="org.eclipse.ui.ide.markerSupport">
      <markerTypeCategory name="My Java Problem">
         <markerTypeReference id="org.eclipse.jdt.core.problem"/>
      </markerTypeCategory>
      <markerAttributeCategory attribute="id">
         <markerTypeReference id="org.eclipse.jdt.core.problem"/>
         <markerAttributeMapping
               name="603979894"
               value="Unused member category"/>
      </markerAttributeCategory>
   </extension>
Comment 27 Tod Creasey CLA 2006-01-13 13:30:08 EST
Hang fire on this Markus - I am in the process of some fixes/changes Martin and I discussed
Comment 28 John Arthorne CLA 2006-01-13 13:38:51 EST
If you're wondering, like I was, what "hang fire" means:
http://www.answers.com/topic/hang-fire
Comment 29 Tod Creasey CLA 2006-01-25 14:51:56 EST
Subcategories are now gone.