Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 294812

Summary: [hover] Improper hover order prevents using the right hover
Product: [Tools] CDT Reporter: Marc Khouzam <marc.khouzam>
Component: cdt-editorAssignee: Marc Khouzam <marc.khouzam>
Status: RESOLVED FIXED QA Contact: Anton Leherbauer <aleherb+eclipse>
Severity: normal    
Priority: P3 Flags: aleherb+eclipse: review+
Version: 6.0   
Target Milestone: ---   
Hardware: PC   
OS: Linux   
Whiteboard:
Attachments:
Description Flags
Proposed fix
marc.khouzam: iplog-
Small tweak to previous fix
marc.khouzam: iplog-
Alternative fix
aleherb+eclipse: iplog-
Small tweak to alternative fix marc.khouzam: iplog-

Description Marc Khouzam CLA 2009-11-10 16:10:07 EST 
The way that BestMatchHover decides which hover to user is by going through each hover sequentially to find one that will return an output.  When multiple hovers can return an output, it is the first one found that will be used.  This implies that the order in which the hovers are queried is very important.

The bug is that if a plugin is not activated, it's hover is not added to the hover list right away, but only later, once the plugin is activated.  And when it is finally added it is put at the end of the list, losing its proper position.

To reproduce:
1- start eclipse without activating the cdt.debug.ui plugin.  To do this, start in the C/C++ perspective, without any debugging views open.
2- select a CEditor and hover over a variable.  The Source hover will trigger.
3- launch a debug session (CDI or DSF)
4- again, hover over a variable in the editor.  You will again see the Source hover be triggered instead of the Debug hover.

This is because on the step 2, the debug plugin was not active so did not create the debug hover.  On step 4, the debug hover is finally created, but it is added to the end of the hover list, after the Source hover.

I will attach a patch.

Note that this bug was taken straight out of the JDT.  I will file a bug there too.
Comment 1 Marc Khouzam CLA 2009-11-10 16:13:55 EST 
Created attachment 151880 [details]
Proposed fix

This proposed patch preserves the order of the hovers in BestMatchHover.  I preserved the API (although the class in internal), and I tried to minimize the performance impact (although the lists and arrays are of 6 elements for bare CDT).

I will wait until tomorrow to commit, to wait for any comments.

Toni can you review when you have time?
Comment 2 Marc Khouzam CLA 2009-11-10 21:21:22 EST 
Created attachment 151904 [details]
Small tweak to previous fix

I had the size of the array in the patch one element too big.
Comment 3 Anton Leherbauer CLA 2009-11-11 05:43:26 EST 
Good catch!
I would like to address this a little different for 2 reasons:
- I don't like the fNullEntryCount
- The semantics of addTextHover() should be preserved
Patch follows.
Comment 4 Anton Leherbauer CLA 2009-11-11 05:46:44 EST 
Created attachment 151932 [details]
Alternative fix
Comment 5 Marc Khouzam CLA 2009-11-11 08:38:08 EST 
(In reply to comment #4)
> Created an attachment (id=151932) [details]
> Alternative fix

I agree that this is easier to understand.  But there are two reasons why I didn't choose a similar solution:

1- fTextHoverSpecification is no longer shrunk as elements are removed from it.  This impacts performance.  I realize we only have 7 elements, so it is not a big deal, however, since the elements are contributed by extension point, we could have many more.  But I didn't know if it was worth being careful about this so I played it safe, but I'm ok with your solution for this case.

2- More important, the addTextHover() overridable method is no longer called.  I figured that we should keep using this method to be consistent with the API.  And this is where things got ugly and I had to use the fNullEntryCount, to avoid changing the signature of addTextHover().  Now, this class is internal, so we could change it, but I didn't want to make that decision :-)

If you are ok with both points above, then I'm ok with it too.  I would remove the addTextHover() method in this case.  Just let me know and I can commit it if you'd like.
Comment 6 Marc Khouzam CLA 2009-11-11 09:05:21 EST 
FYI, I've opened Bug 294833 on JDT for this similar issue.  The two bugs are independent because they use different copies of the code.
Comment 7 Anton Leherbauer CLA 2009-11-11 09:12:33 EST 
(In reply to comment #5)
> I agree that this is easier to understand.  But there are two reasons why I
> didn't choose a similar solution:
> 
> 1- fTextHoverSpecification is no longer shrunk as elements are removed from it.

The performance impact should be neglectable.

> 2- More important, the addTextHover() overridable method is no longer called. 
> I figured that we should keep using this method to be consistent with the API. 
> And this is where things got ugly and I had to use the fNullEntryCount, to
> avoid changing the signature of addTextHover().  Now, this class is internal,
> so we could change it, but I didn't want to make that decision :-)

From my POV, addTextHover() is protected to allow subclasses to add additional hovers.  But as it is an internal class and there is no subclass within CDT this is actually a non-issue anyway :).

> If you are ok with both points above, then I'm ok with it too.  I would remove
> the addTextHover() method in this case.  Just let me know and I can commit it
> if you'd like.

Yes, that's OK.

(In reply to comment #6)
> FYI, I've opened Bug 294833 on JDT for this similar issue.  The two bugs are
> independent because they use different copies of the code.

I am curious how the final fix will look like in JDT.
Comment 8 Marc Khouzam CLA 2009-11-11 09:26:21 EST 
(In reply to comment #7)
> > If you are ok with both points above, then I'm ok with it too.  I would remove
> > the addTextHover() method in this case.  Just let me know and I can commit it
> > if you'd like.
> 
> Yes, that's OK.
> 
> (In reply to comment #6)
> > FYI, I've opened Bug 294833 on JDT for this similar issue.  The two bugs are
> > independent because they use different copies of the code.
> 
> I am curious how the final fix will look like in JDT.

I will commit the CDT fix now as the JDT one may take some time to be reviewed.  If they come up with a nicer solution, we can change ours.
Comment 9 Marc Khouzam CLA 2009-11-11 09:30:37 EST 
Created attachment 151941 [details]
Small tweak to alternative fix

This is the patch I committed. 
As I was about to commit Toni's patch, it bothered me that we were using list with default size of 2 when we know that we have more hovers.  In fact, we know how many the list will contain, so I made that change.
Comment 10 Marc Khouzam CLA 2009-11-11 09:31:17 EST 
Fixed
Comment 11 Anton Leherbauer CLA 2009-11-12 09:25:21 EST 
JDT's solution is very similar.