Bug 255264 - Out of date diagram editing domain used in property section after "Save As"
Summary: Out of date diagram editing domain used in property section after "Save As"
Status: RESOLVED FIXED
Alias: None
Product: GMF-Runtime
Classification: Modeling
Component: General (show other bugs)
Version: 2.1   Edit
Hardware: PC Windows XP
: P3 critical
Target Milestone: 2.1.3   Edit
Assignee: Alex Boyko CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-11-13 14:47 EST by Li Ding CLA
Modified: 2008-12-17 21:52 EST (History)
3 users (show)

See Also:


Attachments
proposed patch (3.43 KB, patch)
2008-12-10 17:25 EST, Alex Boyko CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Li Ding CLA 2008-11-13 14:47:16 EST
Build ID: M20080911-1700

Steps To Reproduce:
When saving a diagram as a new diagram, a new diagram editing domain instance will be created as part of processing in DiagramDocumentEditor.doSetInput().
However, the diagram editing domain instance cached in property section is not updated accordingly if the property page is currently opened.

This problem can be easily seen while debugging:
1. Set a bkp at AbstractModelerPropertySection.getEditingDomain()
2. Set a bkp at 
TransactionalEditingDomainImpl.initialize().
3. Open a GMF Geoshape example diagram. Select "Show properties view" to open properties view.
4. Take a note for the object id of TransactionalEditingDomainImpl and the id of editing domain returned from AbstractModelerPropertySection.getEditingDomain(). Those two should be the same.
5. Now do a Save As for the diagram.
6. A new instance of TransactionalEditingDomainImpl is created because editor take a new input.
7. Change Font on property page, you will see the editing domain returned from AbstractModelerPropertySection.getEditingDomain() is the old one.


This causes a NPE problem for my editor. My editor is created from an in-memory object. After "Save As..", the editor switches to a file input. In between, my editor does some cleanup including disposing the old editing domain to avoid memory leak. As a result, the recorder field is set as null in the old instance. 

When changing font in property section, the section uses the disposed editing domain which causes NPE eventually ( because recorder is null in the following call:
getInternalDomain().getChangeRecorder().getValidateEditSupport())


This is not new problem in GMF runtime 2.1.2. However, the emf TransactionImpl seems to be updated in eclipse 3.4.1. It now has more entries to call getInternalDomain().getChangeRecorder().getValidateEditSupport() which makes the NPE problem is easier to produce.

More information:
Comment 1 Anthony Hunter CLA 2008-12-02 12:29:57 EST
It is not clear to me how this happens.

When you save as, the model is reloaded and the selection refreshed?

If the selection is refreshed then the AbstractModelerPropertySection should be refreshed and no longer be holding onto the old editing domain.
Comment 2 Li Ding CLA 2008-12-02 14:01:41 EST
From what I saw, the selection was not changed. 

Using Geoshape example, what I did was Save as.. to a new diagram, go directly to properties view to change font size to 10. However, the font was not changed.

Actually there was another NPE problem in .log. I will raise another bugzilla :
java.lang.NullPointerException
at org.eclipse.gef.editparts.AbstractEditPart.getViewer(AbstractEditPart.java:601)
at org.eclipse.gmf.runtime.diagram.ui.editparts.DiagramEditPart.getConnections(DiagramEditPart.java:227)
at org.eclipse.gmf.runtime.diagram.ui.editparts.DiagramEditPart.getPrimaryEditParts(DiagramEditPart.java:241)
at org.eclipse.gmf.runtime.diagram.ui.properties.sections.appearance.DiagramColorsAndFontsPropertySection.getInputIterator(DiagramColorsAndFontsPropertySection.java:39)
at org.eclipse.gmf.runtime.diagram.ui.properties.sections.appearance.ColorsAndFontsPropertySection.updateFontSize(ColorsAndFontsPropertySection.java:543)
at org.eclipse.gmf.runtime.diagram.ui.properties.sections.appearance.ColorsAndFontsPropertySection$2.widgetSelected(ColorsAndFontsPropertySection.java:226) 
Comment 3 Li Ding CLA 2008-12-02 15:17:09 EST
For comment #1, 

If I click on diagram canvas right after the save as action, the input does change. ColorsAndFontsPropertySection.setInput is called.

However, even ColorsAndFontsPropertySection.setInput is called, the cached editing domain instance in ColorsAndFontsPropertySection ( accessiable from AbstractModelerPropertySection ) does not update. 

The only place calls AbstractModelerPropertySection.setEditingDomain is AdvancedPropertySection.setInput. 

Maybe AbstractModelerPropertySection.setInput should do similar thing ?


Comment 4 Alex Boyko CLA 2008-12-10 17:25:49 EST
Created attachment 120135 [details]
proposed patch

