I have tracked this down to a bug in the AbbrevDetection class in the editor.

Basically, if you open a Ruby file and type "foo.each" followed by tab, the template does not get expanded. The reason
this happens is that at some point while typing this string, the abbrevEndPosition is assigned to null such that when
the checkExpansionKeystroke() method is called, it does nothing (because it first checks that abbrevEndPosition is not
null).

Obviously, in the middle of typing "foo.each", there's a dot - which should naturally reset the abbreviation detector
and set the abbrevEndPosition to null. However, typing the rest should revert things, and I'm not sure why.  When I at
first inserted breakpoints in various places (insertUpdate, appendTypedText etc.) I couldn't get the bug to trigger - it
would expand each and every time. So there's something timing dependent here - but without breakpoints I can reproduce
the bug each and every time.

By the way, I can reproduce this in Java too.

In a Java class, type

x.sout followed by Tab.

This doesn't insert the sout template (System.out.println). However, if I insert a breakpoint in appendTypedText and hit
the breakpoint and resume as I typed in each character, the template -does- expand.

Obviously sout isn't a useful template here, but the "eq" for "equals" template is and should be usable in
"foo".eq[tab]. And that doesn't work because of this bug.

Tor, thanks for the detailed analysis. I'm not sure how much this affects ruby developers, but feel free to increase the
priority if this is a showstopper for ruby.

This is really important for many of the common Ruby templates. Somebody else just left a comment on my blog asking
about this:

"2. Tab activation of snippets doesn't seem to work when preceded by a period. For example, 'collection.ea' doesn't
complete the each snippet. You have to put a space before the 'ea'. The enumeration snippets (ea and friends) don't
really make much sense if you can't have a period before them."

Tor, I guess you need to change the resetting acceptor (used to prevent extra expansions e.g. if someone would type
"asout" and Tab then it should not expand the "sout")

        resetAcceptor = SettingsUtil.getAcceptor(kitClass, SettingsNames.ABBREV_RESET_ACCEPTOR, AcceptorFactory.TRUE);

It's set to AcceptorFactory.NON_JAVA_IDENTIFIER by default but feel free to change it for ruby as a quick-fix.
I thought that I could eliminate the resetAcceptor completely by computing it dynamically by using all the chars from
all the abbrevs.
It would eliminate the necessity for the setting and it would not confuse the users when entering an abbrev containing
characters included in the resetting chars. However there's a problem that some abbrevs would not expand as expected.
For example:
1) Enter CT with abbrev "a.b".
2) Now '.' becomes reset char.
3) Type "array.le" followed by Tab and now the "le" would not expand to "length".

So we probably need some more fundamental change. Let me think about it. 

Hi Mila,
I already have a custom abbreviation reset acceptor - I needed to because some of the popular TextMate snippets use
characters like ":" and "=" in their names (and these were causing code template reset rather than getting used as part
of the names).

I've verified that my reset acceptor returns false for ".", so that's not the problem... (see also my comment about
stepping through the AbbrevDetector code - it worked fine when I was single stepping so there's some kind of timing
issue in the code.)

I see. It's strange to me that this is happening when everything related to typing and CTs expansion should happen in
AWT thread.
 I guess that there might be code completion involved (being sensitive to typing '.') but I need to verify that.
Anyway I'll add a logging to the abbrevs expansion hoping that it will reveal the problem.

There definitely is code completion involved, see #116593. CC tries to handle tabs on its own. In code templates without
a dot CC does not kick in and the expansion works fine. When typing a dot CC may interfere with the code template
expansion, I think. That would probably also explain Tor's observations. When debugging CC could have been eliminated by
changing focus to the debugger or something similar and the expansion worked fine as well.

Ok, I know where the problem is: the code templates expansion is disabled while the code completion is active (active
means either computing or visible). This is to prevent insertion of non-senses, see issue #108463. Unfortunately, I do
not see any good solution for this:
-we could rollback issue #108463, which will cause that the CC will in some cases complete non-sense. I do not like this
option.
-we could disable abbrevs expansion only if the code completion is visible, but not when it is computing. The problem is
that the Tab will behave "magically" - sometimes it will be processed by the code templates (and the CT will be
expanded), sometimes it will be processed by the CC (so the common prefix will be inserted).
-change shortcut for "insert common prefix", disallow CT expansion on Enter, and rollback fix for issue #108463 - still
may be confusing for some users.
-Vita's proposal: make the code templates in the code completion expandable by tab. So if you would have CC visible and
a CT item selected, tab (more precisely the selected abbrev shortcut) would expand it. May be a good idea anyway.
Attaching patch that achieves this.

