Community
Participate
Working Groups
Created attachment 181568 [details] patch to provide cdt editor feature to auto-replace "." with "->" under appropriate circumstances a user has requested for a feature to replace "." with "->" when appropriate when typing. the attached patch performs this feature under the following conditions. - the new pref for the feature must be checked - the user must be typing the member name immediately after entring '.' - the user is allowed to backspace to delete & continue typing - the user's member/field name entered must be amongst Content Assist choices by using ContentAssist, it will help distinguish when looking for values for a pointer versus for an object instance. no attempt to perform replacement will occur for non-pointers if that is how ContentAssist would treat it. the attempted replacement will occur when the user types any non-C-ident character. this includes arrow keys to navigate away. the user can abort replacement by hitting ESC at any point during member typing to skip attempted insertio. if the insertion at the end of the member is a "(" or "[" (or any other character for that matter, PointerReplacer will do its work first, and then the existing VerifyKeyListener BracketInserter will do its work second. undo will put the "." back in place and allow the user to continue as previously. this feature is subject to the minor whims of the ContentAssist suggestions (there appear to be some instances where it makes some suggestions.
Created attachment 181569 [details] patch for auto-replace cdt editor feature for CDT_7_0 branch the second patch on the ticket is effectively the same as the first, except that instead of using PreferenceConstants.java to define a new preference string, the strings are simply added and agreed to by the provider and consumer so that the API is not changed. we're interested in this patch because our next release in which we'd like to include this feature is planned on the CDT_7_0 branch.
Created attachment 181570 [details] patch to provide cdt editor feature to auto-replace "." with "->" under appropriate circumstances
Created attachment 181571 [details] ppatch for auto-replace cdt editor feature for CDT_7_0 branch
Frankly, I am not very happy with this feature. Others might have a different opinion, but here are my points: - Performance: Calling into content assist from a KeyVerifyListener can block the user quite a while. This is a no-go. - The criterion for replacing '.' with '->' is rather weak. A test whether the parent expression resolves to a pointer type seems more appropriate. - Replacing the operator while typing the end of the member identifier is surprising. I would expect the '.' gets replaced immediately. - The fact that ESC cancels the auto-replacement is completely non-obvious. Nobody would know that. - Users coding C/C++ should know about the types they are dealing with. Helping them in this way seems not "right". I'd rather try to implement this as a custom content assist proposal computer (there is an extension point for that) or maybe as a Codan check with a quick.
(In reply to comment #4) > Frankly, I am not very happy with this feature. Others might have a different > opinion, but here are my points: > > - Performance: Calling into content assist from a KeyVerifyListener can block > the user quite a while. This is a no-go. > > - The criterion for replacing '.' with '->' is rather weak. A test whether > the parent expression resolves to a pointer type seems more appropriate. > > - Replacing the operator while typing the end of the member identifier is > surprising. I would expect the '.' gets replaced immediately. > > - The fact that ESC cancels the auto-replacement is completely non-obvious. > Nobody would know that. > > - Users coding C/C++ should know about the types they are dealing with. > Helping them in this way seems not "right". > > I'd rather try to implement this as a custom content assist proposal computer > (there is an extension point for that) or maybe as a Codan check with a quick. Toni, Thanks so much for taking the time to look at this carefully. I would really like to know what you were trying to suggest with regard to "a Codean check with a quick" … the rest of your thought seems to have gotten cut off. Regarding not "helping" users in this way and that they should "know" the types they are dealing with … and whether or not this should even be implemented: i will discuss this with others internally regarding our tools, as this is a user request from our community. My personal opinion on this is that, for us, as a user-requested feature that i can imagine finding useful myself in a C/C++ environment, i'm inclined to want to respond to their request by implementing it. If we are to move forward with this, the following are in response to try to address some of the points you made. - immediate post-'.' invocation and checking the pre-'.' expression for pointer type: i think i was fooled by the fact that content-assist makes some suggestions regarding different possibilities for post-'.' auto-completion, and that it would be necessary to find out what the user was attempting to refer to. after looking at these suggestions and double-checking my C++ manual's chapter on multiple-inheritance, i don't think the suggestions make sense … so i will look into both of your suggestions should we decide to proceed. - content-assist performance: realize that this patch completely avoids the UI, and in my practice testing (admittedly not extremely complex, but involving some multiple inheritance), it has seemed instantaneous. is it possible that the "block" you are afraid of is the less instantaneous content-assist UI being built and displayed? still, i will look into more efficient ways of doing this. - ESC as an escape: this is easy enough to remove. i included it primarily for the multiple inheritance case above and also for other cases i may not have thought of. the use of ESC seems no less non-obvious than the use of RET to move the cursor to the end of an auto-completed bracket/brace pair (a feature i was not even aware of until i started working in the guts of this code.) of course, if there's no point in post-'.' insertion of values for pointer types, this is a somewhat meaningless mini-feature. Thanks again for your review.
(In reply to comment #5) > I would really like to know what you were trying to suggest with regard to "a > Codean check with a quick" … the rest of your thought seems to have gotten cut > off. Sorry, this should read "a Codan check with a quick fix". I have to admit I don't know much about Codan and if it would be a good way to implement this. > - content-assist performance: realize that this patch completely avoids the > UI, and in my practice testing (admittedly not extremely complex, but > involving some multiple inheritance), it has seemed instantaneous. is it > possible that the "block" you are afraid of is the less instantaneous > content-assist UI being built and displayed? still, i will look into more > efficient ways of doing this. Invoking content-assist (with or without UI) requires to parse the file up to the point of completion and it requires to query the index for the prefix. Both can take a considerable amount of time depending on the size of the file and the responsiveness of the index which depends on many factors. Although performance of both has been improved in the last few years, we still get complaints about blocking the UI on content-assist auto-activation (e.g. bug 126698 and duplicates). In this particular case the index is queried with a prefix which reduces the performance hit, but I still think this is too risky. > - ESC as an escape: this is easy enough to remove. i included it primarily > for the multiple inheritance case above and also for other cases i may not > have thought of. the use of ESC seems no less non-obvious than the use of > RET to move the cursor to the end of an auto-completed bracket/brace pair > (a feature i was not even aware of until i started working in the guts of > this code.) of course, if there's no point in post-'.' insertion of values > for pointer types, this is a somewhat meaningless mini-feature. When brackets are auto-inserted you get into "linked mode" which is indicated in the UI. I.e. you have at least some visual clue that you are in a special mode.
Created attachment 182027 [details] Experimental implementation I could not resist and quickly hacked this into the content assist processor. If you invoke content assist (automatically or manually) on a '.' and the expression resolves to a pointer type, then the '.' is replaced by '->' and content assist is invoked on the adjusted operator.
I like the idea of replacing '.' with ->. I use this in Visual Assist. I'm pretty lazy, I find it's easier to type '.' every time! There is also an option to do this when operator-> is overloaded. (In reply to comment #7) > If you invoke content assist (automatically or manually) on a '.' and the I tried this patch, it worked for the automatic case but not manual. class Foo { public: void test(); }; int main() { Foo * bar; bar. //press ctrl+space here fast enough to invoke content assist manually (depending on your Auto-activation delay preference), it will result in bar.bar return 0; }
Created attachment 182129 [details] patch starting on changes to prefs for Content Assist patch toni, i like your patch much more than mine for a lot of reasons (makes use of the IAST that i'm still learning; cleaner with less state-machine; more contextually relevant to the user insertion point) ... i think it would make the user happy who submitted this bug/enhancement to us. since i'm interested in seeing this implemented in this way, i started adding prefs CEditorPreferencePage_ContentAssistPage_replaceDotWithArrowManually and CEditorPreferencePage_ContentAssistPage_autoActivationEnableReplaceDotWithArrow to Content Assist pref panel "Insertion" and "Auto-activation" group boxes. (see attachment.) but before i go fully hooking up these prefs into your attachment and creating listeners for user pref changes … is there interest in taking this forward? i'm hoping the fact that you coded up the patch is an indication of a stronger "yes" than your original doubts in your original comment. also, still interested in getting this into CDT_7_0 as an improvement, since that's where we're based for a few months going forward. will be glad to do the prefs-hookup work if so … finally … (In reply to comment #8) > (In reply to comment #7) > > If you invoke content assist (automatically or manually) on a '.' and the > > I tried this patch, it worked for the automatic case but not manual. i also tried the patch with the same problem, but if i got rid of the 'isAutoActivated' in the following conditional: if (isCompletion && getActivationChar(viewer, offset) == '.') { then it also gets invoked upon the user entering 'ctrl-space'
(In reply to comment #9) > since i'm interested in seeing this implemented in this way, i started adding > prefs Is it really necessary to have two options for that? I think one checkbox for both the manual and auto-activated case should suffice. > > but before i go fully hooking up these prefs into your attachment and creating > listeners for user pref changes … is there interest in taking this forward? > i'm hoping the fact that you coded up the patch is an indication of a stronger > "yes" than your original doubts in your original comment. Well, a '.' after a pointer type really does not make a lot of sense and this case is easy to detect. Also as part of content assist, the issue of potentially blocking the UI is no worse than before. > also, still interested in getting this into CDT_7_0 as an improvement, since > that's where we're based for a few months going forward. > > will be glad to do the prefs-hookup work if so … Sorry but new preferences and UI changes are not allowed for a maintenance release. But you could add the feature in 7.0.2 with a hidden preference such that you can enable it in your product. > > I tried this patch, it worked for the automatic case but not manual. > > i also tried the patch with the same problem, but if i got rid of the > 'isAutoActivated' in the following conditional: > > if (isCompletion && getActivationChar(viewer, offset) == '.') { > > then it also gets invoked upon the user entering 'ctrl-space' Right, I only realized afterwards that the feature should actually work in both the automatic and in the manual case. What it misses is the ability to do the replacement when parts of the member identifier has already been typed, but I assume this should not be much harder.
(In reply to comment #10) > Is it really necessary to have two options for that? I think one checkbox for > both the manual and auto-activated case should suffice. you're right, it's not really necessary. not really sure which group to put it in, if any, nor what the best way to word the pref would be. > But you could add the feature in 7.0.2 with a hidden preference such that you > can enable it in your product. a hidden preference will be fine for the interim release we'll be doing will be fine. i will code it up with the hidden pref for 7.0.2 and with the full pref for HEAD. > What it misses is the ability to do the replacement when parts of the member > identifier has already been typed, but I assume this should not be much harder. i will take a look at this tonight or tomorrow … i'd like to learn a little more about what you've done by trying to augment it in this way. but if you have any good ideas, don't let my talking about doing something about it soon stop you from doing more if you are feeling so inclined!
Created attachment 183510 [details] Experimental implementation, overloaded operator What's the status on this? I added the overloaded operator case, but it's missing the ability to cancel it (with backspace?) and a preference. I'd really like to see this in CDT!
(In reply to comment #12) > Created an attachment (id=183510) [details] > Experimental implementation, overloaded operator > > What's the status on this? I added the overloaded operator case, but it's > missing the ability to cancel it (with backspace?) and a preference. I'd really > like to see this in CDT! sorry for the delay, marc. my part is on hold temporarily while dealing within edc inline stepping issues. it is next on my priority queue to properly do a preference (hidden for 7.0.2, live for HEAD) and look at the other issues on this ticket. was hoping to get to it this weekend or monday, actually.
Created attachment 183640 [details] patch containing Marc-Andre's changes plus code manipulating and using the preference for it (In reply to comment #12) > Created an attachment (id=183510) [details] > Experimental implementation, overloaded operator > > What's the status on this? I added the overloaded operator case, but it's > missing the ability to cancel it (with backspace?) and a preference. I'd really > like to see this in CDT! the newly attached patch contains the preference code, but nothing for canceling the operation. not certain what would be best for this, and for now, if the user hits esc, it cancels and leaves the '->'; if the user hits ctrl-z, it cancels and puts the '.' back. and if the user just types . and the next character fast enough, it won't auto-replace for that instance. hopefully that's good enough. i've added the checkbox with the other auto-activation characters, and modified Toni & Marc's patches so the user can either turn on the auto-activation for replace '.' with "->", only turn on auto-activation for content-assist for '.', or both, or neither. pending review, i'm hoping this update of the existing patch should be good enough to commit when you are ready. and as previously requested, i'd like to the bits that can go into CDT_7_0 there if possible. once this has been committed to head, i'll cut it back to go into my CDT_7_0 workspace for commit there as well.
(In reply to comment #14) > Created an attachment (id=183640) [details] > patch containing Marc-Andre's changes plus code manipulating and using the > preference for it Thanks for the patch. 1) I don't think it is reasonable to replace '.' with '->' for C++ types which overload the '->' operator. It is still legal to access members with the '.' operator, therefore an automatic replacement is not appropriate in this case. 2) The IASTCompletionNode can be null. This needs to be checked. I noticed that shortly after I attached my initial patch. 3) The preference constant EDITOR_REPLACE_DOT_WITH_POINTER is obsolete and should be removed. 4) class ActivationSet should be static. 5) Please fix the warning about null pointer access (enclose in if(...)) even if it's a false positive. 6) Please update the Copyright year of changed files and add a contributor line.
Created attachment 183742 [details] updated patch incorporating review comments thanks for the review, Toni. 1) regarding auto-replacement for overloaded '->' … it's gone. i personally don't feel strongly either way about it. i just picked it up after Marc-Andre had enthusiastically contributed it. and i could see a pref that would allow it and even go further, giving the user the full set of completions for any operators, and then sticking the right one in after the user choice … but that's a bit more ambitious than the request by our users to add this. the rest were simple, and are done. 2) IASTCompletionNode null-check now performed 3) EDITOR_REPLACE_DOT_WITH_POINTER pref now removed 4) class ActivationSet now static 5) null-checking the false-positive context access in the catch block 6) copyright sections updated plus, fixed the javadoc to better explain the setXXXAutoActivation() additions in contrast with (and referencing in the javadoc) the original on which they are based and which they work together with. plus, moved both new auto-activation set functions closer to their usage point in the code
(In reply to comment #16) > 6) copyright sections updated Thanks for the updated patch. I have committed it with minor changes to comments and formatting. Note that * Copyright (c) 2000, 2008 IBM Corporation and others. becomes * Copyright (c) 2000, 2010 IBM Corporation and others. instead of * Copyright (c) 2000, 2008, 2010 IBM Corporation and others.
Created attachment 183797 [details] Toni's HEAD commit merged to CDT_7_0 branch thanks for the mainline commit. the attached patch selectively pulls the same changes from HEAD to CDT_7_0 for commit on that branch. i believe there are no API level changes, so this patch should merge cleanly. i've tested it and it appears to build and work the same.
(In reply to comment #18) > i believe there are no API level changes, so this patch should merge cleanly. > i've tested it and it appears to build and work the same. I committed the patch to cdt_7_0 without the preference UI, because that's considered an API change as well (because translations are done for major releases only). I also changed the default preference value to false. You need to create your own preference UI and/or add an entry to your product's plugin_customization.ini.
