Community
Participate
Working Groups
Build Identifier: I20110310-1119 The save action gets disabled when the user selects an editpart other than the DiagramEditPart. This can be reproduces in the GMF Logic example. The Save All Action remains active. Reproducible: Always Steps to Reproduce: 1. Create a new GMF Logic example 2. create a LED (the editor gets dirty) 3. select the LED 4. The save action / CTRL + S is disabled / does not work!
This seems to be a problem for *ALL* GMF-based editors, including the Xtext GMF integration example of Xtext. I had to remove that example from the Xtext Indigo release because of bugs like this (and the lack of responsiveness to bug reports from GMF Tooling).
The DiagramActionBarContributor causes the problem. ContributionItemService.getInstance() .contributeToActionBars(bars, descriptor); registers global action handlers for the save action (GlobalSaveAction) and I think the enable state is not updated when the editor fires the dirty state change. Maybe this has something to do with the replacement of the SaveAction with org.eclipse.ui.internal.handlers.SaveHandler in 3.7 ? As a workaround, you can override the init method in your XXActionBarContributor: @Override public void init(IActionBars bars) { super.init(bars); //use the default global action handlers. bars.clearGlobalActionHandlers(); } This works for me...
The previous workaround breaks undo / redo. This is the right one: @Override public void init(IActionBars bars) { super.init(bars); //use the default global action handlers. bars.setGlobalActionHandler(GlobalActionId.SAVE, null); }
It is too late to reenable our Xtext example for Indigo. Seems not only GMF Tooling is abandoned.
I don't have time to investigate why it is happenning. If you have a patch to propose, you're welcome :)
SaveHandler has been introduced to the workbench UI in terms of bug #270007, so the comments there may be worth investigating. RC4 is going to be finalized this Friday so I think we really have to gain ground on this one quite soon. Shipping an Indigo release where no GMF-based graphical editor is able to perfom a save would be quite a nasty thing...
In papyrus we workaround the bug by registering PapyrusGlobalActionHandlerProvider and DiagramWithPrintGlobalActionHandlerProvider on IGraphicalEditPart for save and print action. Save action is still wrongly deactivated after moving an element, and I don't understand why. (see bug 337122).
(In reply to comment #7) > In papyrus we workaround the bug by registering > PapyrusGlobalActionHandlerProvider and > DiagramWithPrintGlobalActionHandlerProvider on IGraphicalEditPart for save and > print action. > > Save action is still wrongly deactivated after moving an element, and I don't > understand why. > (see bug 337122). this bug is on Papyrus, is it on a version based on Eclipse 3.7? If I understand well, save action is enable/disable at the wrong time?
It doesn't look like a bug in Papyrus it used to work out of the box in 0.7. Yes I am talking about Papyrus 0.8 which is in the indigo release train (incubation stage). you can find my workaround commit here (ignore the refactoring) : http://dev.eclipse.org/viewcvs/viewvc.cgi?view=revision&root=MDT_Papyrus&revision=4801
(In reply to comment #9) > It doesn't look like a bug in Papyrus it used to work out of the box in 0.7. > Yes I am talking about Papyrus 0.8 which is in the indigo release train > (incubation stage). > > you can find my workaround commit here (ignore the refactoring) : > http://dev.eclipse.org/viewcvs/viewvc.cgi?view=revision&root=MDT_Papyrus&revision=4801 I tried this workaround for my editor before and encountered the same behaviour. Save action is enable/disable at the wrong time. This is related to GMF and has nothing to do with Papyrus.
ok I understood that it was not related to Papyrus. I was just asking in order to be sure that they observed the issue with Eclipse 3.7 only and if they already find good fix/workaround. Thanks to confirm that it is on Eclipse 3.7 and point your workaround
This issue does not involve Papyrus, I can duplicate without Papyrus in my target. I have all the M4 dependencies required for the GMF Logic Example and the problem does not occur in this M4 based target. I installed GMF RC1 into this M4 based target and the problem still does not occur. The problem is definitely there in the latest RC3 downloads. It would seem there is a regression outside of the GMF Runtime. Do we think the regression was caused by bug 270007 ?
It is strange that the GEF Logic example does not have the same issue in RC3.
(In reply to comment #13) > It is strange that the GEF Logic example does not have the same issue in RC3. This is definitively linked to bug 270007. I try to debug since yesterday to find how to fix this, but this is really not easy. Here is what I understand so far : With the 270007 commit save action is now replaced with a SaveHandler (same for save all action). However when we registered the GlobalSaveAction handler in ContributionItemService#contributeToActionBars, the DirtyStateUpdater (from platform UI) will fire an event to an HandlerProxy instance which will not have as listener the command with "org.eclipse.ui.file.save" as id. I am not sure if this is a platform bug or weird use on our side.
I think it is a bug in GMF since only GMF-based editors have this behavior. I don't think platform is to blame. After having a look at it together with Aurelien, we noticed that the GlobalAction#refresh method is not able to find a handler for the "org.eclipse.ui.file.save" command when a node is selected, whereas it can find one handler when nothing is selected or when a field in properties view is selected. My guess is that this method should always find a GlobalSaveAction whenever a GMF editor is active.
(In reply to comment #15) > I think it is a bug in GMF since only GMF-based editors have this behavior. I > don't think platform is to blame. > > After having a look at it together with Aurelien, we noticed that the > GlobalAction#refresh method is not able to find a handler for the > "org.eclipse.ui.file.save" command when a node is selected, whereas it can find > one handler when nothing is selected or when a field in properties view is > selected. > My guess is that this method should always find a GlobalSaveAction whenever a > GMF editor is active. Anthony what is your opinion ?
(In reply to comment #14) > (In reply to comment #13) > > It is strange that the GEF Logic example does not have the same issue in RC3. > > This is definitively linked to bug 270007. I try to debug since yesterday to > find how to fix this, but this is really not easy. Can you put a blurb on bug 27007 to say that the changes have caused a major regression to GMF diagram editors?
(In reply to comment #17) > (In reply to comment #14) > > (In reply to comment #13) > > > It is strange that the GEF Logic example does not have the same issue in RC3. > > > > This is definitively linked to bug 270007. I try to debug since yesterday to > > find how to fix this, but this is really not easy. > > Can you put a blurb on bug 27007 to say that the changes have caused a major > regression to GMF diagram editors? done on bug 270007
Before Prakash's change, The File>Save menu item pointed to a SaveAction and the org.eclipse.ui.file.save command pointed to an ActionHander(SaveAction). The IDE save action is one per WorkbenchWindow, not per part. Technically, calls to setGlobalActionHandler(*) had the potential to make CTRL+S and File>Save behave differently. Now, both CTRL+S and File>Save point to the same handler, in theory the default handler defined in the plugin.xml in org.eclipse.ui. What are your DiagramActionBarContributor and ContributionItemService.getInstance() .contributeToActionBars(bars, descriptor); doing? Just getting the part IActionBars and calling setGlobalActionHandler(*)? I'm also curious why this is only a problem recently? AFAICT this changes was released in 3.7 M5. PW
(In reply to comment #19) > Before Prakash's change, The File>Save menu item pointed to a SaveAction and > the org.eclipse.ui.file.save command pointed to an ActionHander(SaveAction). > The IDE save action is one per WorkbenchWindow, not per part. > > Technically, calls to setGlobalActionHandler(*) had the potential to make > CTRL+S and File>Save behave differently. > > Now, both CTRL+S and File>Save point to the same handler, in theory the default > handler defined in the plugin.xml in org.eclipse.ui. > > > What are your DiagramActionBarContributor and > ContributionItemService.getInstance() > .contributeToActionBars(bars, descriptor); > doing? Just getting the part IActionBars and calling > setGlobalActionHandler(*)? > > I'm also curious why this is only a problem recently? AFAICT this changes was > released in 3.7 M5. > > PW find call to ContributionItemService.getInstance().contributeToActionBars(bars, descriptor) org.eclipse.gmf.runtime.diagram.ui.parts.DiagramActionBarContributor.init(final IActionBars bars) will investigate on it
This could also be related to the GMF Runtime org.eclipse.gmf.runtime.common.ui.services.action.globalActionHandlerProviders extension point. There is a save referenced in their so I suspect that the issue may be in this service: <GlobalActionId actionId="save"> This is very old code in the "common" later. The GlobalActionHandlerService says 2002 but I suspect its pedigree is two or three years before that.
(In reply to comment #21) > This could also be related to the GMF Runtime > org.eclipse.gmf.runtime.common.ui.services.action.globalActionHandlerProviders > extension point. > > There is a save referenced in their so I suspect that the issue may be in this > service: > <GlobalActionId actionId="save"> > > This is very old code in the "common" later. The GlobalActionHandlerService > says 2002 but I suspect its pedigree is two or three years before that. If I add IGraphicalEditpart as ElementType (previously only DiagramEditPart) fro the save globalActionID: <ElementType class="org.eclipse.gmf.runtime.diagram.ui.editparts.IGraphicalEditPart"> <GlobalActionId actionId="save"> </GlobalActionId> </ElementType> it seems that the enablement state of the save chnage always one time after it should have. What I mean is: - editor not dirty -> save not enable - move an element -> save not enable but saveALl enable - click onb another element -> save enable - do what you want staying on the editor except save -> save enablement stays - do a save -> saveAll is not enabled, save is still enabled - click on a figure -> save is no more enable
Created attachment 197074 [details] fix the issue I add IGraphicalEditPart for ElementTypes of GlobalActionID save AND in GlobalSaveAction, put it to react to property change (IPropertyListener) so in our case react when a figure is moved or modified. Please can others validate it on their use cases?
(In reply to comment #23) > Created attachment 197074 [details] > fix the issue > > I add IGraphicalEditPart for ElementTypes of GlobalActionID save AND in > GlobalSaveAction, put it to react to property change (IPropertyListener) so in > our case react when a figure is moved or modified. > > Please can others validate it on their use cases? I tried your patch. It only works with an additional extension point registration (org.eclipse.gmf.runtime.common.ui.services.action.globalActionHandlerProviders) So the default behaviour (if no globalAction handler is registered explicitly) is still broken and editors without that registration won't work.
(In reply to comment #24) > (In reply to comment #23) > > Created attachment 197074 [details] [details] > > fix the issue > > > > I add IGraphicalEditPart for ElementTypes of GlobalActionID save AND in > > GlobalSaveAction, put it to react to property change (IPropertyListener) so in > > our case react when a figure is moved or modified. > > > > Please can others validate it on their use cases? > > I tried your patch. It only works with an additional extension point > registration > (org.eclipse.gmf.runtime.common.ui.services.action.globalActionHandlerProviders) > > So the default behaviour (if no globalAction handler is registered explicitly) > is still broken and editors without that registration won't work. Oh god you're right. I made my patch badly and it required that the global action handler is registered explicitly. So what is needed in order that the SaveHandler of Eclipse is called when there is a modification on the diagram?
Created attachment 197081 [details] cleaner patch this propose a working solution but need to the client to register a globalactionid
(In reply to comment #26) > Created attachment 197081 [details] > cleaner patch > > this propose a working solution but need to the client to register a > globalactionid this GlobalActionID is generated by default for DiagramEditPArt? If we generate it for IGraphicalEditpart, it should be ok, isn't it?
(In reply to comment #25) > (In reply to comment #24) > > (In reply to comment #23) > > > Created attachment 197074 [details] [details] [details] > > > fix the issue > > > > > > I add IGraphicalEditPart for ElementTypes of GlobalActionID save AND in > > > GlobalSaveAction, put it to react to property change (IPropertyListener) so in > > > our case react when a figure is moved or modified. > > > > > > Please can others validate it on their use cases? > > > > I tried your patch. It only works with an additional extension point > > registration > > (org.eclipse.gmf.runtime.common.ui.services.action.globalActionHandlerProviders) > > > > So the default behaviour (if no globalAction handler is registered explicitly) > > is still broken and editors without that registration won't work. > > > Oh god you're right. > I made my patch badly and it required that the global action handler is > registered explicitly. > > So what is needed in order that the SaveHandler of Eclipse is called when there > is a modification on the diagram? That's the DirtyStateUpdater which should notify the default save handler
(In reply to comment #27) > (In reply to comment #26) > > Created attachment 197081 [details] [details] > > cleaner patch > > > > this propose a working solution but need to the client to register a > > globalactionid > > this GlobalActionID is generated by default for DiagramEditPArt? If we generate > it for IGraphicalEditpart, it should be ok, isn't it? I don't think that this is sufficient. Existing editors will not work. Apart from that, not every one uses GMF Tooling. I personally prefer to build graphical editors "by hand" with GMF Runtime.
(In reply to comment #28) > (In reply to comment #25) > > So what is needed in order that the SaveHandler of Eclipse is called when there > > is a modification on the diagram? > > That's the DirtyStateUpdater which should notify the default save handler So why the DirtyStateUpdater doesn't call the SaveHandler when the modifications occurs on a GMF diagram? as Paul Webster says, Is it because we call a setGlobalActionHandler with an invalid value? @Paul Webster Do you have precise ideas on this?
(In reply to comment #29) > I don't think that this is sufficient. Existing editors will not work. Apart > from that, not every one uses GMF Tooling. I personally prefer to build > graphical editors "by hand" with GMF Runtime. But if you are build ding by hand you have to the globalActionId for DiagramEditPart, isn't it? For previous editor that are not using GMF Tooling, perhaps just a migration note can be enough if we don't find a real solution?
I don't know if it used to work out of the box before but the printing action is also not available by default. registering DiagramWithPrintGlobalActionHandlerProvider fix the problem, and with your patch the save works great too. So if at least the patch is committed it will be ok for papyrus.
(In reply to comment #32) > I don't know if it used to work out of the box before but the printing action > is also not available by default. > registering DiagramWithPrintGlobalActionHandlerProvider fix the problem, and > with your patch the save works great too. > > So if at least the patch is committed it will be ok for papyrus. for DiagramWithprint perhaps I can apply the same fix as GlobalSaveAction. But what about others action: cut, copy, paste, bookmark, delete do we have the same issue with other Providers and if not why we don't have them?
(In reply to comment #31) > (In reply to comment #29) > > I don't think that this is sufficient. Existing editors will not work. Apart > > from that, not every one uses GMF Tooling. I personally prefer to build > > graphical editors "by hand" with GMF Runtime. > > But if you are build ding by hand you have to the globalActionId for > DiagramEditPart, isn't it? > > For previous editor that are not using GMF Tooling, perhaps just a migration > note can be enough if we don't find a real solution? When building by hand, I never had to register any globalActionHandlerProviders by hand. The default behaviour for save worked as expected out of the box.
(In reply to comment #33) > for DiagramWithprint perhaps I can apply the same fix as GlobalSaveAction. It seems ok, an equivalent code to handle property changes seems to be present in GlobalPrintAction. what I was saying is, as with save, print is not available out of the box if you're not registering anything on globalActionHandlerProviders.
Andreas Mülder said: > When building by hand, I never had to register any globalActionHandlerProviders > by hand. The default behaviour for save worked as expected out of the box. Mathieu Velten said: > what I was saying is, as with save, print is not available out of the box if > you're not registering anything on globalActionHandlerProviders. available or not? I think that we don't have the same definition of out of the box.
for me out of the box means without registering something on globalActionHandlerProviders extension point. in this case save and print are not available at all while it used to be with helios. I am ignoring your patch here since I think it should be committed anyway.
Created attachment 197092 [details] patch of gmf tooling if solution here is retained: https://bugs.eclipse.org/bugs/show_bug.cgi?id=346648#c26
(In reply to comment #30) > So why the DirtyStateUpdater doesn't call the SaveHandler when the > modifications occurs on a GMF diagram? when you modify a GMF diagram, are you returning true from isDirty() and firing the org.eclipse.ui.ISaveablePart.PROP_DIRTY? PW
(In reply to comment #39) > (In reply to comment #30) > > So why the DirtyStateUpdater doesn't call the SaveHandler when the > > modifications occurs on a GMF diagram? > > when you modify a GMF diagram, are you returning true from isDirty() and firing > the org.eclipse.ui.ISaveablePart.PROP_DIRTY? > > PW I think yes as the SaveAllHandler is working. org.eclipse.gmf.runtime.diagram.ui.resources.editor.parts.DiagramDocumentEditor.isDirty(){ IDocumentProvider p= getDocumentProvider(); return p == null ? false : p.canSaveDocument(getEditorInput()); } if I add a firing inside the method isDirty directly, I have a stackoverflowerror
(In reply to comment #40) > org.eclipse.gmf.runtime.diagram.ui.resources.editor.parts.DiagramDocumentEditor.isDirty(){ > IDocumentProvider p= getDocumentProvider(); > return p == null ? false : p.canSaveDocument(getEditorInput()); > } > > if I add a firing inside the method isDirty directly, I have a > stackoverflowerror yes, setting the dirty property should fire the event, not the read. PW
(In reply to comment #39) > (In reply to comment #30) > > So why the DirtyStateUpdater doesn't call the SaveHandler when the > > modifications occurs on a GMF diagram? > > when you modify a GMF diagram, are you returning true from isDirty() and firing > the org.eclipse.ui.ISaveablePart.PROP_DIRTY? > > PW yes. As I said previously, when I debug I found that the HandlerProxy does not has a listener the save command instance. >What are your DiagramActionBarContributor and >ContributionItemService.getInstance() >.contributeToActionBars(bars, descriptor); > doing? Just getting the part IActionBars and calling >setGlobalActionHandler(*)? yes you may find the code in AbstractContributionItemProvider class, but it is really difficult to follow without debugging. >I think yes as the SaveAllHandler is working Yes the SaveAllHandler is working fine but we GMF does not override the default global action handler for save all command.
Summarize, a change in the platform in 3.7 M4 has caused a regression such that save is never enabled on a dirty editor with a selection on the diagram. If we do nothing, save will be broken on all GMF Runtime based editors when we release Indigo. If we install the patch, we are partially confident the problem is solved. The change is limited to the save action, so the solution cannot do further damage than save not working at all. Given the risk, I way we commit the patch for RC4.
+1 for the commit.
patch applied
