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

Bug 326418

Summary: Ctrl+Click navigation is broken
Product: [Tools] CDT Reporter: Missing name <uwe_kindler>
Component: cdt-editorAssignee: Sergey Prigogin <eclipse.sprigogin>
Status: RESOLVED FIXED QA Contact: Anton Leherbauer <aleherb+eclipse>
Severity: major    
Priority: P3 CC: eclipse.sprigogin, joshkel, malaperle, paedu.hofer, sducharme, yevshif
Version: 7.0.1   
Target Milestone: 7.0.2   
Hardware: PC   
OS: Windows XP   
Whiteboard:
Attachments:
Description Flags
Build AST in another thread when not available
eclipse.sprigogin: iplog-
Detect hyperlinks without AST when AST is not available
eclipse.sprigogin: iplog-
Added a test for hyperlink detection without AST
eclipse.sprigogin: iplog-
Fixed an issue with treating "end" as a language keyword. eclipse.sprigogin: iplog-

Description Missing name CLA 2010-09-28 10:07:11 EDT 
Build Identifier: 201009241320

Navigating through the code via Ctrl+LeftMouseButtonClick is not working anymore since upgrade to CDT 7.0.1. Navigation via F3 still works fine.

That means most of the time I move the mouse over an item and press Ctrl the item is not underlined to indicate that I can click it. Sometimes it is possible that an item is underlined properly and sometimes it might happen, that an item becomes underlined after some seconds. 

Because navigation via F3 is working properly it is not an indexer issue of my project. The problem occures in all my elicpse projects. 

So I think Ctrl+Click navigation is broken in some way.



Reproducible: Sometimes

Steps to Reproduce:
1. Press Ctrl and move mouse over an include file
2. If include include file is underlined and you can click it, then click it
3. Repeat step 1.
Comment 1 Anton Leherbauer CLA 2010-09-29 07:11:30 EDT 
Probably due to bug 324232.
Comment 2 Marc-André Laperle CLA 2010-09-29 10:20:02 EDT 
The UI blocking was less noticeable to me. Maybe the fix in bug 324232 should be rolled back and the bug reopened?
Comment 3 Sergey Prigogin CLA 2010-09-29 11:20:54 EDT 
The blocking behavior due to bug 324232 was a more serious problem than the delay in showing hyperlinks. I'm open to ideas how to solve the problem in a less intrusive way, but against rolling back the fix for bug 324232 until we have a better solution.
Comment 4 Josh Kelley CLA 2010-09-30 10:41:26 EDT 
I've noticed what appears to be the same problem since upgrading from CDT 6.x to 7.0.1:

 * Ctrl-Click seems to always not work when a source file is first opened in the editor or when switching tabs from one already opened file to another.
 * F3 works just fine.
 * If I mouse over an identifier and wait for the hover tip showing where that identifier is defined to appear, then holding down Ctrl properly hyperlinks identifiers, and Ctrl-Click works fine until the next time I open another file or switch tabs to an already opened file again.
Comment 5 Yevgeny Shifrin CLA 2010-09-30 22:02:25 EDT 
Hi,

I would like to emphasize the severity of this bug. In my opinion, this is very basic functionality used by many developers for code navigation. I think it should be solved as quickly as possible and maybe some patch could be added to already released 7.0.1 (not sure if possible). Unfortunately our development team cannot adopt 7.0.1 CDT as long as this feature is broken.

