Bug 173458 - add camel-case based auto-completion
Summary: add camel-case based auto-completion
Status: RESOLVED FIXED
Alias: None
Product: CDT
Classification: Tools
Component: cdt-core (show other bugs)
Version: 4.0   Edit
Hardware: All All
: P3 enhancement with 2 votes (vote)
Target Milestone: 8.0   Edit
Assignee: Markus Schorn CLA
QA Contact:
URL:
Whiteboard:
Keywords: contributed
: 223625 (view as bug list)
Depends on:
Blocks:
 
Reported: 2007-02-08 10:17 EST by Andrew Ferguson CLA
Modified: 2014-01-29 17:00 EST (History)
12 users (show)

See Also:


Attachments
SegmentMatcher (12.41 KB, patch)
2010-08-11 10:51 EDT, Tomasz Wesolowski CLA
mschorn.eclipse: iplog+
Details | Diff
SegmentMatcher integration (41.44 KB, patch)
2010-12-14 15:35 EST, Jens Elmenthaler CLA
no flags Details | Diff
New version of SegmentMatcher integration (46.98 KB, patch)
2011-01-19 04:32 EST, Jens Elmenthaler CLA
no flags Details | Diff
New version of Integration (67.78 KB, patch)
2011-01-21 11:25 EST, Jens Elmenthaler CLA
no flags Details | Diff
Implementation Completed (98.42 KB, patch)
2011-02-14 12:04 EST, Jens Elmenthaler CLA
no flags Details | Diff
Fix from review (101.56 KB, patch)
2011-02-16 12:19 EST, Jens Elmenthaler CLA
no flags Details | Diff
@noimplement, @noextend to public API (101.85 KB, patch)
2011-02-23 07:27 EST, Jens Elmenthaler CLA
mschorn.eclipse: iplog+
Details | Diff
The patch I described in my comments. (1.27 KB, text/plain)
2011-04-26 16:08 EDT, John Liu CLA
no flags Details
Patch option 2 mentioned in my comments (10.62 KB, text/plain)
2011-04-27 11:50 EDT, John Liu CLA
no flags Details
patch applied to cdt.ui, cdt.core, cdt.ui.tests, cdt.core.tests (25.75 KB, text/plain)
2011-04-28 10:44 EDT, John Liu CLA
no flags Details
Patch for John (19.07 KB, patch)
2011-04-29 03:10 EDT, Jens Elmenthaler CLA
no flags Details | Diff
Patch for John (43.24 KB, patch)
2011-04-29 09:29 EDT, Jens Elmenthaler CLA
cdtdoug: iplog+
Details | Diff
Patch for John (2) (54.79 KB, patch)
2011-05-01 05:40 EDT, Jens Elmenthaler CLA
cdtdoug: iplog+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Ferguson CLA 2007-02-08 10:17:14 EST
hi,

I couldn't find an existing entry for this, and internal users have been asking about it. This is where bindings are looked up based on camel case, i.e. if you have

class OrangePearApple {};

and type and complete on OPA, you will see OrangePearApple as a suggestion

thanks,
Andrew
Comment 1 Andrew Ferguson CLA 2008-03-25 06:46:21 EDT
*** Bug 223625 has been marked as a duplicate of this bug. ***
Comment 2 Mike Kucera CLA 2008-04-04 14:47:14 EDT
I'm not a C++ programmer, but my understanding is that camel case isn't a standard naming convention in C++ like it is in Java. Identifiers using underscores as word separators are probably just as common. This feature should probably support both the camel case and underscores conventions to be really useful.
Comment 3 Tomasz Wesolowski CLA 2010-05-17 09:54:25 EDT
I have added this feature to the list of possible things to improve as a part of my GSoC project. Please give on the wiki some feedback about how you'd like to see it implemented (as the case sensitivity can be approached in several ways):

http://wiki.eclipse.org/CDT/C_editor_enhancements/Camel-case_completion
Comment 4 Tomasz Wesolowski CLA 2010-08-06 18:35:16 EDT
OK, let's do it!

I've been working on a class which would handle prefix / camel case matching in a way similar to JDT, but respecting various conventions (CamelCase or any_underscore_case). It's mostly done, but could use some more polishing and testing. I expect to have it ready soon.

The question arises, where to bind it to have the new behaviour respected well by Context Assist? A suggestion is to have this routine called instead where currently this one is called (with 'true' as the last argument = prefixLookup):
org.eclipse.cdt.core.parser.util.CharArrayUtils.equals(char[], int, int, char[], boolean)
...which, according to the call hierarchy, mostly resides in various implementations of IScope.getBindings(..). 

Will it be sufficient?
Comment 5 Tomasz Wesolowski CLA 2010-08-11 10:51:02 EDT
Created attachment 176358 [details]
SegmentMatcher

Here we go - the class and test case.

Quoting Jens Elmenthaler from the proposal on the wiki:

> For camelCase matching, we should use the same rules as JDT.
> That means someSymbol would not match ss. The question whether
> an underscore_case matches a given string or not is not so simple.
> My proposal would be to treat _ or an uppercase letter in the typed string
> as the beginning of a new word, and then match the canndidates
> accordingly. I.e. typing sS would result in someSymbol and some_symbol.
> s_s would result in some_symbol only.

I followed this convention. See the attached test case for examples (and feel free to expand it if you believe the behaviour should be different).
I used a different algorithm than JDT in order to support different naming conventions. If there's a need, I can describe it somewhere.

--

The important question arises: What to do with it? I could experiment, but it would be quite helpful for someone more familiar with CDT's Context Assist to drop a suggestion here on how to integrate it the best way.
Comment 6 Jens Elmenthaler CLA 2010-12-14 15:35:43 EST
Created attachment 185170 [details]
SegmentMatcher integration

Thanks Tomasz for providing the SegmentMatcher, especially with all the underscore-specific features.

I created this patch to play with the integration. The basic approach to avoid huge API changes is to reinterprete parameters named prefixLookup or similar as segment match lookup. It doesn't look too wrong. I only found NamedNodeCollector and its subclass not fitting this assumption and should have handled it appropriately.

I added a couple of junits that I could figure out quickly. I would add the missing ones (and the remaining cleanup) once a potential committer took a look.

I have been using it in my workbench for 2 weeks now and actually don't want to miss again:-) Furthermore, the CDT core and UI tests still pass.
Comment 7 Marc-André Laperle CLA 2011-01-15 12:57:21 EST
Hi Jens. I'm very excited about this feature! I tried the patch and found an issue

