Bug 559816

Comment 1 Mickael Istria 2020-02-04 03:35:22 EST

isPaintable shouldn't call hasQuickFix.
hasQuickFix is only necessary to compute whether the annotation has some quick-fix, which is only necessary when drawing the annotation (to hook click handler), or that could even be delayed to the click handler so it's only checked when code mining is clicked on.

Comment 2 Andrey Loskutov 2020-02-04 03:37:13 EST

See also bug 553044 comment 2 where AnnotationCodeMiningFilter.isPaintable call participates in the deadlock.

Comment 3 Matthias Becker 2020-02-04 04:03:47 EST

(In reply to Mickael Istria from comment #1)
> isPaintable shouldn't call hasQuickFix.

But it does. See:

AnnotationCodeMiningFilter.java#isPaintable(Annotation a) {
  return annotationAccess.isPaintable(a); //<<<<
}

annotationAccess.isPaintable() is:

public boolean DefaultMarkerAnnotationAccess#isPaintable(Annotation annotation) {
	if (annotation instanceof IAnnotationPresentation)
		return true;
	AnnotationPreference preference= getAnnotationPreference(annotation);
	if (preference == null)
		return false;

	Object type= getType(annotation);
	String annotationType= (type == null ? null : type.toString());
	Image image= getImage(annotation, preference, annotationType); //<<<<
	return image != null;
}

getImage() is:

private Image getImage(Annotation annotation, AnnotationPreference preference, String annotationType) {
	...
	if (hasQuickFix(annotation)) { //<<<<
	...
}

and finally hasQuickFix is:

protected boolean hasQuickFix(Annotation annotation) {
	if (annotation instanceof IQuickFixableAnnotation) {
		IQuickFixableAnnotation quickFixableAnnotation= (IQuickFixableAnnotation)annotation;
		if (!quickFixableAnnotation.isQuickFixableStateSet())
			quickFixableAnnotation.setQuickFixable(fQuickAssistAssistant != null && fQuickAssistAssistant.canFix(annotation)); //<<<<
		return quickFixableAnnotation.isQuickFixable();
	}
	return false;
}

So you see the the code is in DefaultMarkerAnnotationAccess which is not specific for the AnnotationCodeMiningFilter.

Note: I marked interesting lines with "//<<<<"

Comment 4 Mickael Istria 2020-02-04 04:54:43 EST

(In reply to Matthias Becker from comment #3)
> (In reply to Mickael Istria from comment #1)
> > isPaintable shouldn't call hasQuickFix.
> 
> But it does. See:

I trust you it does. My comment was more to interpret "isPaintable must be implement in a way so it doesn't hasQuickFix"

Comment 5 Matthias Becker 2020-02-04 04:57:43 EST

(In reply to Mickael Istria from comment #4)
> (In reply to Matthias Becker from comment #3)
> > (In reply to Mickael Istria from comment #1)
> > > isPaintable shouldn't call hasQuickFix.
> > 
> > But it does. See:
> 
> I trust you it does. My comment was more to interpret "isPaintable must be
> implement in a way so it doesn't hasQuickFix"

So you propose to change the implementation in DefaultMarkerAnnotationAccess?

Comment 6 Mickael Istria 2020-02-04 05:47:13 EST

(In reply to Matthias Becker from comment #5)
> So you propose to change the implementation in DefaultMarkerAnnotationAccess?

As long as it's backward compatible and seems worth it, yes.

Comment 7 Matthias Becker 2020-02-04 06:19:30 EST

(In reply to Mickael Istria from comment #6)
> (In reply to Matthias Becker from comment #5)
> > So you propose to change the implementation in DefaultMarkerAnnotationAccess?
> 
> As long as it's backward compatible and seems worth it, yes.

As this class is not specific to the annotations I can't overlook the consequences...