Does anybody see a better solution? Any comments to the above? Thanks.

Created attachment 49884 [details]
A patch to allow expanding of a selected code template using Tab.

Hi Jan,
I tried your patch but it doesn't seem to do anything for me -- templates don't expand after a dot and templates do get
inserted when I press Enter in the completion dialog.

I read issue 108463 but I don't really see how that specific scenario would cause a problem. Instead, I'm guessing the
trouble occurs if code completion kicks in unintentionally (e.g. if the user types in "Integer.re" slowly, such that
while . is the last character pressed, it initiates code completion). Perhaps by the time you've typed "re" and press
enter (to go to the new line, not at all aware that code completion or code templates are in effect) the code completion
dialog pops up, has "re" as a possible code template, and since Enter allows it to be selected, inserts it.

Is that the scenario we're trying to avoid?

If so, disabling code templates for code completion computations seems like the wrong way to do it.  I think Jan's
proposal of requiring Tab to insert the code templates would do the trick, but I would like two additions:

(1) It should allow the enter key to be used when code completion was invoked directly (e.g. NOT implicitly via the
CompletionProvider.getAutoQueries call).
(2) If you press Enter on a code template, it should post an error message to the status window stating that to insert
this code template, press [template expansion key] instead. If looking it up is hard, just say template expansion key,
usually tab.

I did something similar in the Ruby area for auto queries. If you type "3." you get code completion on the number 3
(which shows method from the number class). However, if you just type "3.14" slowly, as soon as you've typed "3.1" you
get to see "No suggestions" repeatedly. So in my completion provider, I keep track of whether completion was invoked
explicitly, or automatically from ".", and if the latter I hide the completion window as soon as you type in a prefix
that has no matches.   I think there are some similarities to the code templates issue. Just typing a dot might invoke
code completion but if the user isn't really trying to get completion, don't accidentally process completion items on
enter - especially code templates. However, for users who deliberately type dot and wait (I got lots of complaints early
on when I only supported explicit completion - some people didn't know about control-space, and some Mac people have
control space bound to Quicksilver) we want the popup to include code completion and be easy to select (e.g. via enter).  

A plausible additional fix would be to enable enter-selection of code templates as soon as you've either scrolled the
code completion results dialog or it's been visible for 3 seconds or more - but I know that's asking for a lot this late
in the release cycle :)

This is not a P3 for Ruby support; template editing is very common in Ruby.

I'll study Jan's patch and see if I can offer some suggestions.

Hm, I am afraid there might be some misunderstanding.

The patch is not trying to disable "Enter" on CT items in CC, but to allow expansion on [Tab] (or the selected expansion
key) for them (certain code templates appear in the CC as options, until this patch, Tab would not expand them, after
the patch they are expanded).

The problem described in issue 108463 is this: type Integer.re in Java and invoke CC (automatically, or manually). It
proposes reverse and reverseBytes. Press Tab (which is "insert prefix" in CC, like in bash). Before #108463, this would
insert "return", which is nonsense. The reporter of this issue probably used "Enter" as code template insertion key,
which made this problem much more pronounced. This was fixed by disabling code templates insertion while code completion
is active (computing or showing).

So if one types "foo.ea[Tab]" fast, the CC fires after '.', disables expansion of CT and Tab is consumed by the CC
(regardless if it is visible or not, AFAICT). When the CC is visible, then:
-before the attached patch, "insert common prefix" is performed
-after the patch, if a CT is selected in the CC, the CT is expanded. Otherwise, "insert common prefix" is performed.

One of the questions is if this is helpful in this case.

Another option would be to disable CT expansion only when CC is visible (ie. not computing) - original option 2. Would
be confusing, IMO.