BaseClass.h:

#ifndef BASECLASS_H_
#define BASECLASS_H_

class BaseClass {};

#endif

main.cpp:

#include "BaseClass.h"

int main() {
	BC[Ctrl+space here]
	return 0;
}

Is this case working for you?
Comment 8 Jens Elmenthaler CLA 2011-01-17 05:13:05 EST
(In reply to comment #7)
> Hi Jens. I'm very excited about this feature! I tried the patch and found an
> issue
> BaseClass.h:
> #ifndef BASECLASS_H_
> #define BASECLASS_H_
> class BaseClass {};
> #endif
> main.cpp:
> #include "BaseClass.h"
> int main() {
>     BC[Ctrl+space here]
>     return 0;
> }
> Is this case working for you?
No, doesn't work for me either. I'll take a look at it. Thanks for reporting.
Comment 9 Markus Schorn CLA 2011-01-18 07:02:04 EST
I guess the approach has to fail wherever we use (sort of) a binary search in the index.
Comment 10 Jens Elmenthaler CLA 2011-01-18 07:15:22 EST
(In reply to comment #9)
> I guess the approach has to fail wherever we use (sort of) a binary search in
> the index.
I hope not. I stumbled over this in the NamedNodeCollector. I do the matching in 2 stages. In the first, I do the binary search based on the first character of the given prefix only (if there is one). This yields a superset of all matches by pure prefix matching, all the segment matches, plus some that don't match at all. The latter are removed an the second stage (see NamedNodeCollector#addNode).

I would use this approach whereever such a binary search is to be done, by now however I found only the NamedNodeCollector.
Comment 11 Markus Schorn CLA 2011-01-18 08:17:45 EST
(In reply to comment #10)
> (In reply to comment #9)
> > I guess the approach has to fail wherever we use (sort of) a binary search in
> > the index.
> I hope not. I stumbled over this in the NamedNodeCollector. I do the matching
> in 2 stages. In the first, I do the binary search based on the first character
> of the given prefix only (if there is one). This yields a superset of all
> matches by pure prefix matching, all the segment matches, plus some that don't
> match at all. The latter are removed an the second stage (see
> NamedNodeCollector#addNode).
> I would use this approach whereever such a binary search is to be done, by now
> however I found only the NamedNodeCollector.

I probably have not looked close enough. While this idea is not good for the performance of content-assist, it may work. Sorry for the misleading comment.
Comment 12 Jens Elmenthaler CLA 2011-01-19 04:32:59 EST
Created attachment 187086 [details]
New version of SegmentMatcher integration

- Fixes the problems reported by Marc-Andre
- Fixes the problem that the popup is closed after typing more characters

This patch is not exactly against the latest HEAD, as I have not upgraded to 3.7 for the platform.
Comment 13 Jens Elmenthaler CLA 2011-01-19 04:38:22 EST
(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
...
> I probably have not looked close enough. While this idea is not good for the
> performance of content-assist, it may work. Sorry for the misleading comment.
Aggree with that, but by now the proposal compution time seems to be dominated by CContentAssistInvocationContext#getCompletionNode which is used by all parsing based proposal computers.
Comment 14 Jens Elmenthaler CLA 2011-01-21 11:25:01 EST
Created attachment 187299 [details]
New version of Integration

- fixes the issue reported by Marc-Andre (this time really, last time I attached the wrong file)
- MacroContainerCollector adapted simlar to BindingCollector
- conceptual cleanup
Comment 15 Jens Elmenthaler CLA 2011-02-14 12:04:43 EST
Created attachment 188916 [details]
Implementation Completed

- added a preference to turn it on/off to be consistent with JDT.
- polished

Hope I find a committer to look at it.
Please note the Design chapter in the Wiki http://wiki.eclipse.org/CDT/C_editor_enhancements/Camel-case_completion
Comment 16 Markus Schorn CLA 2011-02-16 09:47:43 EST
I had an initial look at the patch, here are my first observations:
Comment 17 Markus Schorn CLA 2011-02-16 09:54:08 EST
I had an initial look at the patch, here are my first observations:
* The patch is not a eclipse workspace patch, makes it hard to apply it.
* PrefixMatcher.match(..) never matches anything
* SegmentMatcher puts a backslash in front of any non-alphanumeric character,
  consider using Pattern.quote(..) to get it right.
Comment 18 Jens Elmenthaler CLA 2011-02-16 10:56:39 EST
(In reply to comment #17)
> I had an initial look at the patch, here are my first observations:
> * The patch is not a eclipse workspace patch, makes it hard to apply it.
Before creating the next patch with the fixes: what does make it an eclipse workspace patch? I just used the wizard under the team menu, and in the "Advanced Options" tab is says "Workspace (Multi-project Apply Patch wizard specific)", which is checked. I cannot select anything different.
Also, the patch starts with the following lines:
### Eclipse Workspace Patch 1.0
#P org.eclipse.cdt.core
...
Comment 19 Markus Schorn CLA 2011-02-16 11:15:55 EST
(In reply to comment #18)
> Before creating the next patch with the fixes: what does make it an eclipse
> workspace patch? I just used the wizard under the team menu, and in the
> "Advanced Options" tab is says "Workspace (Multi-project Apply Patch wizard
> specific)", which is checked. I cannot select anything different.
> Also, the patch starts with the following lines:
> ### Eclipse Workspace Patch 1.0
> #P org.eclipse.cdt.core
That makes it a workspace patch, I obtained the wrong format of the patch by clicking on the link 'raw-unified' in the diff-view of the patch, which came 
up when following the link in commment 15. I apologize and will stay away from
that link.
Comment 20 Jens Elmenthaler CLA 2011-02-16 12:19:15 EST
Created attachment 189119 [details]
Fix from review

- Fixed the two issues reported sofar
- Added test cases for the ContentAssistMatcherFactory that would have caught the wrong PrefixMatcher implementation
Comment 21 Markus Schorn CLA 2011-02-23 05:06:44 EST
(In reply to comment #20)
> Created attachment 189119 [details]
> Fix from review
> - Fixed the two issues reported sofar
> - Added test cases for the ContentAssistMatcherFactory that would have caught
> the wrong PrefixMatcher implementation

Thanks for the update, I have found two more issues:

* The following behavior is inconsistent with JDT's camel case completion. Is this intended?
   class testXblaYbla {};
   void func(){
      testY       // content assist proposes testXblaYbla
   }

* The patch introduces new API. Unless there is a need to allow for that, interfaces need to be marked as @noimplement and @noextend, classes need to be final or marked as @noextend. Otherwise, we cannot make changes to these types in future releases.
Comment 22 Jens Elmenthaler CLA 2011-02-23 05:30:39 EST
(In reply to comment #21)
> (In reply to comment #20)
> > Created attachment 189119 [details] [details]
> > Fix from review
> > - Fixed the two issues reported sofar
> > - Added test cases for the ContentAssistMatcherFactory that would have caught
> > the wrong PrefixMatcher implementation
> Thanks for the update, I have found two more issues:
Thanks for taking the time.

> * The following behavior is inconsistent with JDT's camel case completion. Is
> this intended?
>    class testXblaYbla {};
>    void func(){
>       testY       // content assist proposes testXblaYbla
>    }
This is intended. The JDT has 3 short comings:
1) IAN does not match I_AST_NAME, negotiable for Java, a must for C/C++
2) IAN does not match IASTName, annoys me frequently
3) IAN does not match IName, annoys me frequently because I have such a bad memory for all the segments in the name.

2) and 3) are my personal view to a certain extend. The interesting point is, I don't know how to get 1), without also getting 2) and 3).
 
> * The patch introduces new API. Unless there is a need to allow for that,
> interfaces need to be marked as @noimplement and @noextend, classes need to be
> final or marked as @noextend. Otherwise, we cannot make changes to these types
> in future releases.
I'll create a new patch fixing that.
Comment 23 Markus Schorn CLA 2011-02-23 06:47:25 EST
(In reply to comment #22)
> This is intended. The JDT has 3 short comings:
> 1) IAN does not match I_AST_NAME, negotiable for Java, a must for C/C++
> 2) IAN does not match IASTName, annoys me frequently
> 3) IAN does not match IName, annoys me frequently because I have such a bad
> memory for all the segments in the name.

I understand 1) and 2). 3) should defenitly not match, and with the current implementation it does not, does it?
There is also my example:
  4) IAN should match IASTName but not IASTImplicitName