I propose two things to fix this:
1. Update editing domain when input is set, (as it's done for advanced properties section)
2. After save as is perfomed by the editor and new input is set, call deselectAll() on the viewer. This results in firing the selection event where the selection is implicitly selected contents editaprt (diagram editpart in our case). This is to handle a case where nothing is selected on the diagram (except implicitly selected diagram) and "save as" is clicked. In this case the new input for properties can't be set, because the part stays the same and selection stays the same too.
Li, please let me know if this patch fixes the problem for you.
Comment 5 Li Ding CLA 2008-12-11 15:29:38 EST
I applied patch but have not got that far to verify it. Still got same problem mentioned in comment #2 with Geoshape diagram. 

java.lang.NullPointerException
	at org.eclipse.gef.editparts.AbstractEditPart.getViewer(AbstractEditPart.java:601)
	at org.eclipse.gmf.runtime.diagram.ui.editparts.DiagramEditPart.getConnections(DiagramEditPart.java:227)
	at org.eclipse.gmf.runtime.diagram.ui.editparts.DiagramEditPart.getPrimaryEditParts(DiagramEditPart.java:241)
	at org.eclipse.gmf.runtime.diagram.ui.properties.sections.appearance.DiagramColorsAndFontsPropertySection.getInputIterator(DiagramColorsAndFontsPropertySection.java:39)
	at org.eclipse.gmf.runtime.diagram.ui.properties.sections.appearance.ColorsAndFontsPropertySection.updateFontSize(ColorsAndFontsPropertySection.java:543)
	at org.eclipse.gmf.runtime.diagram.ui.properties.sections.appearance.ColorsAndFontsPropertySection$2.widgetSelected(ColorsAndFontsPropertySection.java:226)
	at org.eclipse.swt.widgets.TypedListener.handleEvent(TypedListener.java:228)
	at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:84)
	at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1003)
	at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1027)
	at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1012)
	at org.eclipse.swt.widgets.Widget.notifyListeners(Widget.java:770)
	at org.eclipse.swt.custom.CCombo.listEvent(CCombo.java:1018)
	at org.eclipse.swt.custom.CCombo$1.handleEvent(CCombo.java:110)
	at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:84)
	at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1003)
	at org.eclipse.swt.widgets.Display.runDeferredEvents(Display.java:3823)
	at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3422)
	at org.eclipse.ui.internal.Workbench.runEventLoop(Workbench.java:2382)
	at org.eclipse.ui.internal.Workbench.runUI(Workbench.java:2346)
	at org.eclipse.ui.internal.Workbench.access$4(Workbench.java:2198)
	at org.eclipse.ui.internal.Workbench$5.run(Workbench.java:493)
	at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:288)
	at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:488)
	at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:149)
	at org.eclipse.ui.internal.ide.application.IDEApplication.start(IDEApplication.java:113)
	at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:193)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:110)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:79)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:386)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:179)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:45)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
	at java.lang.reflect.Method.invoke(Method.java:612)
	at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:549)
	at org.eclipse.equinox.launcher.Main.basicRun(Main.java:504)
	at org.eclipse.equinox.launcher.Main.run(Main.java:1236)
	at org.eclipse.equinox.launcher.Main.main(Main.java:1212) 
Comment 6 Li Ding CLA 2008-12-11 15:38:44 EST
For comment #2 and #5: NPE happens because the editor is not on focus. 
Comment 7 Li Ding CLA 2008-12-11 15:57:42 EST
The original problem (domain not updated) was fixed by patch.

Do you want me to open a new bug to track the NPE in comment #2 ? 
Comment 8 Alex Boyko CLA 2008-12-11 16:50:12 EST
Is the NPE from comment #2 is indeed a regression for you?
Comment 9 Alex Boyko CLA 2008-12-12 15:28:40 EST
Hmm... I don't get this NPE anymore... Li, could please provide the steps that reproduce this issue with a geoshape diagram? And yes, please open a new bugzilla for the NPE from comment 2 and specify whether this is regression from 1.0.3 
Comment 10 Alex Boyko CLA 2008-12-12 15:30:41 EST
BTW, since the patch solves the critical issue for you, I'll get on committing the fix. You do want us to fix the initial issue in 2.1.3, correct?
Comment 11 Li Ding CLA 2008-12-12 15:57:27 EST
Yes, I want the initial problem to be fixed in 2.1.3.

I will open a new bug for the left over NPE problem.
Comment 12 Alex Boyko CLA 2008-12-15 11:17:56 EST
Hi Anthony, could you take a look at my attached patch and see if it's ok to commit it for 2.1.3?
Comment 13 Anthony Hunter CLA 2008-12-17 14:25:09 EST
Hi Alex, the change looks good for 2.1.3.
Comment 14 Alex Boyko CLA 2008-12-17 21:52:32 EST
Fix committed for 2.1.3 and 2.2
Comment 15 Eclipse Webmaster CLA 2010-07-19 12:30:27 EDT
[GMF Restructure] Bug 319140 : product GMF and component Runtime Diagram was the original product and component for this bug