Option 3 was to change "insert common prefix" shortcut to something else (some people won't like this), disallow "Enter"
as CT insertion key, and rollback the fix for issue #108463.

If this is all about supporting "Enter" as a template expansion key, then yes, I'm all for rolling back the original
fix. That's a much narrower scenario than disabling all code templates after a dot.  We've gotta make sure that at least
the default options work out of the box.

But yes, it sounds like we're not really talking about the same thing. If I read your summary correctly, you're saying
that your patch solves the problem where the completion dialog is showing and the expansion key doesn't work?

That's something which should be fixed, but not nearly as high a priority as the original bug scenario here: It's
impossible to use abbreviations after a dot - just type "foo.ea"+tab, and it doesn't expand as it should.

(Note to James - the template is "ea", not "each" - it took me a while to discover that while scratching my head
wondering why the template wasn't expanding even in simple cases :-)  )

I think the reason this isn't working is the original code in AbbrevDetection which is still there with your patch,
which disables abbreviations whenever code completion is active (AbbrevDetection.isAbbrevDisabled still calls
component.getClientProperty("completion-active").

I've been working on some changes to the patch myself to implement the changes I suggested, but it sounds like we've
been solving slightly different problems.

I'd like to fix the following:

(1) Make sure templates can be expanded immediately following a "." (unless "." is a reset abbrev char in the current
language obviously)
(2) Make sure you don't ACCIDENTALLY insert a code template on ENTER (assuming your expansion key isn't enter) if you
happen to type slowly enough that code completion kicks in.  If you're not looking at the screen and you happen to type
some code completion prefix followed by enter, then if the code completion items filtered the given code completion
trigger to the top, it's going to insert the code template on enter when you didn't realize you were entering a code
template.

I thought the second scenario was what we had to fix before we could just rollback the original AbbrevDetection fix, so
I've made some changes to try to support it.

In particular, in your patch to CodeCompletionItem, I look not only for the expansion key, but also for VK_ENTER, and I
consume the event if this is a code template item, AND it was added either as a result of auto-completion or more than 5
seconds ago. (To know if it was auto completion, I made a change to CompletionImpl to not only store completion-active
in the component's client properties, but also "completion-auto" based on whether it invoked completion from auto
queries or not.

If none of these changes are required and this was all about using Enter as the tab key, I suggest we rollback the
AbbrevDetection change, and optionally, in the options dialog, add a simple check for "Enter" as the trigger key. If
somebody tries to make Enter the tab expansion key, display a warning dialog stating that "Enter" may result in some
confusing behavior along with a pointer to this issue.

Well, no, it is not only about Enter as the expansion key (and I am not sure where I suggested this) - it is mostly
about clash between Tab as a code template expansion key and Tab as "insert common prefix" code completion shortcut. In
the example above, before the original fix, the result of pressing Tab would be "Integer.return" (which is nonsense).
After the fix, it is "Integer.reverse" (which is correct).

I think we all would like to fix (1), I just do not know how exactly.

Sorry, but I am quite against (2), mainly because I think it is inconsistent (Enter works for all other completion
items, and I would expect it is the most commonly used shortcut to confirm a CC item), and would probably be confusing
(sometimes Enter works, sometimes not).

Interesting - after years of using NetBeans I had never realized that Tab was itself used inside the code completon
dialog, I guess to insert as much text as possible until the proposed items differ.

I don't really care if #2 gets fixed, I don't think it's very common. If we can just fix #1 I think we'll be okay.

Did I understand it correctly that we can fix #1 by just reverting the AbbrevDetection fix, and that will only affect
people who have bound code template expansion to Enter? If so, I think that's the way to go along with a warning in the
options dialog to people attempting to use Enter.

"Did I understand it correctly that we can fix #1 by just reverting the AbbrevDetection fix, and that will only affect
people who have bound code template expansion to Enter?": with default settings (CT expanded on Tab) one would get
"Integer.return" in the case described the above.

Okay, here's an alternative simple patch.  It basically just disables abbreviation when completion is -visible- instead
of -active- (completion is considered active as soon as you type a dot even though you continue typing and the
completion is put back in inactive state).

The diff to CompletionImpl just adds a new client property, similar to completion-active, named "completion-visible",
which is set to true right before showing the popup, and to false when the popup is hidden.

The diff to AbbbrevDetection just changes the abbreviation-disabling from the active state to the disable state.

This now works the way I think you'd expect:

If you type "Integer.", get a code completion popup and type "re"+tab, it will just select the "reverse" item. However,
if you type "Integer.re"+tab (no code completion) it inserts the code template as expected.

Can you please apply this fix in time for beta2? I know Hrebejk downgraded it to a P3, but he provided no explanation
and I have stated twice that this issue is important for Ruby editing. There was even a plea on the mailing list for
Ruby yesterday for this issue to get fixed.

Here's the patch - I'll attach it as a proper patch file as well.


Index: editor/codetemplates/src/org/netbeans/lib/editor/codetemplates/AbbrevDetection.java
===================================================================
@@ -99,7 +99,7 @@
     private static final String ABBREV_IGNORE_MODIFICATION_DOC_PROPERTY
             = "abbrev-ignore-modification"; // NOI18N
 
-    private static final String COMPLETION_ACTIVE = "completion-active"; // NOI18N
+    private static final String COMPLETION_VISIBLE = "completion-visible"; // NOI18N
 
     private static final String SURROUND_WITH = NbBundle.getMessage(SurroundWithFix.class,
"TXT_SurroundWithHint_Label"); //NOI18N
     private static final int SURROUND_WITH_DELAY = 250;
@@ -243,7 +243,7 @@
     }
     
     private boolean isAbbrevDisabled() {
-        return org.netbeans.editor.Abbrev.isAbbrevDisabled(component) ||
Boolean.TRUE.equals(component.getClientProperty(COMPLETION_ACTIVE));
+        return org.netbeans.editor.Abbrev.isAbbrevDisabled(component) ||
Boolean.TRUE.equals(component.getClientProperty(COMPLETION_VISIBLE));
     }
     
     private void checkExpansionKeystroke(KeyEvent evt) {

Index: editor/completion/src/org/netbeans/modules/editor/completion/CompletionImpl.java
===================================================================
@@ -837,6 +837,7 @@
                 }
                 
                 int selectedIndex = getCompletionPreSelectionIndex(sortedResultItems);
+                getActiveComponent().putClientProperty("completion-visible", Boolean.TRUE);
                 layout.showCompletion(noSuggestions ? Collections.singletonList(NO_SUGGESTIONS) : sortedResultItems,
displayTitle, displayAnchorOffset, CompletionImpl.this, displayAdditionalItems ? completionShortcut : null, selectedIndex);
                 pleaseWaitDisplayed = false;
 
@@ -909,6 +910,7 @@
         if (!completionOnly && hidePerformed && CompletionSettings.INSTANCE.documentationAutoPopup()) {
             hideDocumentation(true);
         }
+        getActiveComponent().putClientProperty("completion-visible", Boolean.FALSE);
         getActiveComponent().putClientProperty("completion-active", Boolean.FALSE);
         return hidePerformed;
     }

Created attachment 50840 [details]
Proposed fix: only kill abbreviations when the completion popup is -visible-

Sorry, when I said:
  The diff to AbbbrevDetection just changes the abbreviation-disabling from the active state to the disable state.
I meant
  The diff to AbbbrevDetection just changes the abbreviation-disabling from the active state to the VISIBLE state.

I have committed slightly modified version of Tor's patch. The modification is that isAbbrevDisabled is also being
called in checkExpansionKeystroke to cover cases like:
-have "Integer."
-type "re"
-press Ctrl-Spac
-tab should insert "reverse"

Dusan please review the fix:
http://editor.netbeans.org/source/browse/editor/codetemplates/src/org/netbeans/lib/editor/codetemplates/AbbrevDetection.java?r1=1.17&r2=1.18
http://editor.netbeans.org/source/browse/editor/completion/src/org/netbeans/modules/editor/completion/CompletionImpl.java?r1=1.85&r2=1.86

Max, Jirka, please try to test it - this is being considered for beta 2.

Tor, could you please verify it works as expected for you (I did some experiments in both Java and Ruby, and it seems OK
in both)? Thanks.

If there is positive feedback, I will merge the fix to the beta2 branch.

Thanks.

Hi Jan,
I've tested your fix and it works beautifully for Ruby. I also didn't see any strangeness in Java editing (and I could
confirm that the Integer.reverse-scenario works as intended).  So thumbs up from me!

(I also read the code diffs and they look good to me).

Thanks for all your help with this!!

The attached diffs look OK to me.

The functionality is ok, there are only two minor problems when using in java. I thing we can live with them, but just
for evaluation, maybe the fix is simple:
1) when '.' is typed in java editor, the code completion will pop up. The chars typed when the CC is opened are not
respected when it is closed by Esc 

2) the code templates are not listed in code completion if invoked behind '.'

Jirka, do you agree with merging into the beta2 branch? Thanks.

Yes, I agree, the fix can go to beta2 branch

Done.

Checking in codetemplates/src/org/netbeans/lib/editor/codetemplates/AbbrevDetection.java;
/cvs/editor/codetemplates/src/org/netbeans/lib/editor/codetemplates/AbbrevDetection.java,v  <--  AbbrevDetection.java
new revision: 1.17.2.1; previous revision: 1.17
done
Checking in completion/src/org/netbeans/modules/editor/completion/CompletionImpl.java;
/cvs/editor/completion/src/org/netbeans/modules/editor/completion/CompletionImpl.java,v  <--  CompletionImpl.java
new revision: 1.85.2.1; previous revision: 1.85
done

verified in beta2