I think we can have that with the following rules:
- A segment is an upper-case letter followed by a sequence of lowercase letters.
- The segment 'Aseg' matches:
    (
     [a-z]*A             // sequence of lower-case letters followed by 'A'
     | [A-Z]*A(?![A-Z])  // sequence of upper-case letters followed by 'A' 
                         // not followed by a upper-case letter
     | [a-zA-Z]*_[aA]    // sequence of letters followed by '_a' or '_A'  
    ) [sS][eE][gG]       // case-insensitive match for seg.

If there is a reason for handling unicode characters, \p{Lu}, [\p{L}&&[^\p{Lu}]] and \p{L} can be used instead of [A-Z], [a-z] and [a-zA-Z].
Comment 24 Jens Elmenthaler CLA 2011-02-23 07:08:41 EST
(In reply to comment #23)
> (In reply to comment #22)
> > This is intended. The JDT has 3 short comings:
> > 1) IAN does not match I_AST_NAME, negotiable for Java, a must for C/C++
> > 2) IAN does not match IASTName, annoys me frequently
> > 3) IAN does not match IName, annoys me frequently because I have such a bad
> > memory for all the segments in the name.
I got 3) wrong. Should read:
3) IN does not match IASTName, annoys me frequently because I have such a bad memory for all the segments in the name.

