Community
Participate
Working Groups
I20060119-0800 I found three impedance mismatches when a CellEditor, a DecoratedField, and a ContentProposalAdapter should work together. (1) Concrete CellEditors like TextCellEditor do not expose the control they use for editing to clients. Therefore, clients would have to guess and try to cast the CellEditor#getControl() to Text to be able to pass a working IControlContentAdapter to the ContentProposalAdapter. (2) Furthermore, I'm not sure whether the current support for CellEditors in TableViewer and TreeViewer works at all with a DecoratedField composite in between the parent (Table/Tree) and the CellEditor control (Text/Combo/...). I think this will cause problems with focus change and other events. (3) Currently, the CellEditors are designed to be placed exactly above a TableItem, such that when the CellEditor is activated, the editable field control appears at the same position as the text that was rendered as item label. Since a DecoratedField reserves space at the left of the field control, a CellEditor with content assist and lightbulb would cause the text to jump to the right when the CellEditor is activated. This would be rather disturbing. The only general solution I see for this problem is to do without an intermediary composite and render FieldDecorations in a separate Shell with NO_TRIM | ON_TOP | NO_FOCUS that is placed just to the left of the decorated field. One interesting application of constent assist in cell editors is the Change Method Signature parameters table (ChangeParametersControl#addCellEditors()). Note that we had to add some non-trivial workarounds there to overcome limitations of the existing CellEditor and TableViewer implementations. I remember having seen other products using content assist in the Properties view, which would suffer from the same problems if they converted to the FieldDecorators. I know I'm a bit late with these observations, but I think we should have a story for this interplay before the API freeze.
Thanks, Markus. I agree we need to discuss now and form a story before API freeze. I prefer to separate the issues of the content proposal adapter and the decoration itself. I implemented these separately under the assumption that the use of content assist does not assume the presence of a cue or any particular cue implementation. We need to get a story right away for how an IControlContentAdapter can be used with a cell editor. It is totally appropriate to use a ContentProposalAdapter for cell editors and property pages, so this needs to be resolved. The field decoration implementation always assumed it was decorating a dialog or form field (it arose out of the need for general decorations such as required fields, etc., not just content assist). I think we should try to share the concept of the FieldDecoration, so that the workbench can define a FieldDecoration for content assist and have it used consistently. Fortunately, the definition for a field decoration has always been independent of the specification and implementation for where it appears and how it's rendered. In dialogs, forms, etc., clients can use a DecoratedField to render this decoration. It sounds like we need an analogous implementation for the cell editor case where the decoration is rendered differently. It is transient and preserves the exact positioning of the control, etc. We have two issues we need to resolve rather quickly. Cc'ing Eric and Boris for comments: 1) can cell editors implement IControlContentAdapter so that they could be passed directly to the ContentProposalAdapter? 2) how/where/who should implement the rendering of a FieldDecoration for cell editors? I think it's best done as a separate class in the cell editor world that renders a FieldDecoration for a cell editor. 2a) It might be appropriate to rename FieldDecoration to ControlDecoration to reinforce the notion of one generic decoration for controls that can be rendered differently for fields, cells, ??? Also cc'ing MVM and Tod since this discussion crosses several boundaries in JFace.
>I implemented these separately under the assumption >that the use of content assist does not assume the presence of a cue or any >particular cue implementation. Correct. The concept should be separate be there should be a helper/adapter that connects decoration and content assist with a widget. >1) can cell editors implement IControlContentAdapter... I wouldn't code this into 'CellEditor' if possible, but there are definitely changes to the cell editors needed: - allow clients to setup their (Text) field and use the corresponding IControlContentAdapter - this means that at the cell editors must provide API to get the control, e.g. getText() on TextCellEditor - interaction as outlined by Markus needs to be addressed, e.g. cell editor being deactivated when the focus switches to the code assist pop-up (see bug 58777). >2) how/where/who should implement the rendering of a FieldDecoration You could offer different rendering policies (decorators), e.g. the one you already have for fields and another which works like the one we currently use (which does not require an additional control). This would also allow text content assist clients to use the rendering which doesn't need an additional control (e.g. to ease the layouting pain when having several fields). >2a) Naming it ControlDecoration sounds a bit too technical to me. I'd either leave it as is or simply use Decoration or Decorator.
Agree that direct rendering saves a lot of hassle, but has its own problems as can be seen in the change parameters table (cue is cut off even when it is made so small). Putting the decoration in its own shell will serve to introduce a whole new set of focus issues. I'm not willing to take that one on given the different event ordering/flow across platforms. The big question here is whether platform UI is biting this off for 3.2. Something will have to slip for this to get done...will investigate and report back here.
Per discussions in the platform UI team...we'll shift some other work and provide a DecoratedTextCellEditor that can use DecoratedField. Trying to get away without explicitly reserving space for the decoration will just keep causing cut off issues/layout issues, so we'll address it with a specialized cell editor. >This would also allow text >content assist clients to use the rendering which doesn't need an additional >control (e.g. to ease the layouting pain when having several fields). Bottom line is that if a client wants to mix fields with cues with some that do not have cues, they have to know about the space and reserve it. This gives us future flexiblity with cue sizes, locations, etc.
(In reply to comment #4) > Per discussions in the platform UI team...we'll shift some other work and > provide a DecoratedTextCellEditor that can use DecoratedField. Note that if this DecoratedTextCellEditor should support content assist, it would need bug 58777 fixed, or work around those issues like our TableTextCellEditor. > Trying to get away without explicitly reserving space for the decoration will > just keep causing cut off issues/layout issues, so we'll address it with a > specialized cell editor. And where will this DecoratedTextCellEditor position its Text field? Will the text jump to the right by the width of the left-hand decorations, as soon as the cell editor is activated? I fear this will lead to rather "bumpy" user interactions, especially for people using the mouse to select something.
For a first try, I plan to try the cue on the left with the text moving first. This is the easiest to try since it uses decorated field as is. If it looks too jumpy, we'll regroup. There is going to have to be some tradeoff here... Thanks for the heads up on focus issues. At first glance, I plan to work around these issues (to avoid a bunch of side effects that fixing bug #58777 may cause), but I'll have to get further into this before getting a feel for what might be right.
Created attachment 34176 [details] NewTypeWizardPage.patch super-hacked NewTypeWizardPage - EXPERIMENT
Markus & Dani - Attached is a hacked NewTypeWizardPage.patch that uses a new type of cell editor, "ContentAssistCellEditor." This cell editor is implemented using a decorated field. I've barely worked on the cell editor, so the focus issues, etc. are not addressed and content assist is not even being invoked yet. My purpose for releasing the cell editors and attaching this patch is to let you see what it looks like. Please try it and comment on the look/behavior of the decoration. I chose the NewTypeWizard/subinterfaces table (instead of the change method signature parameter table) because it demonstrates your implementation at its best (vs. the table with the cut off light bulb). I thought it best to compare against your best case. The text definitely "jumps" a little more horizontally than it does in your implementation. Side by side comparing, I notice the difference, but both have a jump and flicker. - text jumps up vertically in both cases - selection flickers before the cursor is selected in both cases - text jumps horizontally a pixel or two in your implementation, and a few extra pixels in my implementation. I don't see the extra pixels of horizontal jump as a "deal-breaker." I realize that it's nice the way the bulb gets superimposed on the interface icon in your implementation. But in cases that don't look as nice (cut off cue), I think the jump is better than half a light bulb. At least it will be consistently placed. What do you think? If you think you (and your clients) can live with the few extra pixels of shift, then I will proceed on making the cell editor real. At a minimum the API will be settled for M5 and working on windows. Then it will be fleshed out/bugs fixed for M6.
It looks quite good to me. The only thing which I don't like too much is that the caret is positioned at the wrong location when using the mouse (move mouse somewhere over the text of an inactive cell, then click to activate, click to set caret). But I doubt we can do anything about this. Auto-placing the caret on the second click wouldn't be nice and adding an additional column for the image is complicated as well. And as you already pointed out it irons out some problems we have with our implementation like cutting off the image. The code for clients looks good in fact it's getting simpler for them. The light bulb image looks ugly (wider than normal). I filed bug 126553 for that. >- text jumps up vertically in both cases Yes, that's bad in both implementations - probably a cell editor bug. Having a fix for that in the future would be great. Markus, did you already file a bug report for this?
I also tried the preview on Dani's machine, and I found that the horizontal jumping is way less disturbing than I thought. Since it would solve the clipping and positioning issues, I guess this is the way to go for content assist. The vertical pixel-jumping of cell editors has been there for ages, but I'm not aware of a bug for this. I always assumed this was "by design" to give the user feedback that editing started. However, I agree that it would be nicer without. A side issue: With this approach, the decorations could only be shown when the CellEditor is up. If DecoratedFields should be used to implement bug 120238 (indicate required fields / errors), then we need another mechanism to mark those. But I think this can be dealt with later.
>The only thing which I don't like too much is that >the caret is positioned at the wrong location when using the mouse (move mouse >somewhere over the text of an inactive cell, then click to activate, click to >set caret).. The caret positioning at the end of the text is the same used by the current platform text content assist cell editor, so I just implemented mine to be consistent with it. I think it's better than the default cell editor, which selects everything also. >The vertical pixel-jumping of cell editors has been there for ages, but I'm >not aware of a bug for this. I always assumed this was "by design" to give >the user feedback that editing started. However, I agree that it would be >nicer without. I've always assumed the vertical jump was "by accident" in the sense that the editing control is positioned to fill the cell and this naturally causes a jump based on the margins, etc. used by the platform controls. That and the selection flicker seem natural side effects of the implementation and I think those should be addressed independently. I'm not planning on opening a bug for these at this time. >A side issue: With this approach, the decorations could only be shown when the >CellEditor is up. If DecoratedFields should be used to implement bug 120238 >(indicate required fields / errors), then we need another mechanism to mark >those. But I think this can be dealt with later. When a decoration is added to a field, the client can specify whether the decoration only shows on focus or all the time. They can be mixed and matched. I have an example that does this...some decorations show all the time, but the content assist cue only shows when there's focus. I think we have converged on an approach, so I will continue down this path fixing the new and old bugs.
Dani and/or Markus - I've got the cell editor working on Windows (have not tested other platforms given the need to push on API issues first). Note that in the hacked NewTypeWizardPatch, I just hardcoded the content proposal values to strings. Does Markus (or anyone else familiar with the JDT side of this) have time to try to make the Java Type Processor behave like an IContentProposalProvider (along the lines of what I did with the RegExContentAssistProcessor becoming RegExContentProposalProvider). I suspect there might still be API issues that will surface in trying this. For example, the fact that content assist is not valid for certain cursor locations, hooking up the right label provider to provide the icons, etc. I need to focus on some unrelated API stuff for a couple of days and I'm not so familiar with all the JDT content assist code. If someone had a few hours to try this experiment, it would help flesh out any remaining API deficiencies. Otherwise I'll try to get to it near the end of the week.
>The caret positioning at the end of the text is the same used by the current >platform text content assist cell editor, so I just implemented mine to be >consistent with it. I think it's better than the default cell editor, which >selects everything also. Mmh, my test steps from comment 14: >(move mouse >somewhere over the text of an inactive cell, then click to activate, click to >set caret). were probably not good enough, so let's try again: 1. use NewTypeWizardPage and make sure the table has several 2. select a table line 3. move the mouse in the middle of another table line and place the mouse cursor exactly between two characters 4. press left mouse button once ==> line gets selected and caret at the end (OK - same as Platform Text) 5. press left mouse button again once ==> NOT OK: since the line was shifted to the right the caret is now at a different location than I pointed at in step 3. In the Platform Text implementation the caret is set where expected > I'm not planning on opening a bug for these at this time. Filed bug 126695 against CellEditor.
>5. press left mouse button again once > ==> NOT OK: since the line was shifted to the right the caret is now at a > different location than I pointed at in step 3. In the Platform Text > implementation the caret is set where expected I think the difference is a matter of timing of the events as well as degree of the shift. In both cases, suppose you click to edit a cell where the text is longer than the column. The text is going to shift dramatically when the editor opens. Opened bug #126758 for this issue. I don't see this as a stopper, but let me know if you disagree. I think the fix would be along the lines of not reacting to that second click, but I will have to investigate. In the platform text case, a fast second click is interpreted as a selection and the editor disappears, so you don't have the caret problem, even when the text shifts, but neither do you have an editor anymore. In the new code, a fast second click is passed into the editor, and if you click fast enough, the caret is set in the wrong place. In both cases, if the second click is slower (probably past the double click time, but I didn't check that this is the issue), and the editor "sets" itself, then the caret goes where expected. Opened bug
>I don't see this as a stopper, but let me know if you disagree. No, I fully agree.
Created attachment 34313 [details] Proof of concept with decoration in shell We discussed the cell editors issue a bit in our lab, and already got opposition against the "jumping" cell editors. Here's a proof-of-concept SWT snippet that shows how the alternative approach with flying decoration shells could work. To run it, you need to copy org.eclipse.jface.contentassist.images/content_assist_cue.gif from org.eclipse.jface.text to the same package as the snippet. I think having such a HoverFieldDecorator as an alternative to DecoratedField would be more valuable to the general public than a special DecoratedTextCellEditor.
I haven't run the snippet yet, but glanced at the code. I'm sure it looks nice, but I don't think it's a good proof of concept without showing that the hover over the decoration, and the content assist popups (along with secondary popups) can be invoked on all platforms without auto-dismissal/shell activate/focus issues. NOFOCUS is a hint, and even with NOFOCUS widgets, the shell activate/deactivates still fire, and in different orders on the different platforms. I'm very leary of this, although I agree it would be nice if it worked. We already have two content assist popups and the hover shell at play here. No promises at this time as we are running out of runway on M5. If you feel strongly, can you take another crack at it and use it inside the change signature parameters table or inside the new interface wizard. If it seems to work on windows and at least one of Mac or Linux without focus issues, then I'll take a harder look at it.
Just ran it. Definitely looks promising (though I assume you would want the hover to look more like the current jface content assist/decorated text hover rather than the tooltip below the cursor). I still think it needs to invoke the content assist popup to be a proof of concept, and checked on all platforms with scenarios such as: - click on the decoration - click onto another app and make sure all shells go away - decoration should only show when field has focus - etc. If you can show that it "mostly" works, I'd be willing to take on working through platform bugs, etc.
Markus & Dani - I've been thinking about this issue and how we can defer the debate on the best implementation of a decorated cell. Since the goal of M5 is API freeze, I'm wondering if we can agree for now that a CellEditor that manages content assist and its cue is a good thing. I propose that I move DecoratedTextCellEditor from JFace API to org.eclipse.ui,internal.fieldassist. This way, the only API exposed is org.eclipse.ui.fieldassist.ContentAssistCellEditor. Regardless of the implementation of the decoration, there will be a need for a higher level construct that manages the cue, the adapter, etc. (Much like ContentAssistField is used in the find/replace dialog vs. DecoratedField). This leaves us time to evolve the implementation in M6. If we decide to implement the cue as a floating decoration in ashell, it won't affect API. If we later decide that this floating decoration should be made API, that can be a separate decision. This seems a safer choice than trying to rush any new API for now. Thoughts? If you agree, then I will move DecoratedTextCellEditor. However, I still have the concerns voiced in comment #12. That is, I think hooking up the real Java Type proposals will flesh out any remaining API issues with the ContentAssistCellEditor (and more particularly, with related interfaces such as IControlContentAdapter, etc.)
That's fine with me. Since we are currently the only clients waiting for code assist in cell editors I think you could still move it back into the API zone for M6, but don't know how strict Platform UI is on this one.
I agree with moving DecoratedTextCellEditor to internal, but I would move ContentAssistCellEditor as well. Note that Text is not the only kind of "field", and it seems strange that the general ContentAssistCellEditor extends only DecoratedTextCellEditor but does not offer e.g. Combo or StyledText as alternatives. I started with porting the Java Type Processor, but I got stuck with the same problems as listed in bug 120237 comment 32. Unfortunately, I don't think I'll find much time before M5 to look at this again. Notable differences I found that are available with ICompletionProposal and its extensions, but not in IContentProposal are: - ICompletionProposalExtension#getTriggerCharacters() e.g. allows to type in "java.l", press Ctrl+Space, press ArrowDown to select "java.lang.ref", type "." to complete to "java.lang.ref." (including dot) - preferences of java editor content assist (insert single proposal automatically, insert common prefix automatically), further controlled with queries from ICompletionProposal[3,4]
One more: Unlike ContentAssistField, the ContentAssistCellEditor does not give access to its ContentAssistCommandAdapter. Thus, it is impossible for clients to add their own label provider and do other configurations.
Markus, you make good points. I propose the following for M5: 1 - make both cell editors internal. It will be easier to add new API in M6 then it will be to keep iterating it. This lets us continue to iterate and then move to a public package when we feel it is solid. 2 - wrt cell editors: you are right about the API mismatch between the cell editor world and the DecoratedField world. It would seem to make more sense that a "DecoratedFieldCellEditor" would take an IControlCreator, IControlContentAdapter, etc. rather than have different cell editors for different controls. This makes sense and I think is the direction to go in. 3 - implementation of the decoration itself is still an open issue as described 4 - ContentAssistCellEditor should give access to its adapter via API As far as the issues with ICompletionProposal. I'm not trying to replace full on content assist in editors, so I'm not so worried about java editor preferences. For this reason, I've never intended to be fully compatible with ICompletionProposal[N] interfaces. The issue is knowing how much of those interfaces are crucial to clients of org.eclipse.jface.contentassist. Do you have a feel for this?
1 - I fully agree 2,3 - I think in M6, we should explore the possiblities of alternative decorations that would not require an intermediate Composite like DecoratedFields do. That would make the impedance mismatch between IControlCreator and subclasses of CellEditor go away and would also help clients who can't or don't want to constrain all the other widgets in their dialogs to align with decorations at left as well as at right. Compatibility with ICompletionProposal: If the insertion issues can be resolved, then I think the basic infrastructure is now fine for common use cases. However, I think clients *will* eventually expect full support for the prefix completion and trigger characters as they have in the editor, since entering an element name in a dialog is essentially the same as entering it in the editor. But I agree that this is not too urgent and can also be added later if pressure for it rises.
Agree. fyi, right now trigger characters can be specified at the adapter level. We can let this play out with clients and if it's inconvenient/misplaced. I'm reassigning this bug report to M6 since we are agreeing to defer exposing any API. I'm not so sure the composite will go away. It may come in handy later for visuals such as colored borders, etc. But we can worry about that later.
Created attachment 34554 [details] NewTypeWizardPage.patch replacement patch - due to moved classes.
(In reply to comment #25) > fyi, right now trigger characters can be specified at the adapter level. We > can let this play out with clients and if it's inconvenient/misplaced. Hm, IContentAssistProcessor#getCompletionProposalAutoActivationCharacters() corresponds to ContentProposalAdapter#setAutoActivationCharacters(..). But there's also ICompletionProposalExtension#getTriggerCharacters(), which are characters that trigger the *insertion* of a selected proposal (and not the activation of the popup), which what I tried to describe in comment 21. AFAICS, these insertion triggers are currently not supported by fieldassist.
We need to figure out what to do for M6. I just sat down with Kim Peter to show her: 1 - the current implementation inside the new interface wizard (where the light bulb overlays the interface icon) 2 - the current implementation inside the change signature parameters list (where the light bulb gets cut off) 3 - the decorated field implementation of the cell editor (where the light bulb shifts the text) 4 - the flying decorator sample Kim's comments: - doesn't like the position of the light bulb on the interface overlay in case #1, but can live with it - doesn't like the clipping in case #2 - the decorated field sample is significantly better than clipping, and somewhat better than the overlay. It might be good enough. - thinks that a floating decorator might need an additional affordance (such as a bubble, etc.) so that it doesn't get "lost." A floating decorator still has the problem whereby its placed without knowledge of its adjacent content, so it can still show up in a place where it is hard to see or misleading. It might be a better solution in the long run, but needs visual design work so that it "pops out" appropriately. So the question is...do we leave this alone or use the DecoratedFieldCellEditor? Since the only use cases are in JDT, I won't push this issue unless JDT wants to fix the clipping issue. Arguments for using it: Fixes the ugly clipping and improves visibility on the new interface wizard, provides a general solution for other use cases in the future. Argument against using it: We would have to get PMC approval for API addition of the DecoratedTextCellEditor and we have limited time. Might be worth the effort if we thought this was the long term answer, but it may not be. I am ambivalent. I am still against the notion of flying decorators from an implementation/focus/platform differences point of view, so I would prefer to use what we already have if possible. It may be "good enough." But if you truly think it will be changed again, then I have plenty of other work to do and would easily punt this issue for now.
The final decision on this has to be made by Martin. In my opinion we shouldn't push on this and add new API for 3.2 since the discussion shows we aren't fully happy yet with this. I think it is worth to further explore the decorated shell approach but we all agree that this will happen post 3.2.
Martin and I discussed at EclipseCon. I showed him the various cell editor uses and how the alternate implementation fixes some problems. We agree that there is no clear reason to rush in API to fix this. I think he is still going to discuss with Markus, but we left the conversation assuming that the existing JDT cell editor implementation would remain for 3.2 and we will discuss again in next release. Side note: the DecoratedField API is already in the release and in use, and there are still issues about dialog layout, etc. happening for that, but it is a separate issue. I'm only stating that here because this was the source of some confusion when Martin and I talked. As soon as I get confirmation from Martin I will pull the cell editor classes from the internal field assist package so as not to cause confusion (they are there now just to try it out)....
moving this bug to RC1. Assuming that we will be pulling the internal cell editor classes out. Martin, please update this bug if I'm wrong about that.
Removed the package org.eclipse.ui.internal.fieldassist from the org.eclipse.ui.workbench plugin as it only contained the experimental cell editor classes. Removing the 3.2 milestone from this bug. We will take up this discussion again in the 3.3 release.
Renaming this bug. Cell editors are a good use case, but the general issue is that clients want a way to decorate an arbitrary control without shifting the alignment of the control. See also bug #145622 and bug #144703. Different reasons for the request, but boils down to a similar solution. Whatever we do here needs to play well for cell editors and UI forms. Are there any other components we need to play well with that anyone knows of? The shell seems a promising solution since it handles the clipping issues and for 3.3 we have time to investigate the inherent focus issues, etc.
investigate for 3.3 M3
Markus and others... I've released (>20061026) new support for decorating arbitrary controls. The changes are intended to address three major issues as we've discussed ad nauseum: 1) decorations should be able to be added to arbitrary, already created controls (bug #145622) 2) decorations should be rendered adjacent to controls without automatically allocating the space, thus forcing the client to align decorated and non-decorated controls (bug #124873) 3) the lack of a controllable margin between a decorated control and the decoration (bug #144703) The new support is in the class ControlDecoration. The creation model has changed, in that you attach a ControlDecoration to an already created control, specifying the positioning of the decoration using SWT style bits. You can now choose to vertically center the decoration, as well as use top or bottom alignment. You may still choose to locate the decoration to the left or right of the control. The decoration is now directly rendered similarly to the platform text content assist. As discussed offline, I investigated using an SWT sibling label to render the control, but the main problem is that there is no generic way to instruct SWT to exclude the decorator label from the layout. Any customized "tricks" to do so (requiring a grid data with exclude=true, a special composite that doesn't iterate the decorators, etc.) did not seem practical. The FieldAssistExample (in org.eclipse.ui.examples.fieldassist) has been updated with a new dialog that uses the new support. ExampleDialog shows use of the DecoratedField. ExampleDialog2 shows use of the ControlDecoration. I realize you originally opened this bug to discuss how DecoratedField would play with cell editors, and it grew into a larger discussion. In order to keep things a bit simpler, I am closing this bug as FIXED and have opened bug #162497 to discuss our latest thinking for how decorations should be shown near table rows...
Verified the presence of the function in I20061031-0656, WinXP by running the FieldAssistExample, ExampleDialog2.
