Bugzilla – Full Text Bug Listing
| Summary: | Add emacs-style "Alt-/" hippie auto completion | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Lev Epshteyn <levik> | ||||||||||||||||||
| Component: | Text | Assignee: | Tom Hofmann <eclipse> | ||||||||||||||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||||||||||||||
| Severity: | enhancement | ||||||||||||||||||||
| Priority: | P3 | CC: | akiezun, brian, burner, changzhouwang, daniel_megert, eclipse, jasc, jlquinn, johan.walles, jon.vandegriff, lorenzo.donati.bz | ||||||||||||||||||
| Version: | 2.0 | Keywords: | helpwanted | ||||||||||||||||||
| Target Milestone: | 3.1 M5 | ||||||||||||||||||||
| Hardware: | Other | ||||||||||||||||||||
| OS: | All | ||||||||||||||||||||
| Whiteboard: | |||||||||||||||||||||
| Bug Depends on: | 5998 | ||||||||||||||||||||
| Bug Blocks: | |||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||
|
Description
Lev Epshteyn
not planned for 2.0 unless we get external help Erich, I wrote the code that does just this. I attach the project workspace of the plugin I wrote for it. I think you can add it just as-is (and please keep the @author). Genady Created attachment 806 [details]
Auto completion as a plugin
Just one note: It looks only in the open documents in the current page, and does not prioritize recent completions (but it prefers the current editor). If anybody would like more functionality I can add it. Wow, great plugin. It's really helpful for me, since I've been emacsing so long I even hit Alt-/ in vi on occasion. Is is possible to add an Alt-Backspace binding to delete the word? Thanks for the contribution! We are only one day away from the 2.0 freeze and this gives us no time to iterate on this. We will look at it but I cannot make any promises to get this into 2.0. What I can offer as a fallback is to make the plugin available as an optional JDT plugin from the JDT component page. I have a sourceforge project for it: http://sourceforge.net/project/showfiles.php?group_id=53485 It will be available there with any enhancements if needed. Genady reopen to track this again having this available as a separate plugin is a good solution for 2.0. we cannot offer more integration of this for 2.0. Added link to plugin from JDT UI component page Defer Reopening for 2.1 consideration Are you going to implement this feature although a plugin is available ? Genady I think it would be a good idea to make this part of the generic text editor. I can contribute the code for eclipse 3.0. Genady *** Bug 28580 has been marked as a duplicate of this bug. *** I would prefer to have the actual key configurable, but I guess it will most likely be that anyway... is there any status update on this RFE ? >is there any status update on this RFE ?
There are no plans to include this for 3.0 except we get an external contribution.
*** Bug 53829 has been marked as a duplicate of this bug. *** If this gets added to Eclipse, please ensure you allow the key binding to be changed. I am an ex-Netbeans user, and in reprospect the thing I miss the most was Ctrl- K/L, which did this functionality backwards/forwards. Extremely powerful. I can integrate my code into eclipse. just tell me what plugin to modify This would be in one of the Text plug-ins (most likely org.eclipse.ui.workbench.texteditor). Reading the comments there seems to be no UI involved (except for inserting or overwritting with) the completion. You could contribute a command to plugin.xml together with a key sequence (to let others change it ;-) and write an action that gets triggered by that command. Intersting issue: would it only look in editors of same type (e.g. Java editor only looks in other Java editors) or would it look for completions in all text-based editors? The existing lunar eclipse plugin looks in all open editors and I had no complaints about that. Comments are welcome here as well. Another question is whether it should only look at files in open editors or the n last recently used. Reason: some users have set the max. open editors to e.g. 3 and they would be somehow limited. It would also make the feature more usable in cases where users have the habit to close editors aggressively. IMO hippie-expand should prefer editors of the same kind as the current one, but if nothing is found there it should fall back on other editors. Same kind first strategy sounds good to me. I will be working against the 3.0.x stream and later merge it with HEAD. I diffed the two streams and I see that not much has changed. Created attachment 14942 [details]
First patch
My first attempt. Can somebody look at this and tell me whether I'm doing the
integration into the editor actions right?
Did anybody try it? Sorry, not yet. Tom, please look into this during 3.1 M5. Created attachment 17032 [details]
HippieCompletion.diff against org.eclipse.ui.workbench.texteditor
Nice work, Genady, thanks.
We would definitely like to include this into the 3.1 stream. There are a
couple of things I have already changed, and some more points I would like to
see changed before commiting it.
Phew, this is a long list, but most of it is straight forward, so don't worry.
-tom
A) Things I changed (see patch against ~20050110 HEAD)
------------------------------------------------------
EmacsCompletionAction:
1) removed field 'editor' and replaced references by calls to
super.getTextEditor()
2) removed calls to setHighlightRange and showHighlightRangeOnly in 'run' as
these do something completely different (they enable the "show selected element
only" editor mode where you only see one method at a time)
There is currently no easy way to temporarily highlight parts of the editor.
There are annotations, which you could use here, or see what the find-replace
dialog does to highlight the focus section.
3) added copyright header, adding you as the initial contributor
AbstractTextEditor:
4) don't mark the action as selection/content dependent, since you register
your own listeners. If you do, override IUpdate.update to do something
meaningful. See (8).
B) TODO's before inclusion into the eclipse 3.1 stream:
-------------------------------------------------------
5) rename *EmacsCompletion* to *HippieCompletion* or anything else less
copyright-problematic.
6) create correctness TestCases and add to WorkbenchTextEditorTestSuite.java
7) Style / Spelling (being picky - but if you don't fix it now, nobody ever
will...)
- DocumentChangeListerner -> DocumentChangeListener
- use platform-text coding style
- field prefix: fMyField
- compact assignment: i= 3
- why not make EmacsCompletion final?
- visibility - make getCurrentPrefix private
- get rid of autogenerated catch blocks and System.out.println
- no *-imports
8) handle document input changes
AbstractTextEditor implements IReusableEditor, so you have to be prepared to
the editor's document being changed. You can either do that by registering an
ITextInputListener with the editor's viewer (being in the same package you have
access to AbstractTextEditor.getViewer), or by marking EmacsCompletionAction as
'content dependent', so that the action's IUpdate.update() method will be
called on any text or text input change.
If you choose the latter, you can get rid of the document listener all
together.
9) Selection handling
You currently use a combination of DocumentEvent and SelectionEvent to clear
your state (see (8) about the document changes). The problem with selection
events is that they are only sent out when the selection length changes (i.e.
from an empty selection to a non-zero-length selection and vice versa). That
means that you don't loose your state if you just change the caret position
using the cursor keys.
Try this (| is the caret):
===== snip ======
fooone
footwo
foothree
foo|
==================
- invoke completion once
- press ARROW_UP
- invoke completion
expected: completion occurs at the current location
actual: completion occurs at the previous location (state was not cleared)
You could solve this by tracking the position of your last edition and clearing
the state in run() if the position is not the same.
10) postfix handling (this is an enhancement request, I guess):
Consider this:
==========
fooone
footwo
foo|one
==========
- invoke completion once.
Expected: the caret is moved behind fooone
Actual: I get fooone|one
-> you could try to be smart and jump over any matching postfix
11) selection updating
You currently use ISelectionProvider.setSelection. This is fine, but causes
flickering on some platforms due to its wrapping call to
StyledText.setRedraw(off/on). Since you make only very small changes to the
document, and revealing will not usually be needed, consider calling
setSelectedRange on the editor's viewer.
*** Bug 5998 has been marked as a duplicate of this bug. *** Tom, Thanks for your comments and corrections (I noticed that you have also corrected some comments that appear in section B). Regarding item 8 - I'm not sure I fully understand the difference between the setEditor method and the update method. Why is the latter called on editor reuse and not the former? Created attachment 17127 [details]
Few comments addressed
The patch is against M4. from the suggested changes - items 6 and 10 were not addressed. 6-I will be working on it. Regarding 10 - this is not how Emacs works, so I'm not sure whether it is appropriate. Still to do: 1) Prefer matches before the cursor over matches after the cursor in the same document. Initially implementation was deferred because of bug #74993 2) Prefer matches from editors of the same type. What do you think is the appropriate regex for matching a word? Currently I use "\w+". Should it contain digits/underscore/other symbols? Created attachment 17183 [details] New patch I consider it almost final, since I have no good ideas of what to do about the open issues. New stuff: Backwards search is preferred over forward search in the same editor (the completion order is - search backwards, search forward, search in other editors). Bugfix of bug #74993 is not applicable, naive (but efficient) implementation used. Open issues: I didn't find a reasonable way to check whether two editors are of the same "type" (define "same type"). What is a "word". "\w+" equals "[a-zA-Z_0-9]+". Should anything be done for non-ascii (accents, bidi, chinese) characters? Is it relevant for the users? Created attachment 17184 [details]
tests patch
Both this and previous patch are against M4.
I have compiled all the changes into a plugin that replaces M4's org.eclipse.ui.workbench.texteditor. It can be downloaded from http://lunar- eclipse.sf.net/org.eclipse.ui.workbench.texteditor_3.1.0_with_completion.zip and extracted into the eclipse installation (it overwrites some files, but that is ok). Genady, sorry for not getting back to you sooner, I was away... I will be looking at the patches and give you feedback shortly. For now: > Regarding 10 - this is not how Emacs works, so I'm not sure whether it is > appropriate. Agreed. > Regarding item 8 - I'm not sure I fully understand the difference between the > setEditor method and the update method. Why is the latter called on editor > reuse and not the former? Hm... I guess this is not all too well documented. The IUpdate interface (implemented by TextEditorAction) exists so not every editor action needs to manage its own plethora of listeners with the editor and viewer. If an action is marked as being XXX dependent (see AbstractTextEditor.markAsXXXDependentAction), the editor will call the action's update() method when certain things happen. Specifically for content-dependent actions, the update method is called on TextEvents. So, there are two ways to make your action react on document changes: either mark it as content-dependent, and implement update(), or don't mark it as content dependent and register your own document listener. Oh, and I was wrong when I said that registering as content-dependent will get you the input-document-changed events as well. This is simply not true (see AbstractTextEditor.TextListener), so you do need your own IEditorInputListener. > What is a "word". "\w+" equals "[a-zA-Z_0-9]+". Should anything be done for > non-ascii (accents, bidi, chinese) characters? Is it relevant for the users? We should indeed support unicode. Character.isJavaIdentifierPart would be a good start also for general text, we don't need any complicated BreakIterator stuff for this. [a-zA-Z_0-9]+ is definitely not enough (whether in Java or text). Created attachment 17243 [details]
HippieCompletion2.diff against org.eclipse.ui.workbench.texteditor
Genady,
I looked over your patches. Good progress, most of it is fine, also the tests.
I guess I created a little confusion with my remark about ITextInputListener
and all, thats why I redid the listener story to some extent.
I changed a couple of formalities plus a number of real things, as you see
below, and there are six more points that I would like to see changed, but only
one is non-trivial. Could you have a look and resubmit your patch? I expect
this to be the last iteration.
-tom
-- snip --
CHANGES:
A) formal stuff:
- we don't use "(non-javadoc)" to mark inherited doc comments any more
- reformatted using tabs, (almost) no line continuations
- added logging for exceptions
- spelling errors
- added @since tag
B) algorithmic stuff:
Added correct regex word pattern for javaIdentifierPart
Added try ... finally in completeNext to ensure fModifyLock gets reset.
getCurrentPrefix should not copy the entire document!
Modified how exceptions are handled: exceptions when accessing other editors
are logged, but ignored, exceptions from the current document result in
aborting, since we have a serious problem then (concurrent modification in a
non-ui thread, which theoretically cannot happen - but who knows).
Redid the listener / state validation stuff. Sorry for changing so much here -
my main concern was to avoid leaks (listeners, fDocument) and to avoid
installing listeners where not needed. It should be pretty obvious if you look
at the code - feel free to change and rename as you like when resubmitting...
The cached completion state has clearer semantics now. It can be asked for
validity (isStateValid), it can be cleared (clearState, which happens upon
selection and content changes), and it can be updated (updateState). I got rid
of the update method along the way - as you noted, the update we receive from
the editor comes too late, because it gets async-posted - no need to mark the
action as contentDependent now in AbstractTextEditor.
TODOs:
- make HippieCompleteAction package visible (we don't want to make it API just
yet :-)) and final (why not?)
- add javadoc comments to all fields and make them private (listeners)
- reorder members in this order: constants, static inner types, inner types,
constructor, then append methods in any order that makes sense to you
- add a help context id analogous to the other actions
- no public or protected members where not needed
- I wouldn't make the static methods static either, even though they do not
depend on the state. If you want to refactor the completion-searching
algorithm, I would prefer moving it to its own class (I guess you made them
public static for testing... the clean way would definitely be to separate
engine and action that you could then test separately).
Hi Tom, I would be happy to get rid of all the public stuff, but I need that for testing. I noticed that an Accessor class is used in some of the existing tests, but it does not work well with static methods. I can modify that class if needed. Please let me know what you think. We need to get rid of the public stuff. There are two approaches, but I believe
the first is a much cleaner design.
1) factor out the proposal collection algorithm into its own class:
class HippieCollector {
String[] collectSuffices(String prefix, IDocument document, int offset);
String[] collectSuffices(String prefix, IDocument[] documents);
HippieCollector() {}
}
-> HippieCompleteAction focuses on state and lifecycle management, but
instantiates a collector to actually collect its proposals. Testing the
collector it is easy, as there are no editor dependencies.
2) make the public methods non-static (there are no real reason for them to be
static, are there?) and private, use the Accessor you mentioned to test them.
Created attachment 17310 [details]
Refactored patch (both action and tests)
I hope I didn't miss anything.
Genady
fixed, available > 20050120
Genady, thanks for your contribution!
Before committing, I applied the following modifications:
- added captions for the action - just in case we ever want to add the action to
the UI (toolbar, menu...).
- added your name to the header of all Java source files where you contributed
- formatted using tabs, rewrapped javadoc comments
HippieCompletionEngine:
- made the class final
- added ctor
- used super types where possible (List instead of ArrayList, CharSequence
instead of String)
- renamed getCompletionsForward(IDocument, int) -> getCompletions(IDocument, int)
- Test and Action now have a field with the engine
- all engine methods non-static
[ Why don't I like static methods? - well, imagine you want to add some
configuration later, e.g. configure the regex pattern used by the engine. If
everything is static, you will have to add a parameter to all methods. The way
it is now, you can simply add a setter or another constructor.
I guess we don't follow that rule everywhere in text-land, but we're always
picky with contributions ;-) ]
---
> I consider it almost final, since I have no good ideas of what to do about the
> open issues.
Oh, now before you leave for the beach... I have all kinds of ideas! For
example, the hippie-completions could be made to also appear as completion
proposals. We currently don't have content assist in the text editor, but the
infrastructure is there and we should have it anyway to support spelling.
Thanks for your attention. I'm glad it's finally in :) I will get back to look at various improvements after some user comments arrive (after M5). Genady Great feature, but since I work heavily with XML based documents (ant scripts, xslt stylesheets, etc.), I often have to type hyphenated or dotted "identifiers" such as "this-is-an-ant-target-name" or "very.nice.property". The hippie completion, in this case, treats hyphens and dots as "stopchars", and it is usually a pain to search for the right identifier with "hippie completion" in these cases. It would be a real time saver to have an option in the preference page to make hyphens and dots (and maybe other characters) be parsed as being part of the candidate completion proposals. Thanks |