Shall mean, I often remember it's an interface, and it had "Name" in it, but I don't rember some subtle segments in between, nor their order. The new feature here is that I can skip specifying segments and just focus on the relevant.
Comment 25 Jens Elmenthaler CLA 2011-02-23 07:11:48 EST
(In reply to comment #23)
> (In reply to comment #22)
>   4) IAN should match IASTName but not IASTImplicitName
To be clear in this point, I really propose that IAN should match IASTImplicitName, too.
Comment 26 Jens Elmenthaler CLA 2011-02-23 07:27:07 EST
Created attachment 189591 [details]
@noimplement, @noextend to public API
Comment 27 Markus Schorn CLA 2011-02-23 08:49:59 EST
(In reply to comment #25)
> (In reply to comment #23)
> > (In reply to comment #22)
> >   4) IAN should match IASTName but not IASTImplicitName
> To be clear in this point, I really propose that IAN should match
> IASTImplicitName, too.

If it is on purpose, I buy it. I want to check for another one:
(5) Should ISN really match IASTName?
Comment 28 Jens Elmenthaler CLA 2011-02-23 09:17:53 EST
(In reply to comment #27)
> (In reply to comment #25)
> > (In reply to comment #23)
> > > (In reply to comment #22)
> > >   4) IAN should match IASTName but not IASTImplicitName
> > To be clear in this point, I really propose that IAN should match
> > IASTImplicitName, too.
> If it is on purpose, I buy it. I want to check for another one:
> (5) Should ISN really match IASTName?
Yes and no;-)
No, because being around in the CDT I know that IASTName means I AST Name.
Yes, because in general it could be IA ST Name, and then it is what comes out if you have 2)? Example where it makes perfect sense: ICAB for ICPPASTBaseSpecifier.

There is a trade off to be made whether 2) is valuable enough (I never know whether "ast" is AST or Ast, the projects I'm working in I always have both). I wouldn't go for a preference in the first version, it's either in or out.

Pros for having 2)
- you don't need to remember whether it's AST or Ast, and type AST if it is AST. In dynamic teams you almost certainly have both
- simple SegmentMatcher
- surprise that ICAB does not match ICPPASTBaseSpecifier

Cons
- Inconsistency with JDT
- surprise that ISN matches IASTName
Comment 29 Jens Elmenthaler CLA 2011-02-23 09:19:53 EST
(In reply to comment #28)
> (In reply to comment #27)
> > (In reply to comment #25)
> > > (In reply to comment #23)
> > > > (In reply to comment #22)
> > > >   4) IAN should match IASTName but not IASTImplicitName
> > > To be clear in this point, I really propose that IAN should match
> > > IASTImplicitName, too.
> > If it is on purpose, I buy it. I want to check for another one:
> > (5) Should ISN really match IASTName?
> Yes and no;-)
> No, because being around in the CDT I know that IASTName means I AST Name.
> Yes, because in general it could be IA ST Name, and then it is what comes out
> if you have 2)? Example where it makes perfect sense: ICAB for
> ICPPASTBaseSpecifier.
> There is a trade off to be made whether 2) is valuable enough (I never know
> whether "ast" is AST or Ast, the projects I'm working in I always have both). I
> wouldn't go for a preference in the first version, it's either in or out.
> Pros for having 2)
> - you don't need to remember whether it's AST or Ast, and type AST if it is
> AST. In dynamic teams you almost certainly have both
> - simple SegmentMatcher
> - surprise that ICAB does not match ICPPASTBaseSpecifier
Should read: - no surprise that ICAB does not match ICPPASTBaseSpecifier

> Cons
> - Inconsistency with JDT
> - surprise that ISN matches IASTName
Comment 30 Markus Schorn CLA 2011-02-23 09:27:49 EST
Ok, I am with you. Let's keep the stuff as it is and see what kind of bug reports we get. 
I guess, that a user who is unaware of the camel-case completion (which is enabled per default) will type lower-case letters. With that there won't be
any surprises.
Comment 31 Markus Schorn CLA 2011-02-23 11:09:12 EST
Jens, thanks for the great work and your patience with me. I have committed the patch with the following changes:
* Added @noreference tag to ContentAssistMatcherFactory.shutdown()
* Removed redundant fields in MacroContainerCollector and NamedNodeCollector,
  you may want to review these changes of mine. (I hope I did not break any-
  thing.)
* Changed 2010 to 2011.

In case ContentAssistMatcherFactory.createMatcher() is called very often it  may be worth to reconsider, whether it is necessary to make the method synchronized (I don't think so).
Comment 32 CDT Genie CLA 2011-02-23 11:23:19 EST
*** cdt cvs genie on behalf of mschorn ***
Bug 173458: Camel case based content assist, by Jens Elmenthaler.

[*] BasicCompletionTest.java 1.29 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core.tests/parser/org/eclipse/cdt/core/parser/tests/prefix/BasicCompletionTest.java?root=Tools_Project&r1=1.28&r2=1.29

[+] ContentAssistMatcherFactoryTest.java  http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core.tests/parser/org/eclipse/cdt/core/parser/tests/ContentAssistMatcherFactoryTest.java?root=Tools_Project&revision=1.1&view=markup
[+] SegmentMatcherTest.java  http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core.tests/parser/org/eclipse/cdt/core/parser/tests/SegmentMatcherTest.java?root=Tools_Project&revision=1.1&view=markup
[*] ParserTestSuite.java 1.48 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core.tests/parser/org/eclipse/cdt/core/parser/tests/ParserTestSuite.java?root=Tools_Project&r1=1.47&r2=1.48

[*] EmptyIndexFragment.java 1.15 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core.tests/parser/org/eclipse/cdt/internal/index/tests/EmptyIndexFragment.java?root=Tools_Project&r1=1.14&r2=1.15

[*] CompletionTests_PlainC.java 1.30 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.ui.tests/ui/org/eclipse/cdt/ui/tests/text/contentassist2/CompletionTests_PlainC.java?root=Tools_Project&r1=1.29&r2=1.30
[*] CompletionTests.java 1.55 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.ui.tests/ui/org/eclipse/cdt/ui/tests/text/contentassist2/CompletionTests.java?root=Tools_Project&r1=1.54&r2=1.55

[*] InclusionProposalComputer.java 1.6 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/text/contentassist/InclusionProposalComputer.java?root=Tools_Project&r1=1.5&r2=1.6
[*] CCompletionProposal.java 1.27 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/text/contentassist/CCompletionProposal.java?root=Tools_Project&r1=1.26&r2=1.27
[*] DOMCompletionProposalComputer.java 1.31 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/text/contentassist/DOMCompletionProposalComputer.java?root=Tools_Project&r1=1.30&r2=1.31

[*] PreferencesMessages.java 1.51 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/preferences/PreferencesMessages.java?root=Tools_Project&r1=1.50&r2=1.51
[*] PreferencesMessages.properties 1.102 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/preferences/PreferencesMessages.properties?root=Tools_Project&r1=1.101&r2=1.102
[+] AbstractMixedPreferencePage.java  http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/preferences/AbstractMixedPreferencePage.java?root=Tools_Project&revision=1.1&view=markup
[*] CodeAssistPreferencePage.java 1.19 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/preferences/CodeAssistPreferencePage.java?root=Tools_Project&r1=1.18&r2=1.19

[*] IIndex.java 1.27 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/core/index/IIndex.java?root=Tools_Project&r1=1.26&r2=1.27

[+] SegmentMatcher.java  http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/core/parser/util/SegmentMatcher.java?root=Tools_Project&revision=1.1&view=markup
[+] ContentAssistMatcherFactory.java  http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/core/parser/util/ContentAssistMatcherFactory.java?root=Tools_Project&revision=1.1&view=markup
[+] IContentAssistMatcher.java  http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/core/parser/util/IContentAssistMatcher.java?root=Tools_Project&revision=1.1&view=markup

[*] PDOMLinkage.java 1.73 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/dom/PDOMLinkage.java?root=Tools_Project&r1=1.72&r2=1.73
[*] BindingCollector.java 1.8 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/dom/BindingCollector.java?root=Tools_Project&r1=1.7&r2=1.8
[*] MacroContainerCollector.java 1.5 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/dom/MacroContainerCollector.java?root=Tools_Project&r1=1.4&r2=1.5
[*] NamedNodeCollector.java 1.12 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/dom/NamedNodeCollector.java?root=Tools_Project&r1=1.11&r2=1.12

[*] PDOM.java 1.159 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/PDOM.java?root=Tools_Project&r1=1.158&r2=1.159
[*] PDOMProxy.java 1.21 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/PDOMProxy.java?root=Tools_Project&r1=1.20&r2=1.21

[*] PDOMCPPNamespace.java 1.56 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/dom/cpp/PDOMCPPNamespace.java?root=Tools_Project&r1=1.55&r2=1.56
[*] PDOMCPPClassScope.java 1.14 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/dom/cpp/PDOMCPPClassScope.java?root=Tools_Project&r1=1.13&r2=1.14
[*] PDOMCPPEnumScope.java 1.3 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/dom/cpp/PDOMCPPEnumScope.java?root=Tools_Project&r1=1.2&r2=1.3

[*] CPPClosureType.java 1.5 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/cpp/CPPClosureType.java?root=Tools_Project&r1=1.4&r2=1.5
[*] CPPScope.java 1.62 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/cpp/CPPScope.java?root=Tools_Project&r1=1.61&r2=1.62
[*] CPPClassScope.java 1.85 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/cpp/CPPClassScope.java?root=Tools_Project&r1=1.84&r2=1.85
[*] CPPASTQualifiedName.java 1.51 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/cpp/CPPASTQualifiedName.java?root=Tools_Project&r1=1.50&r2=1.51

[*] EmptyCIndex.java 1.22 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/index/EmptyCIndex.java?root=Tools_Project&r1=1.21&r2=1.22
[*] CIndex.java 1.46 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/index/CIndex.java?root=Tools_Project&r1=1.45&r2=1.46
[*] IIndexFragment.java 1.32 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/index/IIndexFragment.java?root=Tools_Project&r1=1.31&r2=1.32

[*] CScope.java 1.35 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/c/CScope.java?root=Tools_Project&r1=1.34&r2=1.35
[*] CVisitor.java 1.151 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/c/CVisitor.java?root=Tools_Project&r1=1.150&r2=1.151

[*] CCorePlugin.java 1.158 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/src/org/eclipse/cdt/core/CCorePlugin.java?root=Tools_Project&r1=1.157&r2=1.158
[*] CCorePreferenceConstants.java 1.18 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/src/org/eclipse/cdt/core/CCorePreferenceConstants.java?root=Tools_Project&r1=1.17&r2=1.18

[*] CCorePreferenceInitializer.java 1.26 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/src/org/eclipse/cdt/internal/core/CCorePreferenceInitializer.java?root=Tools_Project&r1=1.25&r2=1.26
Comment 33 Jens Elmenthaler CLA 2011-02-23 11:26:32 EST
(In reply to comment #31)
> Jens, thanks for the great work and your patience with me. I have committed the
> patch with the following changes:
> * Added @noreference tag to ContentAssistMatcherFactory.shutdown()
> * Removed redundant fields in MacroContainerCollector and NamedNodeCollector,
>   you may want to review these changes of mine. (I hope I did not break any-
>   thing.)
> * Changed 2010 to 2011.
> In case ContentAssistMatcherFactory.createMatcher() is called very often it 
> may be worth to reconsider, whether it is necessary to make the method
> synchronized (I don't think so).
You really saved my day:-) I will check MacroContainerCollector and NamedNodeCollector once they get visible to me, but as long as the tests pass...
Comment 34 John Liu CLA 2011-04-25 15:47:20 EDT
Hi, Markus / Jens:
 
Can ContentAssistMatcherFactory class be moved to the cdt.ui plugin? As it deals with IPreferenceChangeListener directly, it breaks the the content assist function in RDT, which doesn't have the eclipse UI plugins in the class path. 

Is it possible that we let DOMCompletionProposalComputer to handle the ContentAssistMatcherFactory to produce an IContentAssistMatcher at the UI level and pass it into indexer to the do the query after?
Comment 35 Jens Elmenthaler CLA 2011-04-26 15:10:41 EDT
(In reply to comment #34)
> Hi, Markus / Jens:
> Can ContentAssistMatcherFactory class be moved to the cdt.ui plugin? As it
> deals with IPreferenceChangeListener directly, it breaks the the content assist
> function in RDT, which doesn't have the eclipse UI plugins in the class path. 
> Is it possible that we let DOMCompletionProposalComputer to handle the
> ContentAssistMatcherFactory to produce an IContentAssistMatcher at the UI level
> and pass it into indexer to the do the query after?
You are so right, content assist is a UI feature. I spent most of the time for this patch trying to find a way to achieve that - just to figure out that content assist is too deeply rooted in the core. Just passing the matcher through all the stages would have changed significant protions public APIs.

However, how can it be that using the preferences stuff creates depedencies to UI plugins? After all it's living in org.eclipse.equinox.preferences. And I didn't add new dependencies in the CDT core plugin, nor did I change the classpath. Furthermore, CDT core used those packages already.
Comment 36 John Liu CLA 2011-04-26 15:52:57 EDT
(In reply to comment #35)
> (In reply to comment #34)
> > Hi, Markus / Jens:
> > Can ContentAssistMatcherFactory class be moved to the cdt.ui plugin? As it
> > deals with IPreferenceChangeListener directly, it breaks the the content assist
> > function in RDT, which doesn't have the eclipse UI plugins in the class path. 
> > Is it possible that we let DOMCompletionProposalComputer to handle the
> > ContentAssistMatcherFactory to produce an IContentAssistMatcher at the UI level
> > and pass it into indexer to the do the query after?
> You are so right, content assist is a UI feature. I spent most of the time for
> this patch trying to find a way to achieve that - just to figure out that
> content assist is too deeply rooted in the core. Just passing the matcher
> through all the stages would have changed significant protions public APIs.
> However, how can it be that using the preferences stuff creates depedencies to
> UI plugins? After all it's living in org.eclipse.equinox.preferences. And I
> didn't add new dependencies in the CDT core plugin, nor did I change the
> classpath. Furthermore, CDT core used those packages already.

Thanks, Jens for your reply. 
CDT has no problem at all for your solution, it just breaks RDT, which runs index query at the server side, where doesn't have a full eclipse runtime.

I have a temporary work-around for RDT, I make another copy of the class ContentAssistMatcherFactory at RDT server side, in which I remove any functions that deal with the preference. Then the variable value of showCamelCaseMatches is gotten from the client side class (the original ContentAssistMatcherFactory class) and is passed to the server side ContentAssistMatcherFactory class.

To make this workaround works, I need to add a setter and a getter function for the variable of showCamelCaseMatches in the class of ContentAssistMatcherFactory, where the setter function is just for compiling purpose. I am not sure if it is OK to add these functions into the CDT head stream?

I will attach a cvs patch of the code change for your and Markus review. Please let me know if you have any better idea. Ideally, I hope we can find a way to move the preference handling functions up to the UI level and just pass a Boolean variable to the index binding find function (at the core).
Comment 37 John Liu CLA 2011-04-26 16:08:29 EDT
Created attachment 194101 [details]
The patch I described in my comments.
Comment 38 Jens Elmenthaler CLA 2011-04-26 16:16:11 EDT
(In reply to comment #36)
> (In reply to comment #35)
> > (In reply to comment #34)
...
> Thanks, Jens for your reply. 
> CDT has no problem at all for your solution, it just breaks RDT, which runs
> index query at the server side, where doesn't have a full eclipse runtime.
So, you don't use
org.eclipse.cdt.internal.core
org.eclipse.cdt.internal.core.language
org.eclipse.cdt.internal.core.model
org.eclipse.cdt.internal.core.pdom.indexer
on the server-side, they all use the preferences mechanism? Are you stripping packages out of the CDT core plugin, how does it work?
Comment 39 Jens Elmenthaler CLA 2011-04-26 16:25:27 EDT
(In reply to comment #37)
> Created attachment 194101 [details]
> The patch I described in my comments.
Could it be that the problem is that the UI is needed to alter that preference? Without it, camel case matching is always on on the server side?
Comment 40 John Liu CLA 2011-04-27 10:02:43 EDT
(In reply to comment #39)
> (In reply to comment #37)
> > Created attachment 194101 [details] [details]
> > The patch I described in my comments.
> Could it be that the problem is that the UI is needed to alter that preference?
> Without it, camel case matching is always on on the server side?

I have two copies of ContentAssistMatcherFactory class, the one at server side is removed of preference related functions, the one at the client side is your original class which handles preference change, and the value of showCamelCaseMatches is passed to the server side to set the copy class of ContentAssistMatcherFactory.

I am just thinking we can use the setter function at CDT as well, we can just move the preference lisenter to the UI plugin, which can set the value of showCamelCaseMatches to the ContentAssistMatcherFactory class at UI level when the content assist is triggered. Then if we change to this way, I don't need to have another copy of the class at the server side. What do you think about it? I can make the change and attach a patch for your consideration.
Comment 41 John Liu CLA 2011-04-27 11:50:07 EDT
Created attachment 194174 [details]
Patch option 2 mentioned in my comments

In this patch, I create a new class called ContentAssistMatcherPreference under org.eclipse.cdt.internal.ui.text.contentassist (CDT UI plugin), which listens to the content assist preference of SHOW_CAMEL_CASE_MATCHES and is to set the option value to ContentAssistMatcherFactory class when the content assist is triggered. Any functions related to eclipse preference are removed from the class ContentAssistMatcherFactory, so it can be reused by RDT at the server side.

Jens/Markus, can you please take a look at the patch and let me know if you are OK with it?

I put the setting call of showCamelCaseMatches inside of ContentAssistProcessor class, please see if it is a right place? DOMCompletionProposalComputer may be the best fit place to set it, but I am just afraid I may miss some cases.
Comment 42 Jens Elmenthaler CLA 2011-04-27 11:53:40 EDT
(In reply to comment #40)
> (In reply to comment #39)
> > (In reply to comment #37)
> > > Created attachment 194101 [details] [details] [details]
> > > The patch I described in my comments.
> > Could it be that the problem is that the UI is needed to alter that preference?
> > Without it, camel case matching is always on on the server side?
> I have two copies of ContentAssistMatcherFactory class, the one at server side
> is removed of preference related functions, the one at the client side is your
> original class which handles preference change, and the value of
> showCamelCaseMatches is passed to the server side to set the copy class of
> ContentAssistMatcherFactory.
> I am just thinking we can use the setter function at CDT as well, we can just
> move the preference lisenter to the UI plugin, which can set the value of
> showCamelCaseMatches to the ContentAssistMatcherFactory class at UI level when
> the content assist is triggered. Then if we change to this way, I don't need to
> have another copy of the class at the server side. What do you think about it?
> I can make the change and attach a patch for your consideration.
I'm very unhappy with the setter/getter pair:
1) Currently the API design is closed for change, but open for extension: I can add additional content assist schemes and options, and the API does not change
2) If I have this getter/setter pair, multiple plugins can easily take advantage and, thus, break each other.

I can circumvent 1) by not providing the setter/getter for the modes, but a setter for the actual factory. This would mean the camel case matcher would completly move into the UI and the core would always do the prefix matching, unless there isn't someone who registers something else. However, that would not solve 2). I would also like to hear the opinion of a committer.

Finally, you didn't answer my question what you do with the other classes in CDT core that use the preferences, copy them as well? Is there a CDT core plugin running at all on the server, or is it only copies of the code? Maybe they provide a better idea to address the problem.
Comment 43 Mike Kucera CLA 2011-04-27 12:04:25 EDT
Only the parser, indexer and model code is on the RDT server. The classes get ripped out of CDT and put into a jar file. This is very problematic because references to CDT and eclipse platform classes often result in NoClassDefFoundErrors. Essentially CDT is built normally with the compiler classpath including all those classes. Then afterwards the classes are ripped out and put into a jar. Not pretty.

John, I ran into a similar problem before with the XLC language preferences. I refactored RDT so that the preferences are sent over every time the parser is run. No preferences state is maintained on the server. Is it possible to follow a similar approach with the remote content assist?
Comment 44 John Liu CLA 2011-04-27 13:35:52 EDT
(In reply to comment #43)
> Only the parser, indexer and model code is on the RDT server. The classes get
> ripped out of CDT and put into a jar file. This is very problematic because
> references to CDT and eclipse platform classes often result in
> NoClassDefFoundErrors. Essentially CDT is built normally with the compiler
> classpath including all those classes. Then afterwards the classes are ripped
> out and put into a jar. Not pretty.
> John, I ran into a similar problem before with the XLC language preferences. I
> refactored RDT so that the preferences are sent over every time the parser is
> run. No preferences state is maintained on the server. Is it possible to follow
> a similar approach with the remote content assist?

Mike: That is the approach I am working on. Get the preference option and send it over the wire when the content assist is called.

Jens: 
I think Mike has answered your question, can you please look at the code change in my second patch. In this latest approach, I separate the preference listener to another class (called ContentAssistMatcherPreference) and place it in the CDT UI plugin. The ContentAssistMatcherPreference class makes sure the value of showCamelCaseMatches is updated with the preference change and pass the value to ContentAssistMatcherFactory class when the content assist is triggered. This approach has the change limited to the scope of ContentAssistMatcher and has no api change to the core index query functions.

Your suggested approach to set factory seems a good approach too. Can you please provide a detailed code change for it? However, I would prefer to at least having a getter function for the variable of showCamelCaseMatches, otherwise I will have to have another class to listen to the preference change, which is a duplicate work.

I understand that it is close to the Indigo dcut, so does RDT, but the content assist function is completely broken in RDT right now, so we have to make something to restore the function for RDT.
Comment 45 Jens Elmenthaler CLA 2011-04-27 15:50:37 EDT
(In reply to comment #44)
> (In reply to comment #43)
> Jens: 
> I think Mike has answered your question, can you please look at the code change
> in my second patch. In this latest approach, I separate the preference listener
> to another class (called ContentAssistMatcherPreference) and place it in the
> CDT UI plugin. The ContentAssistMatcherPreference class makes sure the value of
> showCamelCaseMatches is updated with the preference change and pass the value
> to ContentAssistMatcherFactory class when the content assist is triggered. This
> approach has the change limited to the scope of ContentAssistMatcher and has no
> api change to the core index query functions.
The patch should work, just that the design assumes that the value set by setShowCamelCaseMatches for one client is not changed by a second client until the proposal computation is finished. Bad design. But I don’t know a better solution, except the approach Mike was mentioning.

Here what I found:

ContentAssistMatcherFactory:
- setShowCamelCaseMatches
  - should be synchronised
  - the documentation should be adapted, at least on place in the CDT UI has to call it

ContentAssistMatcherPreference
- Since the preferences handling is not in the core, but the UI, move CCorePreferenceConstants.SHOW_CAMEL_CASE_MATCHES to a place in the UI
- Since you are in the UI plugin, you should use CUIPlugin#getPreferenceStore() in getPreferences() and updateOnPreferences()

CodeAssistPreferencePage
- it can now derive from AbstractPreferencePage again. In fact revert to the previous version and add the new preference, which is now straight forward.

> Your suggested approach to set factory seems a good approach too. Can you
> please provide a detailed code change for it?
That would basically mean to write most of the stuff anew, which I will not be able to do before feature freeze.
Comment 46 John Liu CLA 2011-04-28 10:44:56 EDT
Created attachment 194275 [details]
patch applied to cdt.ui, cdt.core, cdt.ui.tests, cdt.core.tests

Jens, I update the code fix according to your suggestions. 
Also, I remvoe the class variable of showCamelCaseMatches from the class of ContentAssistMatcherPreference in my previous patch. So the ContentAssistMatcherPreference class will update ContentAssistMatcherFactory directly when there is a change from preference page. In this way, we can guarantee the variable is always updated and no need to update it when the content assist is triggered in the content assist processor class(means no change to content assist processor).

I also move the test case from core.tests to ui.tests.

Please review the code change, let me know if you are OK with the change or if have any other suggestions. Thanks.
Comment 47 John Liu CLA 2011-04-28 17:17:19 EDT
Jens / Markus: 
I know it is a busy time, but could any of you please review my latest code patch and let me know if we can commit it into head stream? I really hope we can check it in before the feature freeze date. Thanks.
Comment 48 Jens Elmenthaler CLA 2011-04-29 03:10:51 EDT
Created attachment 194332 [details]
Patch for John

Hi John, I modified the patch slighly:
- ContentAssistMatcherFactoryTest back into the core, they test code that still is in the core. Instead of using the preferences, however, it now uses the setter.
- Renamed ContentAssistMatcherPreference to ContentAssistPreferences. I think this class should become the place for all helpers around content assist preferences.
- initialized showCamelCaseMatches in the factory to true
Comment 49 Jens Elmenthaler CLA 2011-04-29 03:13:59 EDT
Any committer out there to help John and the RDT and check + apply the last patch? I don't have commit rights, and it seems Markus is on vacation during the easter holidays.
Comment 50 Markus Schorn CLA 2011-04-29 03:42:05 EDT
(In reply to comment #47)
> Jens / Markus: 
> I know it is a busy time, but could any of you please review my latest code
> patch and let me know if we can commit it into head stream? I really hope we
> can check it in before the feature freeze date. Thanks.

The patch creates API that allows to change the behavior of content assist
without changing the preference setting. Besides that multiple clients could
concurrently try to use and not-use camel case matching, it also allows for 
introducing a discrepancy between the preference UI and the behavior. As a minimum I would require that the problematic method is marked as non-API. Nevertheless it's a hack and as soon as it is checked in you will no longer be interested in providing a clean solution.

The preference can be made a UI-preference setting, when the the
ContentAssistMatcherFactory is passed as an argument to the content-assist
within the parser. That is possible by creating new API methods at least in
IScope, IASTCompletionContext and IIndex. This would be the right thing to do. 
I encourage you to work on that.

Alternatively, to influence the behavior of content-assist in your scenario you
can provide a preference implementation on the server side. This implementation
shall return whatever setting you find best on the server side. Could be the
value transfered from the client to the server (you'd have to deal with the
conflict between different settings of different clients) or a fixed settting.
Comment 51 Jens Elmenthaler CLA 2011-04-29 04:07:24 EDT
(In reply to comment #50)
> (In reply to comment #47)
...
> The preference can be made a UI-preference setting, when the the
> ContentAssistMatcherFactory is passed as an argument to the content-assist
> within the parser. That is possible by creating new API methods at least in
> IScope, IASTCompletionContext and IIndex. This would be the right thing to do. 
> I encourage you to work on that.
Thanks for the reply, and I agree that is (almost) the right way to go: Instead of the passing the factory, I would pass the matcher itself. I think that is cleaner and it also provides for better performance (remember all the parsing and regexp compiling in the constructor + there are some things in calcuting the relevance where it would help, too).

I would want to give it a try, what would be the deadline I would have to meet?
Comment 52 Markus Schorn CLA 2011-04-29 08:04:49 EDT
(In reply to comment #51)
It'd be probably better to make this kind of change after 8.0 has been released. It is probably best to use the patch that moves the preference into the UI plugin and controls the factory from there. In addition to that we need to move ContentAssistMatcherFactory into an internal package, in order to be able to make the necessary changes after 8.0.
Comment 53 Jens Elmenthaler CLA 2011-04-29 09:29:37 EDT
Created attachment 194355 [details]
Patch for John

- Moved ContentAssistMatcherFactory to org.eclipse.cdt.internal.core.parser.util
- Merged ContentAssistPreferences into ContentAssistPreference, looks awkward having two classes dealing with content assist preferences
Comment 54 Jens Elmenthaler CLA 2011-04-29 09:36:55 EDT
(In reply to comment #52)
> (In reply to comment #51)
> It'd be probably better to make this kind of change after 8.0 has been
> released. It is probably best to use the patch that moves the preference into
> the UI plugin and controls the factory from there. In addition to that we need
> to move ContentAssistMatcherFactory into an internal package, in order to be
> able to make the necessary changes after 8.0.
I added two methods to public API that would be wrong afterwards, removing them later again is not possible, is it?

IIndex#findBindingsForContentAssist
IIndexFragment#findBindingsForContentAssist
Comment 55 John Liu CLA 2011-04-29 09:51:18 EDT
Thanks a lot, Jens / Markus for reviewing and updating the patch.

Jens: 

Your updates in your patch look good for me, just that I would prefer to at least keeping some of ui test for the ContentAssistPreference, as it also tests the preference changing and initialization. But if you and Markus are OK without the ui test, I am fine with it.

Markus:

Could you please help to check the code in once you have reviewed the updated patch from Jens?

I agree that passing in a factory or a matcher to the content assist in the parser is the best way to do it. 

I have tried your suggested alternative approach (my first patch here), but it would require the getter and setter function from ContentAssistMatcherFactory as well. As the ContentAssistMatcherFactory is called from the contenet assist query in the parser, I have to create a stub class for ContentAssistMatcherFactory with the same name and same package at the server side, and set the option value to it in fly when the server gets request. But since the RDT server jar is compiled with the cdt.core plugin also, I have to add the setter function to the original class as well. With all of these mess, it becomes a very ugly patch.
Comment 56 Jens Elmenthaler CLA 2011-05-01 05:40:48 EDT
Created attachment 194441 [details]
Patch for John (2)

- Move IIndex#findBindingsForContentAssist to a new, internal interface IIndexInternal. Having this method in public API would make it later very difficult to implement the proposed optimal solution.
- Added test for correct reponse to SHOW_CAMEL_CASE_MATCHES: ShowCamelCasePreferenceTest
Comment 57 Markus Schorn CLA 2011-05-02 05:28:16 EDT
(In reply to comment #56)
> Created attachment 194441 [details]
> Patch for John (2)
> - Move IIndex#findBindingsForContentAssist to a new, internal interface
> IIndexInternal. Having this method in public API would make it later very
> difficult to implement the proposed optimal solution.
> - Added test for correct reponse to SHOW_CAMEL_CASE_MATCHES:
> ShowCamelCasePreferenceTest

Thanks Jens and John, I have released the patch (attachment 194441 [details]) after removing the IIndexInternal interface in favor of marking the new method in IIndex with the @noreference tag. With that it is not part of the public API and can be changed or removed in the next version of CDT.
Comment 58 CDT Genie CLA 2011-05-02 06:23:03 EDT
*** cdt cvs genie on behalf of mschorn ***
Bug 173458: Make camel-case content assist a UI preference, by Jens Elmenthaler.

[*] CUIPlugin.java 1.103 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.ui/src/org/eclipse/cdt/ui/CUIPlugin.java?root=Tools_Project&r1=1.102&r2=1.103

[*] CodeAssistPreferencePage.java 1.20 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/preferences/CodeAssistPreferencePage.java?root=Tools_Project&r1=1.19&r2=1.20

[*] InclusionProposalComputer.java 1.9 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/text/contentassist/InclusionProposalComputer.java?root=Tools_Project&r1=1.8&r2=1.9
[*] CCompletionProposal.java 1.28 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/text/contentassist/CCompletionProposal.java?root=Tools_Project&r1=1.27&r2=1.28
[*] DOMCompletionProposalComputer.java 1.32 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/text/contentassist/DOMCompletionProposalComputer.java?root=Tools_Project&r1=1.31&r2=1.32
[*] ContentAssistPreference.java 1.24 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/text/contentassist/ContentAssistPreference.java?root=Tools_Project&r1=1.23&r2=1.24

[*] ContentAssist2TestSuite.java 1.10 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.ui.tests/ui/org/eclipse/cdt/ui/tests/text/contentassist2/ContentAssist2TestSuite.java?root=Tools_Project&r1=1.9&r2=1.10
[+] ShowCamelCasePreferenceTest.java  http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.ui.tests/ui/org/eclipse/cdt/ui/tests/text/contentassist2/ShowCamelCasePreferenceTest.java?root=Tools_Project&revision=1.1&view=markup

[*] ContentAssistMatcherFactoryTest.java 1.2 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core.tests/parser/org/eclipse/cdt/core/parser/tests/ContentAssistMatcherFactoryTest.java?root=Tools_Project&r1=1.1&r2=1.2

[*] CCorePreferenceInitializer.java 1.28 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/src/org/eclipse/cdt/internal/core/CCorePreferenceInitializer.java?root=Tools_Project&r1=1.27&r2=1.28

[*] IIndex.java 1.28 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/core/index/IIndex.java?root=Tools_Project&r1=1.27&r2=1.28

[*] SegmentMatcher.java 1.2 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/core/parser/util/SegmentMatcher.java?root=Tools_Project&r1=1.1&r2=1.2
[-] ContentAssistMatcherFactory.java  http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/core/parser/util/ContentAssistMatcherFactory.java?root=Tools_Project&view=markup

[*] NamedNodeCollector.java 1.13 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/dom/NamedNodeCollector.java?root=Tools_Project&r1=1.12&r2=1.13
[*] MacroContainerCollector.java 1.6 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/dom/MacroContainerCollector.java?root=Tools_Project&r1=1.5&r2=1.6

[*] CPPClosureType.java 1.6 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/cpp/CPPClosureType.java?root=Tools_Project&r1=1.5&r2=1.6
[*] CPPScope.java 1.63 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/cpp/CPPScope.java?root=Tools_Project&r1=1.62&r2=1.63
[*] CPPClassScope.java 1.87 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/cpp/CPPClassScope.java?root=Tools_Project&r1=1.86&r2=1.87
[*] CPPASTQualifiedName.java 1.54 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/cpp/CPPASTQualifiedName.java?root=Tools_Project&r1=1.53&r2=1.54

[*] PDOMCPPClassScope.java 1.16 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/dom/cpp/PDOMCPPClassScope.java?root=Tools_Project&r1=1.15&r2=1.16
[*] PDOMCPPEnumScope.java 1.5 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/dom/cpp/PDOMCPPEnumScope.java?root=Tools_Project&r1=1.4&r2=1.5

[*] CCorePlugin.java 1.162 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/src/org/eclipse/cdt/core/CCorePlugin.java?root=Tools_Project&r1=1.161&r2=1.162
[*] CCorePreferenceConstants.java 1.19 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/src/org/eclipse/cdt/core/CCorePreferenceConstants.java?root=Tools_Project&r1=1.18&r2=1.19

[*] MANIFEST.MF 1.73 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/META-INF/MANIFEST.MF?root=Tools_Project&r1=1.72&r2=1.73

[+] ContentAssistMatcherFactory.java  http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/parser/util/ContentAssistMatcherFactory.java?root=Tools_Project&revision=1.1&view=markup

[*] CScope.java 1.36 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/c/CScope.java?root=Tools_Project&r1=1.35&r2=1.36
[*] CVisitor.java 1.152 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/c/CVisitor.java?root=Tools_Project&r1=1.151&r2=1.152