Bugzilla – Full Text Bug Listing
| Summary: | [FieldAssist] API - Applications should be able to install a reserved decoration size and commonly used decorations | ||
|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Markus Keller <markus.kell.r> |
| Component: | UI | Assignee: | Susan McCourt <susan> |
| Status: | VERIFIED FIXED | QA Contact: | |
| Severity: | enhancement | ||
| Priority: | P3 | CC: | daniel_megert, n.a.edgar, susan, Tod_Creasey |
| Version: | 3.2 | ||
| Target Milestone: | 3.2 M5 | ||
| Hardware: | PC | ||
| OS: | Windows XP | ||
| Whiteboard: | |||
This is a good point. I didn't like the static either, but had not evolved things to the point of having a better solution. The timing here is good. I'm working right now on workbench-level decoration code that can supply stock decorations, such as the content assist cue decoration. I started to wonder if this class (which is currently just a static that can answer particular decorations) should instead fit into a registry concept. If we go the factory concept, then this factory could also have the capability of registering decorations by name. Then the workbench could create its factory/registry with the specified width and install the common decorations. Thoughts? Tod - any thoughts from a JFace point of view? I've released some initial support for this >20060127, marked experimental. I suspect it will need some iteration, but I wanted to get something you could look at. I'm also cc'ing Dani (fyi) and Nick (please comment on RCP issues). The initial concern in this bug is that I was using a static to specify reserved decoration width and Markus wants a way for apps to install their own policy without affecting the decorations of others. He suggested a factory for creating decorated fields, so that a reserved width could be set into the field by the factory, and apps could use different factories if needed. However, an additional concern that has come up in bug #124873 is how to use DecoratedFields inside cell editors. My feeling is that we will end up with a generic notion of FieldDecorations, with different renderings based on their use. For example, a DecoratedField might draw the decorations in the reserved space next to the field as it does today. A cell editor decoration might float the decoration next to the cell editor. This leads me to believe that a factory that sets the width into decorated fields is not the answer, but rather a way for DecoratedField and potential other implementations to get to the preferred sizes in a way that lets apps install overrides for the defaults. What I did: - FieldDecorationRegistry defines a registry for decorations keyed by decoration id. It also provides accessors for the reserved width and height that match these supplied decorations. It makes sense to me to supply the decorations that conform to the reserved widths and heights in the same place where the widths/heights are declared. - There is a singleton registry that can be set and accessed by applications: FieldDecorationRegistry.setDefault(new FieldDecorationRegistry(width, height)); FieldDecorationRegistry.getDefault().getFieldDecoration("someId"); FieldDecorationRegistry.getDefault().getReservedFieldDecorationWidth(); - DecoratedField (and in the future...DecoratedCellEditor?) can get the reserved width and height from the default registry. What I don't like about this is: - I'm not sure it addresses Markus' initial concern since the DecoratedField uses the singleton to get the reserved width. This doesn't let you mix and match different decoration strategies. An alternate approach is to set the registry into each decorated field that is created. - I'm not sure if the initialization of the registry by the workbench is in the right place for an RCP app. I initialize the decorations in Workbench.init() because it made sense to do it where colors, fonts, and such are initialized. But this doesn't make it simple for an RCP app to override what happens...perhaps this should be done in the WorkbenchAdvisor somewhere?...Nick? Markus - I suspect that you would find setting a decoration registry into each field as cumbersome as setting a width on each field. I can also add factory methods for DecoratedFields to this registry, so that the width is automatically set up, but I'm concerned this won't scale if we then add a DecoratedCellEditor, etc. Let me know what you think after you've digested all of this. At first glance, it seems like the FieldDecorationRegistry is attempting to do two separate things: 1. be a registry for known field decorations 2. provide default dimensions I guess there is a relationship between the two, in that the default dimensions should be greater than or equal to the size of all registered decorations. Is there an actual requirement to allow the default size to be changed? If not, I'd suggest just keeping this internal. Also, can other plug-ins register decorations? If so, you might want to consider allowing them to be added declaratively via an extension point (we never did this for ISharedImages, which was a mistake). Since the API is at JFace layer, which does not use extension points, the extension point could be defined in org.eclipse.ui, and read early to populate the JFace registry. For the RCP APIs, it depends on what you want to allow the application to do. The field registry is currently initialized before WorkbenchAdvisor.initialize() is called. This allows the app to either replace the registry (wiping out any default entries), or add more entries to it. Of course, there's nothing to prevent regular plugins from doing this as well, though the plugin would need to be activated first. Thanks for the comments, Nick. The relationship between the minimum size and images is what drove me to put them both together. Your comments make me realize another possibility that makes more sense. The registry could compute the maximum decoration size for the registered decorations and use this value for reserved space, rather than making a public setter. Down side is that a plug-in registering a large decoration can affect the layout of other dialogs, but I think this is reasonable since you don't know when/if different sized decorations will be mixed on the same dialog. This solves Markus' fear of an app setting it to be too small. I think the getter for reserved width and height needs to remain public for these uses: - DecoratedField needs to know how much space to reserve for decorations (this could be made package visiblity, but...) - Clients that mix decorated fields with non-decorated fields need to be able to know how much space is reserved so that fields line up. If the space isn't accessible through public API, the only choice for a client is make every field a decorated field so that they all line up. I was hoping to avoid forcing this on clients. So I think the getter remains API but should be renamed maximum decoration width rather than reserved. Then the class feels more like a registry which happens to let you access computed maximums for the decorations it contains. Given your comments I think things are set up properly for an RCP app (they can wipe it out or add to it). There's no reason a plug-in shouldn't be able to add a decoration to the registry, although the original requirement is to make the ones used by the workbench accessible to others. Do you see a reason why the extension point should be declared now or can this be added later if we see through usage that it makes sense? I think we should hold off on the extension point until there's an actual need, but allowing it may impact the API as follows: - We would likely want to defer loading of the image until used, so it would be better if the API was in terms of ImageDescriptor instead of Image. But then somebody would need to manage the lifecycle of the Image (it could go in the JFaceResources.getResources() resource manager). - If the the image loading is deferred, then you don't know its dimensions up front, so that would need to be passed in to the API, and declared as an attribute in the extension point. Fixed >20060202. I think I have a solution that meets all needs and makes it possible to define these declaratively in the future. - decoration images can be declared in three ways: 1) pass in an image id. It is looked up in the JFaceResources.getImageRegistry() registry. Easy for plain JFace apps. 2) pass in an image id and a registry. It is looked up in the given registry. Easy for workbench-level stuff (using workbench image registry). 3) pass in an image. It is used directly (there is a use case for this already whereby one decoration wants to use the image defined by another one). - image lifecycle is not managed by the decoration registry. It is assumed to be done by the image registry for cases 1 and 2 and by the client for case 3 - images are not created until the decoration is requested - the computed maximum size is updated any time a decoration is requested (which causes the image to be created) - DecoratedField consults the computed size when allocating space - Clients can consult the computed max size to make non-decorated fields line up...such as: GridData data = new GridData(); data.horizontalAlignment = SWT.FILL; data.horizontalIndent = FieldDecorationRegistry.getDefault() .geMaximumDecorationWidth(); I don't like the idea of declaring the image size in the markup or in the API, as it will be error prone when images change. I think the chances of having dynamic sizing issues are slim. I modified decorated field to handle the case whereby a decoration gets added after the dialog is up and it is of a larger size. The fields will adjust. Clients who set layout margins using the value could possibly get burned if the size is consulted before never-used decorations of larger sizes were obtained, but I don't see this as a typical code path. Normally the decorations are the same size, or they are all created up front (some hidden, some not). whoops, meant to mark it fixed. verified in 20060214-0800 on Win XP - the find/replace patch from bug #120237 demonstrates that ContentAssistField finds the cue from the registry - the field assist example uses its own registry to register the required field, error field, and cue decorations - the dialog layout code successfully uses the registry's #getMaxDecorationWidth() to lay out decorated and non-decorated fields and align them. |
I20060119-0800 The API in DecoratedField to modify the static variable RESERVED_WIDTH is dangerous. In big applications, you can never know who may have called setReservedDecorationWidth(-1), and this could screw up all other clients of DecoratedField. The workaround to always use ... DecoratedField.setReservedDecorationWidth(8); new DecoratedField(..); ... is a bit ugly and is easily forgotten. I would prefer a proper encapsulation of this width e.g. in a DecoratedFieldFactory: public class DecoratedFieldFactory { public DecoratedFieldFactory(int reservedWidth) {...} ; public DecoratedField create(Composite parent, int style, IControlCreator controlCreator) {...}; public static DecoratedFieldFactory getDefault() {...}; } Clients using the JFace-supplied adornments could then use the default factory while other clients could provide their special factory in a central access point of their framework.