Community
Participate
Working Groups
The find/replace dialog should be adapted to use the new fieldassist support in JFace as a proof of concept of both the API and the function.
Created attachment 31527 [details] patch to find/replace dialog Quick and dirty adaptation of Find/Replace dialog to use new field assist code
The attached patch adapts the find/replace dialog to use the new field assist support. It is a quick and dirty proof of concept, not the suggested implementation. The approach was to take the function defined in the current RegExContentAssistProcessor and move to a new class, RegExContentProposalProvider. This contains the "meat" of how proposals are described and accepted. Then, the old content assist classes were replaced a ContentProposalAdapter. Since decorations are separated from content proposal, the find/replace fields were replaced with DecoratedFields. The client code in the dialog is what I would expect to see, except that the cue image is grabbed from AbstractControlContentAssistSubjectAdapter. I would expect that as we evolve platform ui dialogs to use content assist, we'll provide this image as a workbench image. Likewise, the code to grab the invoking keystroke will likely be provided by the workbench. I believe that the behavior is similar enough to compare, although not identical. Per the comments in ContentProposalPopup: * For now, there is limited API to control how the popup is sized or * placed relative to the control, or whether it is resizable. The intention is * that standard techniques will be implemented to "do the right thing" * depending on the associated control's size/location and the content size, * and API added if we discover there are additional choices to be made. Noticeable differences: - size of the popup. - I do not provide API for setting its color, so it is the system info color - auto-activation is not on a delay. It could be, and again, we could make it always be on delay, or settable. - the new code inserts proposals such that any selected text is replaced. The old code ignored any selections. - the new code filters the proposals with a cumulative filter (typing successive characters further filters the proposals.) Using backspace unwinds the filter. I think this adaptation shows that the new API supports the scenario. I'd like to evolve the code to meet your requirements for addressing differences, and, where possible, "fix" my code to do the right thing wrt placement, colors, etc. rather than introduce a bunch of API.
cc'ing Dani and others potential clients, since I expect function/API discussions to happen here, and I'd like to get comments from potential users.
Moving this bug to Dani so he can look at what I did. Dani - it is not critical for M4 to look at this, but we must agree on the API by M5. I assume we'll ping-pong this bug back and forth a bit during M5.
The goal should be that Platform Text can deprecate its 'org.eclipse.jface.contentassist' package and tell clients to use the new field assist support.
The new field assist framework with its API looks promising and is quite flexible, especially regarding the field decorations. However, in order to be widely used by new clients and adopted by existing clients of the Platform Text content assist APIs it has to offer the same level of abstraction as the current Platform Text support does: the submitted patch inlines stuff (e.g. cue image) from the Platform Text field content assist code and codes the key binding stuff by its own. In order to get the same ease of use for clients plus consistent LAF inside the Eclipse SDK there needs to be a place in Platform UI that does the setup of the image and key sequence, similar to the code in org.eclipse.ui.contentassist.ContentAssistHandler otherwise clients will simply continue to use the existing support in Platform Text. Detail questions: - when typing in the field while the assist popup is open, the text is not inserted into the field, is this per design? - why did the layout of the Find/Replace dialog change with the patch
Dani - can you clarify your first comments? I'm not sure what image code you are referring to in ContentAssistHandler (I see the image definition in AbstractControlContentAssistSubjectAdapter). If your point is that there should be a common place to retrieve a cue image, I agree. I also agree there needs to be a workbench-level place to establish the key sequence (see also bug #120408). Specific answers: - you can configure whether the content proposal adapter propagates keys to the subject control once the popup is open. I set it up to not propagate, since the proposal popup supports filtering, but I see now that the original implementation propagates keystrokes. This can be configured in the constructor for the ContentProposalAdapter. - the change in layout was the result of bug #124202 in the decorated field code. If you load the latest, you should note that the layout doesn't change significantly
Yes, my point is that the Find/Replace dialog now has to manage and provide the cue image and create the code to setup the keybindings. The only thing that he currently needs to do (and must be possible with the new approach) is - provide the completions (IContentProposalProvider) - tell which field needs content assist - do some minor configuration - enable/disable the code assist (handler) ==> image and key binding support are setup for me behind the scenes >I'm not sure what image code you are referring to in ContentAssistHandler The handler does not contain the image (code), it easily allows to enable and disable the content assist by (de-)installing the cue and keybinding support. My two detail questions are resolved - thanks.
Some differences I noticed (using N20060118-0010 plus patch in this PR): - when moving the Find/Replace dialog, content assist stays at the initial position instead of being dismissed. - clicking back into text editor leaves the content assist open - resizing is not persisted - afair the previous version, the initial size seems smaller now in N20060118-0010
>- when moving the Find/Replace dialog, content assist stays at the initial > position instead of being dismissed. >- clicking back into text editor leaves the content assist open these issues are tracked in bug #120457. I haven't wanted to tackle this until the various Popup dismissal bugs were fixed, so this is next on my list for field assist. >- resizing is not persisted Is this a hard requirement? I notice that it causes problems in the text implementation with the positioning of the secondary popup (which doesn't track it). I'm not sure this should be done at the level of the content proposal popup but if it is a requirement I will make sure the API allows you to do so >- afair the previous version, the initial size seems smaller now in N20060118-0010 I observe that the patched version of the dialog is now the same size as the original. The combos themselves are a bit smaller because the decorated field reserves space on each side for decorations, so it's no longer necessary for the client to do so.
>Is this a hard requirement? There should at least be an option to easily enable it. Again, this is need in order to convince people to use the new field assist support instead of the one in Platform Text which I'd like to deprecate in favor of the field assist support. Regarding the key binding and image: easiest would be if the client could setup those through a command ID and a constant for the image. The two don't need to be tied together i.e. can be provided by different classes.
People who want to port their usages of org.eclipse.jface.contentassist to org.eclipse.jface.fieldassist and have to deal with CellEditors might also want to have a look at bug 124873.
I've opened bug #125587 to track the issue of getting the image. Bug #120408 still tracks the use of a command id to trigger content assist. So this bug is still open to track general integration issues/bugs. I'll be attaching a set of patches shortly that utilize the new WorkbenchContentAssistAdapter, etc.
Created attachment 33751 [details] org.eclipse.ui.workbench.texteditor patch New version of the FindReplaceDialog and the removal of the content assist command definitions from the plugin
Created attachment 33752 [details] org.eclipse.ui patch addition of the content assist commands to the org.eclipse.ui plugin
Dani and Markus - please have a look at the latest adaptation. I think it addresses the issues of command bindings and a common image. Some of the underpinnings (see bug #124520) may change, but I think the API to the content adapter itself and retrieval of decorations should stay the same. We will need to coordinate the removal of the content assist command and key definitions from org.eclipse.ui.workbench.texteditor and the addition into org.eclipse.ui. If defined in both places, content assist is not triggered in the FindReplaceDialog (or in the new class wizard, etc.). I can release them first on my end, but will wait for your go-ahead that you intend to delete them soon after. I'd like to track remaining bugs and issues separately if possible. I have open bugs for resize (#125600) and for the dismissal of the popup (#120457).
I applied the patches (including org.eclipse.ui) but there seems to be a major problem with the key binding stuff (using N20060129-0010): 1. open Find/Replace 2. make sure regular expression search is enabled 3. [optional] press Ctrl+Space in the Find field (==> works) 4. close Find/Replace 5. open again 6. press Ctrl+Space in the Find field ==> no content assist, inserts spaces, nothing in .log Regarding the o.e.u.w.texteditor patch itself: I still find it cumbersome that every client needs to handle the FieldDecoration (addFieldDecoration,show/hide). This should be done for me? The WorkbenchContentAssistAdapter already knows the decorated field and should therefore do this for me under the hood. >I'd like to track remaining bugs and issues separately if possible. Do you want separate bugs for above two issue? For me those are not "remaining bugs" but crucial in order to release the patches. Minor: the o.e.u.w.texteditor patch should remove no longer used code like classes and property file entries.
Created attachment 33781 [details] org.eclipse.ui.workbench.texteditor patch This is an improved version that - removes auto-activation trigger characters from the proposal provider where it does not belong (inlined the code into FindRepalceDialog) - adds comment to ITextEditorActionDefinitionIds about code assist command now being provided by o.e.ui - removes unused code - applies Platform Text coding style
I agree that the problem with the binding is critical to integration and should remain in this bug. Will investigate.
didn't mean to ignore your second comment. I've gone back and forth on the issue of keeping decorations separate. It's possible (see bug #124873) that there will be different ways in the future to combine content assist and show a cue. I'm leaning toward a solution that provides a generic Workbench-level adapter that manages only the key binding issue, and a more specific subclass that configures the decorated field as expected. My next set of patches (based on your latest) will address this issue as well as the binding problem.
Created attachment 33837 [details] org.eclipse.ui.workbench.texteditor.patch This patch is based on Dani's changes. It matches the latest org.eclipse.ui.fieldassist in HEAD. I've created a "ContentAssistField" that manages the decorations and the adapter. It cleaned things up a bit. I renamed "WorkbenchContentAssistAdapter" to "ContentAssistCommandAdapter" since the important part is the mapping of a command binding to invoke content assist. The bug with the binding not working after subsequent invocations is fixed. (fyi, the problem did not appear if you closed the dialog with "Close," but only if Esc or the shell close box was used.)
Hi Susan, I like the new code now. It's nice and offers a good migration path for clients for the Platform Text API. Remaining stoppers I found are: - the initial size is by far too small (I'd even say useless, at least for the existing places where we use field code assist) which means all clients moving to the field assist API will get code assist which is too small. This is a problem since we also have resize bug 125600. Is there API that would allow clients to specify the initial size? - if I enter characters so that the list gets empty and continue typing then the code assist remains open and "eats" the characters (they don't appear in the field). The user has then to delete all those "eaten" characters until the popup again contains values. Such hidden character behavior is not easy to understand for the user. - if I continue typing the Backspace key after above scenario I get a StringIndexOutOfBoundsException each time (see attached log.txt). Same happens if I open code assist, enter 'a', type Backspace more than once. - open code assist for 'replace' field, decide to edit 'find' field by clicking into it ==> code assist stays open and when hitting 'Ctrl+Space' I have two code assists popups. - open code assist and wait for the hover then switch to another application ==> hover stay on top of this application. Minor (not stopping): - bug 125808: [Field Assist] assist hint hover should use tool tip caption
Created attachment 33858 [details] log.txt
I've opened two new bugs for the SIOOB and the cumulative filtering confusion. I'll consider the other issues as part of the existing bugs (the resizing bug/size API and the popup not dismissing bug). All are now listed here as dependencies. I've cc'ed you on those bugs in case there's any further discussion, as I think there are some API things to iron out. The bugs just need to get fixed. Thanks for spending the time with it, I appreciate the extra testing.
>Thanks for spending the time with it, I appreciate the extra testing. np.
Bug 120237 is about persistent resize and API to set the initial size (good). What about the initial size when the new API isn't used? Currently the code assist pop-up is much smaller. I'm asking because even with the new API clients of the Platform Text API would have to 1. find out which size it currently has 2. use the API to set the size
The default is currently to the size of the field. I don't mind a change in this value, but am looking for suggestions...Any comments can be made in bug #125600.
Created attachment 34154 [details] org.eclipse.ui.workbench.texteditor patch latest adaptation of dialog to match current state in HEAD. Many constructor parameters moved to getter/setter to unclutter constructor. Also includes sizing API.
Latest patch does not work against latest code (N2060206-0010) due to bug 126044, see bug 126044, comment 16 for details.
Found bug 126555 while playing with this.
I've released the 'org.eclipse.ui' patch and the changes to plugin.xml and plugin.properties in the 'org.eclipse.ui.workbench.texteditor' patch.
Susan, I played around a bit in I20060208-0848 with the patch from comment 28. Apart from bug 127080, I found some problems with replacements and caret positioning (check the current Find/Replace dialog for the expected behavior). - type "abc", set caret to after "a", use content assist to insert \t was: every way I tried to insert the proposal, some of the surrounding text was lost expected: text is "a\tbc", regardless of way to insert proposals scenarios: - Ctrl+Space, doubleclick with mouse to insert proposal - Ctrl+Space, use cursor keys and enter key to insert - Ctrl+Space, type "\", then use mouse to insert - Ctrl+Space, type "\", then use keyboard to insert A bit more tricky are proposals that set the caret to somewhere inside the inserted text: - type "(?" was: popup comes up in default order expected: proposals starting with "(?" are first The relevance sorting seems to work after typing one more character and then typing backspace. - in "abc" with caret after "a", insert proposal (?<=) expected: text is "a(?<=)bc", caret is after "=" was: surrounding text deleted, only parts of proposal inserted I've not dived deeply into the patch, but you have to make sure that all these scenarios are (easily) implementable for clients of the fieldassist framework.
Markus, thanks for spending the time with this. There is a known timing problem right now triggered by some work done in bug #125952. In the test app for field assist, the filtering of content proposals was done by the adapter itself. In bug #125952, the API was extended to allow different kinds of filtering (cumulative, character, or client). With client filtering, the proposals are reobtained from the proposal provider as keys are typed. The problem is that the control contents are not updated yet as we are in the middle of the key event. I opened bug #127108 to track that issue specifically and have added your observations to that one. I don't believe this to be an API issue, but just a serious bug right now in event flow. It will be addressed as soon as we've vetted out the remaining API issues.
The API portion of this bug has been released for M5 i.e. I've deprecated the 'org.eclipse.jface.contentassist' package. Due to some remaining bugs in the field assist code we will continue to use the deprecated code and switch during M6. TO DO: must add migration help to the deprecated code.
Created attachment 35572 [details] org.eclipse.ui.workbench.texteditor.patch Attaching the latest patch of the dialog that matches the M5 API. Doing this for my Mac/Linux testing, there's nothing for Platform Text to do yet until I've finished with bug #126138.
Created attachment 35657 [details] org.eclipse.ui.workbench.patch Latest patch that addresses all open problems
I've fixed all the bugs that were listed as dependencies, but have added a new one. We are about ready for prime time. The remaining issue is an insertion position problem in combo boxes when the proposal is selected by double click instead of the enter key. That problem is tracked in bug #127108. It may be an SWT issue, I am investigating.
Sorry but the latest patch (applied against latest I-build) does not work: the content assist cue is a red square.
NOTE: I expected that the bugs recently marked as fixed wouldn't be fixed yet when using the I-build but I thought the image issue had been fixed a while ago.
hmmm. The images should have been working in the latest I-build, although they moved to a different plug-in (available via JFace API) after the I-build. Since the images moved, if you've got any of org.eclipse.jface, org.eclipse.ui, org.eclipse.ui.workbench from HEAD in your workspace, you will need all three of them. It sounds like you may have a newer workbench and older JFace or else an older workbench and newer org.eclipse.ui. All my testing was done with all three of those from HEAD as I was fixing the other bugs.
Created attachment 35919 [details] org.eclipse.ui.workbench.texteditor.patch These are the latest changes, and to my knowledge the find/replace content assist is now behaving compatibly with the currently released implementation. Bug #127108 describes some of the fringe cases that were tested. The primary change in the RegExContentProposalProvider is that the proposal provider must alter the desired insertion position when it detects that part of the proposal has already been typed into the control. This was not always true in the previous implementation (or in the old RegExContentAssistProcessor). I believe this adaptation to be good and I've tested all the known problems as well as fringe cases that I can ascertain from reading the RegEx proposal source. However someone on the text side should give it a run through in case there are other fringe cases I'm not aware of.
>find/replace content >assist is now behaving compatibly with the currently released implementation. I don't think so (applied patch against I20060314-1200): 1. no auto-delay for activation (even though bug 120458 is marked as fixed). This is really a paing for users that simply want to type in the regex. They now get a code assist popup in their face. 2. no auto-delay for showing additional info. It should at least use a post-selection changed listener and not constantly update the additional info hover while pressing e.g. the down arrow key. 3. can't enter '\' after having deleted it once using Backspace. Note sure whether this makes a difference: '\' is AltGr+'<' (or Ctrl+Alt+'<' on my Swiss German keyboard. Nothing in .log At least 1 and 3 are stoppers for releasing the patch.
>1. no auto-delay for activation (even though bug 120458 is marked as fixed). > This is really a paing for users that simply want to type in > the regex. They now get a code assist popup in their face. Bug #120458 added the support for auto-activation delay in the API and was fixed from a framework point of view. It's reasonable that ContentAssistCommandAdapter sets this value to a value similar to that in org.eclipse.jface.text.contentassist.ContentAssistant. Fixed in bug #132038. >2. no auto-delay for showing additional info. It should at least use a > post-selection changed listener and not constantly update the additional > info hover while pressing e.g. the down arrow key. opened bug #132039 >3. can't enter '\' after having deleted it once using Backspace. Note sure > whether this makes a difference: '\' is AltGr+'<' (or Ctrl+Alt+'<' on my > Swiss German keyboard. Nothing in .log What do you mean by can't enter? That it doesn't propagate to the popup or that it doesn't type into the field at all? Does this happen when content assist is open or anytime after you've backspaced over the '\' character? Can you backspace other characters and it works okay? I don't have any trouble on a US English keyboard with '\', so I'm not sure how to replicate and therefore fix this. Any further detail you can provide would be helpful.
Sorry for not being more clear in comment 42. See bug 132101 for case 3 from comment 42: it's not depending on pressing 'Backspace' but in fact it's impossible to enter any character that needs 'AltGr' into the field once the code assist popup is up.
The stopper bugs are fixed. I'm hoping this can be released and we can get some user feedback about the secondary popup delay (or lack thereof). I don't find it a problem, so I'd like to get more input or the history behind it. We can move that discussion to bug #132039.
Patch released to HEAD. Available in builds > I20060321-0800.
