Community
Participate
Working Groups
The Project Explorer (Common Navigator) currently does not support the ISaveablePart2 interface. It means that its content is stuck with the saving pattern tied to the active editor. Since PE is a common navigator for logical concepts in some cases (like ours) the content providers populate it with much more stuff than just the top level resources. In the combination with context menu actions and the Properties View the entries in the PE are actually modified. We did usability testing centered on modifications of our resources through PE and the users constantly are trying to use CTRL + S (Save), Save As and Save All based on what is selected in Project Explorer, not what is the active editor. I have tried to bypass this by registering ISaveablePart2 adapter on Project Explorer but that does not really work since the PE does not expose any way to send the 'part dirtied' events used to enable various Save related actions. Not to mention that my adapter could easily be in collision with similar adapters from other PE clients. The real solution would be for PE to implement this interface itself and then provide extension point for clients to take on handling for the selected node. If no client handles the selected node, it would resort to the default behavior which is go with the active editor. I marked this as ‘major’ issue for us because it seriously hampers our usability.
This requirement is documented (thanks to this bugzilla) and will be considered a high priority item for the port of the Common Navigator for Platform 3.2. I am tentatively targetting this for 1.0M9, but please be advised that the work may be done in the 3.2 stream, and thus WTP would not pick up the change until WTP 1.5 (on 3.2).
Note that simply implementing ISaveablePart2 is not really the right answer. See bug 112225 comment 11 for more details.
Changing the summary to more accurately reflect the problem. Just to clarify the requirements, it's not really the support for custom save prompts (as provided by ISaveablePart2) that Dusko is after. Dusko, please correct me if I'm mis-stating anything here. What is really needed is to allow content extensions to participate in the save lifecycle if they represent a model that may have unsaved changes. The main requirement here is to allow the Save action (File>Save or Ctrl+S) to save the model(s) containing the selected element(s). Also important is to somehow indicate that the model has unsaved changes, e.g. using a '*' dirty flag on relevant elements. This would typically only be shown on the outermost element representing the "editing domain" (I don't necessarily mean the EMF concept here -- I mean the boundary of what gets saved when you hit Ctrl+S). For example, in RSM, it would be the file representing the model. The navigator view should also likely show the dirty flag if any content extensions being shown have unsaved changes (but see the subsequent comments in bug 112225). To allow us to make progress on this, we need, at a minimum: - a way for a content extension to express that it supports the save lifeycycle (i.e. it may have unsaved changes) - a way for a content extension to express that is currently has unsaved changes (i.e. is dirty) - a way to tell the content extension to save its contents, passing a progress monitor Note that the Save action (class SaveAction) looks for ISaveablePart on the active view or editor using both a direct instanceof check and a getAdapter(ISaveablePart.class) check. This is updated each time the active part changes, or when the active view or editor fires a property change event with ISaveablePart.PROP_DIRTY (e.g. using WorkbenchPart.firePropertyChange(int)). An expedient way to address this problem would be to just delegate the lookup of ISaveablePart to the selected content extension(s), but we'd also need to: 1. properly handle the case where the selection crosses multiple content extensions 2. allow some way for the content extension to notify of a change to the dirty state
This is an important item for RSM for 3.2.
I'm starting to prototype this, using the new ISaveableModel support (see bug 112225 for more details).
I was not able to complete this for M5. Need to reassess in early M6.
Created attachment 36350 [details] work in progress This is just to keep a copy of the current state.
Created attachment 36968 [details] work in progress
Initial version of new API released for I20060330-0010.
Minor comment: INavigatorSaveablesService and ISaveablesSourceHelper are missing class comments. Minimally they should specify if clients can implement them.
Here is a summary of the new API: New classes and interfaces: SaveablesProvider SaveablesProviderFactory INavigatorSaveablesService ISaveablesSourceHelper New in CommonNavigator: implements ISaveablePart, ISaveablesSource New in INavigatorContentService: getSaveablesService() New extension point: saveablesProviderFactory
Some initial comments: The schema of saveablesProviderFactory has no top level description or examples section. This should be corrected. The schema describes a saveablesProviderFactory which has a sequence of one element "navigatorContentId" that has a value attribute. Why not model this element as part of the navigatorContent extension? This would prevent needing to deal with the case where two saveable providers are associated with the same content extension. I would actually describe the following approach to the extension point: (1) Either <saveablesProviderFactory> should be a nested element under the org.eclipse.ui.navigator.navigatorContent extension point, with its own Core Expression. I don't really think that this should be modeled as a factory though, as there are no other cases where a client contributes a factory, which then contributes the actual "extension" piece. This would have the advantage that clients could (a) add saveable models to content that they did not contribute. For instance, a file may be contributed by the Resource extension which has XML content. Another extension might come along and decide to expose the XML document structure in the Navigator as children of a file. The XMLContent extension might then want to be able to indicate when that XML document has gone dirty with decorators and allow the user to save the XML document from the viewer. Your current design would not allow this (or at least it's not obvious how it would work just by looking at the pieces exposed to a client). When using core expressions, there could be more than one client that has a Saveable representation for a given element; however under the current design (driven from the content extension id), the same problem can occur. (2) Or, a <saveablesProvider> element should be nested under a <navigatorContent> element. The implication here would be that the saveableProvider is directly associated with the enclosing navigator content extension, and should be queried whenever the element in the tree matches its possible children expression. This approach limits other downstream providers from contributing a Saveable for an element in the viewer that they did not contribute, unless they declare it as one of their possible children (which I'd consider a hack to get around the limitation). Alternatively, you could drive the selection of which saveableProvider gets precedence by using the _actual_ source of the element; but again, you limit downstream extensibility. I think the example use case of the XML document above is quite likely, and if you enforce this limitation, the only way to implement the use case would be to override the Resource extension for the single element (whatever XML file matched the downstream clients model). The current implementation just ignores extra Saveable objects if they exist for a given object. _________________ This implemention is also subtley different from the other secondary frameworks in the Common Navigator in that it seems to drive all INavigatorContentService instances from a single factory. The other secondary services are created by a INavigatorContentService if necessary, and drive their object lifecycles from the object lifecycle of the INavigatorContentService that the secondary service associated with. This prevents leaking objects that are not required if the view has been closed. The NavigatorSaveablesService should _not_ have any static methods (such as bundleChanged(), add(), or remove()). Under the current implementation, there will be 0..* INavigatorContentServices each with 0..1 INavigatorSaveablesService which drive their state from a single static List.The NavigatorSaveablesService should be the bundleListener (one for each instance) that will add and remove itself as the INavigatorCOntentService creates and disposes() of it. The current design has a bundle listener attached to the NavigatorPlugin that could be notified even if there are no active INavigatorContentServices. There is also no place where the listener is removed so long as the NavigatorPlugin is active. The lifecycle of every other secondary service is driven from the lifecycle of the INavigatorContentService and this service should be as well. I would also recommend elimenating the SaveablesSourceHelper and merging its behavior into the INavigatorSaveablesService. This would be more consistent with the existing architecture. In the current design, a client must create an INavigatorSaveablesService from an INavigatorContentService; then the client must create an ISaveablesSourceHelper. Therefore if the client required access to a single instance of the helper they created, they must manage passing that around (opposed to just sending a handle to the INavigatorContentService, which could return the same INavigatorSaveablesService exposed elsewhere). It also appears that this architecture requires each SaveablesSourceHelper to compute the available Saveables on each bundle event, which means if a client creates more than one of these, then each Helper will duplicate the computation of all other helpers. It's not clear from the implementation why this would be required, and therefore seems unnecessary. Basically the helper is doing the work that the service should be doing. This implementation also fetches the Content Provider from the viewer throughout. Instead it should access the content provider from the contentService that it is created with (I'm referring to the SaveablesSourceHelper class, but again this should be merged into the NavigatorSaveablesService, which is created with the contentService in its constructor). The goto loop in SaveablesSourceHelper needs to be removed. These are _not_ used in the Common Navigator architecture and _should absolutely not_ be used. You should be able to restructure your loop so that you don't need to use the "break <label>" style code. SaveablesSourceHelper.getSaveablesProviders() is not thread safe; there is no synchronziation. It should ensure that saveableProviders is only in the process of being initialized one time. The Factory takes the Content Service Content Provider when it receives a call to createXXX(); I would recommend instead passing in the INavigatorCOntentExtension of the associated extension. The client should not need to call into the full-scale extensibility of the Navigator Content Service Content Provider to determine their local models; the individual content providers will have less overhead and should perform better than the content service content provider which must do more work to compute the children. Clients could use the extensible content provider and accidentally trip the load of plugins that do not need to be loaded; if the client only has access to _their_ content provider, they cannot load more plugins than they need. Why does doSave() and doSaveAs() in Common Navigator not do anything? If nothing is selected in the viewer, shouldn't everything be saved? Should CTRL+SHIFT+S also save all of the contents of the viewer? So in summary (1) The extension point should be a part of org.eclipse.ui.navigator.navigatorContent either as a top level element (saveableProvider) with a core expression of its own or a nested element under <navigatorContent /> and drive itself from either the _actual_ source of an element (see the contribution memory methods on NavigatorContentService) or through the <possibleChildren /> expression. I prefer the former approach. This also implies that with either choice here, the client only provides the SaveableProvider, and not a factory --> provider type pattern. I'd have a SaveableProvider.init(ICommonContentExtensionSite) type method (or another ICommonSaveableExtensionSite, if you think it merits one). (2) SaveableSourceHelper should be merged into NavigatorSaveableService. (a) The goto-like code should be removed. (b) The initial computation of the Saveables should be thread safe. (3) The static bundle listener attached as part of the NavigatorPlugin's start up should be removed. NavigatorSaveableService instances should add themselves as bundle listeners when created and remove themselves when they are disposed. (This implies that they should be one of the disposed services in the NavigatorContentService.dispose() method). This will be required as you refactor NavigatorSaveableService to drive its object lifecycle from that of NavigatorContentService. (4) CommonNavigator (the view part) should implement doSave() and doSaveAs() like a "save all" operation, and also these should be invoked on Ctrl+Shift+S (if not already).
I also noticed that the bundle listener seems to be notified for every bundle event. And for each bundle event, it recomputes the set of Saveables. This means that for scenarios where something like a GMF based editor starts, the 30+ plugins triggered to load the editor will cause the recomputation of the saveable model for every provider. Only affected providers should be recomputed (e.g. only bundle events for bundles that define common navigator extensions should trigger the recomputation). Ideally, this should be done as a Job that is scheduled with a delay for each bundle STARTED or STOPPED event (and only these event types; there is _no_ filtering in the current implementation). The delay should be greater than the average amount of time between two plugins loading when a batch of plugins is loaded. So perhaps a good delay would be anywhere from 250-1000ms? If there was someway to define an ISchedulingRule that would prevent the Job from executing so long as the Bundle management system was loading or unloading bundles. For a commercial product of 1000-2000 plugins, with the Navigator Plugin now being nearer to the bottom of the stack than the top, this has the potential to not only impact startup performance, but also the performance of any plugins loaded once the Workbench is opened. This is really really not a good idea.
Michael, thanks for your feedback, I will respond to it tomorrow.
Michael, I agree with most of your points and will come up with a patch to address them.
Created attachment 38248 [details] work in progress Michael, here is the patch. I would like to discuss it with you before I release it.
Created attachment 38325 [details] patch against org.eclipse.ui.navigator This patch addresses Michael's comments. It changes the API in a way that does not cause compile errors for early adopters who are already using it, but it forces them to update their implementations to prepare for a smooth transition. Some methods and interface are marked as deprecated and will be removed after 3.2RC1. RC1 still contains the deprecated methods in order to avoid compile errors downstream. Please contact me if you need help with porting to the improved API.
Comment #11 is now obsolete. The new API looks like this: New classes and interfaces: SaveablesProvider INavigatorSaveablesService New in CommonNavigator: implements ISaveablePart, ISaveablesSource New in INavigatorContentService: getSaveablesService() New attribute for org.eclipse.ui.navigator.navigatorContent extensions: providesSaveables (boolean, defaults to false) Content providers for which providesSaveables is true must adapt to SaveablesProvider.
+1. ok to proceed.
There's one loophole I see that's easy to fix. The Team use cases wanted the ability to programmatically bind or activate extensions, so onExtensionActivation() doeesn't catch the programmatic visiblity cases (since your configuring the content service directly). All you need to do is : (1) make NavigatorSaveablesService implement VisibilityAssistant.VisibilityListener (2) move " if (navigatorSaveablesService != null) { navigatorSaveablesService.recomputeSaveablesAndNotify(true, null); }" to the listener method and (3) add yourself as a listener to the assistant (the Visibility Assistant in NavigatorContentService) when created (in getNavigatorSaveablesService()) (4) Remove yourself as a listener to the assistant in NavigatorContentService.dispose().
Released (with the changes suggested in comment #20) for >20060411.
Verified using I20060602-0010.