Thanks,
Yevgeny
Comment 6 Sergey Prigogin CLA 2010-10-01 17:08:01 EDT 
(In reply to comment #5)
This issue presents a tough dilemma. In order for Ctrl+Click to work, CElementHyperlinkDetector has to determine if there is navigable symbol at the current cursor location. In order to do that CElementHyperlinkDetector needs an AST. AST takes considerable time to build. The behavior before 7.0.1 was to wait for the AST to be built whenever Ctrl key is pressed. This behavior was problematic since the Ctrl key doesn't uniquely identify user intent and is used as part of various hot keys, most often with Ctrl+C and Ctrl+V. Waiting for the AST in CElementHyperlinkDetector resulted in freezing the UI when user was trying to copy and paste or was doing other actions involving Ctrl key.

Based on the principle that freezing the UI is the worst sin, the behavior was changed in 7.0.1. CElementHyperlinkDetector checks for availability of the AST, but doesn't wait for it to be built if it's not yet available. As a result, there is a time gap between opening an editor and before the hyperlinks can be shown. This time gap existed before 7.0.1, but the UI would simply freeze for the duration of the gap and the hyperlink would be eventually shown.

I can think of the following imperfect solutions:

1. Introduce a preference allowing user to choose between the wait and no-wait behaviors. Doesn't really solve the problem, but allows user to make her own choice between the two evils.
2. Change CElementHyperlinkDetector to always display a link when the AST is not ready yet. Once user clicks on the link, it is checked using the AST. This may result in showing links in weird places, for example on C/C++ reserved words. False links won't be clickable.
2a. In addition to 2 we can do some shallow parsing to eliminate some of the false links.

If you have other ideas how to solve this problem, please express them here. Ideas accompanied by patches are even more welcome.
Comment 7 Missing name CLA 2010-10-02 03:52:04 EDT 
> but doesn't wait for it to be built if it's not yet available. As a result,
> there is a time gap between opening an editor and before the hyperlinks can be
> shown. This time gap existed before 7.0.1, but the UI would simply freeze for
> the duration of the gap and the hyperlink would be eventually shown.

In 7.0.1 it may last 10 or event more than 10 seconds for us before the links are clickable while F3 navigation works immediatelly. In CDT 7.0 I never noticed UI freezing when clickng Ctrl key. If I navigate through the code I expect a very fast navigation experience because that is an essential thing when writing C++ code. I may have to search through a number of header files (i.e. when working with Qt), declaration and definitions. This happens always and any time when writing code. So reading through existing code and navigating through existing code often takes more time than writing code. So in my opinion this is a very essential and basic feature for working with an IDE that is used all the time by the user. Therefore the speed of this feature needs to be very high. 

So working with CDT 7.0 was much smoother for us and we did not notice (or remember) any UI freezing while clicking Ctrl key in 7.0 - so it must have happened very seldom on.
Comment 8 Marc-André Laperle CLA 2010-10-02 10:03:41 EDT 
(In reply to comment #6)
> As a result,
> there is a time gap between opening an editor and before the hyperlinks can be
> shown. This time gap existed before 7.0.1, but the UI would simply freeze for
> the duration of the gap and the hyperlink would be eventually shown.

What I'm experiencing is that now the "time gap" has to be triggered by something first. Like Josh wrote, a hover on a identifier can trigger it or clicking somewhere (triggering mark occurrences) or opening call hierarchy and probably anything that asks for the AST. But if you don't do any of that, the hyperlinks will never show up.

(In reply to comment #6)
> If you have other ideas how to solve this problem, please express them here.
> Ideas accompanied by patches are even more welcome.

Is there a way this could be done asynchronously?
Comment 9 Sergey Prigogin CLA 2010-10-02 11:40:48 EDT 
(In reply to comment #8)
> What I'm experiencing is that now the "time gap" has to be triggered by
> something first. Like Josh wrote, a hover on a identifier can trigger it or
> clicking somewhere (triggering mark occurrences) or opening call hierarchy and
> probably anything that asks for the AST. But if you don't do any of that, the
> hyperlinks will never show up.

Interesting observation. We should probably trigger building of AST as soon as editor receives focus not relying on the reconciler.
> 
> (In reply to comment #6)
> > If you have other ideas how to solve this problem, please express them here.
> > Ideas accompanied by patches are even more welcome.
> 
> Is there a way this could be done asynchronously?

IHyperlinkDetector.detectHyperlinks method is synchronous and runs on the UI thread. This is defined by the Platform.
Comment 10 Marc-André Laperle CLA 2010-10-02 12:54:33 EDT 
Created attachment 180109 [details]
Build AST in another thread when not available

This patch will build the AST in another thread if it's not available yet. The problem with this approach is that the user still needs to do a mouse move to get the hyperlink after the job is done.

(In reply to comment #9)
> (In reply to comment #8)
> > What I'm experiencing is that now the "time gap" has to be triggered by
> > something first. Like Josh wrote, a hover on a identifier can trigger it or
> > clicking somewhere (triggering mark occurrences) or opening call hierarchy and
> > probably anything that asks for the AST. But if you don't do any of that, the
> > hyperlinks will never show up.
> 
> Interesting observation. We should probably trigger building of AST as soon as
> editor receives focus not relying on the reconciler.

I think that would make sense. It would reduce the initial delay too.

> > (In reply to comment #6)
> IHyperlinkDetector.detectHyperlinks method is synchronous and runs on the UI
> thread. This is defined by the Platform.

Yes, I guess I was asking, would it be possible to make it asynchronous even if it requires changing the Platform. But maybe that's overkill.
Comment 11 Sergey Prigogin CLA 2010-10-04 00:44:03 EDT 
Created attachment 180125 [details]
Detect hyperlinks without AST when AST is not available

I've realized that hyperlinks can be detected with reasonably good fidelity even without an AST. The attached patch falls back on non-AST algorithm when the AST is not ready yet. I'm going to submit this to HEAD now and to 7.0.x once people get chance to play with it.
Comment 12 Sergey Prigogin CLA 2010-10-04 01:15:39 EDT 
Created attachment 180127 [details]
Added a test for hyperlink detection without AST
Comment 14 Marc-André Laperle CLA 2010-10-04 12:42:10 EDT 
(In reply to comment #11)
> the AST is not ready yet. I'm going to submit this to HEAD now and to 7.0.x
> once people get chance to play with it.

I've tried the patch, it works reasonable well.

I noticed a couple of minor problems:
- overloaded operators
- vector::end(), map::end()
Comment 15 Sergey Prigogin CLA 2010-10-05 02:19:23 EDT 
Created attachment 180224 [details]
Fixed an issue with treating "end" as a language keyword.
Comment 16 CDT Genie CLA 2010-10-05 03:23:01 EDT 
*** cdt cvs genie on behalf of sprigogin ***
Bug 326418 - fixed false positives in language keyword check.

[*] CElementHyperlinkDetector.java 1.21 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/editor/CElementHyperlinkDetector.java?root=Tools_Project&r1=1.20&r2=1.21
Comment 18 Sergey Prigogin CLA 2010-10-06 03:44:36 EDT 
Fixed in HEAD and cdt_7_0 > 20101006.
Comment 19 Yevgeny Shifrin CLA 2010-10-06 04:08:13 EDT 
Hi,

In comment 14, issue with overloaded operators was mentioned. Was this issue fixed as well? Thank you for handling this quickly. Is it possible to provide some kind of patch for 7.0.1 CDT?

Thanks,
Yevgeny
Comment 20 Sergey Prigogin CLA 2010-10-06 12:56:29 EDT 
(In reply to comment #19)
> Hi,
> 
> In comment 14, issue with overloaded operators was mentioned. Was this issue
> fixed as well?

No, this is a limitation of the no-AST approach. You can create a separate bug for it if you like.

> Is it possible to provide some kind of patch for 7.0.1 CDT?

You should be able to get it from the next 7.0.2 build when it becomes available.
Comment 21 Marc-André Laperle CLA 2010-10-06 13:03:34 EDT 
(In reply to comment #20)
> (In reply to comment #19)
> > Hi,
> > 
> > In comment 14, issue with overloaded operators was mentioned. Was this issue
> > fixed as well?
> 
> No, this is a limitation of the no-AST approach. You can create a separate bug
> for it if you like.

I think it should request the AST so that the AST approach becomes available at some point. Also, other hyperlinks contributed might need the AST, for example bug 319480.
Comment 22 Sergey Prigogin CLA 2010-10-06 13:13:03 EDT 
(In reply to comment #21)
> I think it should request the AST so that the AST approach becomes available at
> some point. Also, other hyperlinks contributed might need the AST, for example
> bug 319480.

I agree that AST should start building as soon as an editor becomes active. We shouldn't wait for Ctrl to be pressed to trigger AST build.
Comment 23 Yevgeny Shifrin CLA 2010-10-07 04:11:45 EDT 
(In reply to comment #20)
> (In reply to comment #19)
> > Hi,
> > 
> > In comment 14, issue with overloaded operators was mentioned. Was this issue
> > fixed as well?
> No, this is a limitation of the no-AST approach. You can create a separate bug
> for it if you like.
> > Is it possible to provide some kind of patch for 7.0.1 CDT?
> You should be able to get it from the next 7.0.2 build when it becomes
> available.

Hi,

This bug was reported for regression in Ctrl+LeftMouseButtonClick functionality in 7.0.1 in compare to 7.0.0. Solution was provided, which does not fully solve this issue (overloaded operators). I think this bug should be closed once the functionality is working as in 7.0.0. If you insist I will open a new bug, but I don't understand why is it needed?

Thanks,
Yevgeny
Comment 24 Marc-André Laperle CLA 2011-02-23 13:16:27 EST 
Created bug 338004 for overloaded operators.