Bug 574094: [Toolsmiths - Properties Editor] Unmodified XWT files are saved/reformatted

The mechanism of trying to create an output stream to test for writability of a resource
truncates the resource to 0 bytes, after which it is (usually) saved again with full
content. But that defeats EMF's save-only-if-modified-by-memory-buffer strategy.

Instead, give the editing domain specific knowledge of ppe: URI scheme and use
heuristics supported by EMF (including URI schemes and URIConverter) to test for
real read-only condition and also, while saving, provide ephemeral read-only state
for resources that are not modified. This prevents unmodified but otherwise writable
XWT resources being saved again with slightly rearranged but otherwise identical content.

Change-Id: I0df7e37847df85eb5a0f84f9b1feae7020170be8
Signed-off-by: Christian W. Damus <give.a.damus@gmail.com>

2 files changed, 93 insertions, 50 deletions

diff --git a/plugins/views/properties/org.eclipse.papyrus.views.properties.toolsmiths/src/org/eclipse/papyrus/views/properties/toolsmiths/editor/UIEditor.java b/plugins/views/properties/org.eclipse.papyrus.views.properties.toolsmiths/src/org/eclipse/papyrus/views/properties/toolsmiths/editor/UIEditor.java
index 178ada406f1..ea2a1b1134d 100644
--- a/plugins/views/properties/org.eclipse.papyrus.views.properties.toolsmiths/src/org/eclipse/papyrus/views/properties/toolsmiths/editor/UIEditor.java
+++ b/plugins/views/properties/org.eclipse.papyrus.views.properties.toolsmiths/src/org/eclipse/papyrus/views/properties/toolsmiths/editor/UIEditor.java
@@ -12,14 +12,12 @@
  *  Camille Letavernier (CEA LIST) camille.letavernier@cea.fr - Initial API and implementation
  *  Christian W. Damus (CEA) - Factor out workspace storage for pluggable storage providers (CDO)
  *  Nicolas FAUVERGUE (CEA) nicolas.fauvergue@cea.fr - Bug 548720
- *  Christian W. Damus - bug 573987
+ *  Christian W. Damus - bugs 573987, 574094
  *****************************************************************************/
 package org.eclipse.papyrus.views.properties.toolsmiths.editor;
 
 import static org.eclipse.papyrus.views.properties.toolsmiths.preferences.CustomizationEditorActionKind.getOnOpenCustomizationEditorAction;
 
-import java.io.IOException;
-import java.io.OutputStream;
 import java.util.EventObject;
 import java.util.HashMap;
 import java.util.HashSet;
@@ -43,7 +41,7 @@ import org.eclipse.emf.ecore.provider.EcoreItemProviderAdapterFactory;
 import org.eclipse.emf.ecore.resource.Resource;
 import org.eclipse.emf.ecore.resource.ResourceSet;
 import org.eclipse.emf.ecore.resource.URIConverter;
-import org.eclipse.emf.ecore.resource.URIHandler;
+import org.eclipse.emf.ecore.resource.impl.ResourceSetImpl;
 import org.eclipse.emf.ecore.util.EcoreUtil;
 import org.eclipse.emf.ecore.xmi.XMLResource;
 import org.eclipse.emf.edit.domain.AdapterFactoryEditingDomain;
@@ -291,43 +289,13 @@ public class UIEditor extends EcoreEditor implements ITabbedPropertySheetPageCon
 
 	@Override
 	public void doSave(IProgressMonitor progressMonitor) {
-		if (editingDomain.getResourceToReadOnlyMap() == null) {
-			editingDomain.setResourceToReadOnlyMap(new HashMap<Resource, Boolean>());
-		}
-
-		Map<Resource, Boolean> readOnlyMap = editingDomain.getResourceToReadOnlyMap();
-		ResourceSet resourceSet = getEditingDomain().getResourceSet();
-
-		for (Resource resource : resourceSet.getResources()) {
-			if (readOnlyMap.containsKey(resource)) {
-				continue;
-			}
-
-			URIHandler handler = resourceSet.getURIConverter().getURIHandler(resource.getURI());
-			Map<String, Object> options = new HashMap<>();
-			options.put(URIConverter.OPTION_URI_CONVERTER, resourceSet.getURIConverter());
-			try {
-				OutputStream os = handler.createOutputStream(resource.getURI(), options);
-				readOnlyMap.put(resource, os == null);
-				if (os != null) {
-					os.close();
-				}
-			} catch (IOException ex) {
-				// Currently, createOutputStream() fails on a NPE if the resource is read-only.
-				// Only log a warning, since the editor is currently not able to properly check for
-				// read-only state without calling createOutputStream
-				// See Bug 351146 for potential options regarding a proper fix
-				Activator.log.warn("Trying to save a read-only resource: " + resource.getURI());
-				readOnlyMap.put(resource, true);
-			}
-		}
-
 		setSaving(true);
 		try {
 			super.doSave(progressMonitor);
 		} finally {
 			setSaving(false);
 		}
+
 		refreshContext();
 	}
 
@@ -521,6 +489,13 @@ public class UIEditor extends EcoreEditor implements ITabbedPropertySheetPageCon
 
 					@Override
 					public void run() {
+						if (!commandStack.isSaveNeeded()) {
+							// Make sure that all resources report unmodified so that when next
+							// checked, they will not remember having been modified by changes
+							// that were undone
+							editingDomain.getResourceSet().getResources().forEach(res -> res.setModified(false));
+						}
+
 						firePropertyChange(IEditorPart.PROP_DIRTY);
 
 						// Try to select the affected objects.
@@ -542,7 +517,71 @@ public class UIEditor extends EcoreEditor implements ITabbedPropertySheetPageCon
 			}
 		});
 
-		// Replace the parent editing domain with a standard one. We don't want to override the isReadOnly() method.
-		editingDomain = new AdapterFactoryEditingDomain(adapterFactory, commandStack);
+		// Track modification in the resources that we load, to not save unmodified resources.
+		ResourceSet resourceSet = new ResourceSetImpl() {
+			@Override
+			public Resource createResource(URI uri, String contentType) {
+				Resource result = super.createResource(uri, contentType);
+				result.setTrackingModification(true);
+				return result;
+			}
+		};
+
+		// Customize read-only testing to account for ppe: scheme URIs and also preventing save
+		// of unmodified resources
+		editingDomain = new AdapterFactoryEditingDomain(adapterFactory, commandStack, resourceSet) {
+			private final PropertiesURIHandler ppeHandler = new PropertiesURIHandler();
+
+			{
+				resourceToReadOnlyMap = new HashMap<>();
+			}
+
+			@Override
+			public boolean isReadOnly(Resource resource) {
+				if (resource == null) {
+					return true;
+				}
+
+				boolean result = basicIsReadOnly(resource);
+
+				// Additionally, if we're saving, treat unmodified resources as though
+				// read-only to avoid rewriting XWT files
+				if (isSaving() && !result) {
+					result = resource.isTrackingModification() && !resource.isModified();
+				}
+
+				return result;
+			}
+
+			protected boolean basicIsReadOnly(Resource resource) {
+				Boolean result = resourceToReadOnlyMap.get(resource);
+				if (result == null && resource != null) {
+					URIConverter converter = getResourceSet().getURIConverter();
+					URI uri = converter.normalize(resource.getURI());
+
+					// Handle ppe: scheme
+					if (ppeHandler.canHandle(uri)) {
+						uri = ppeHandler.getConvertedURI(uri);
+					}
+
+					if (uri.isPlatformPlugin()) {
+						// Don't let modifications be made in resources deployed in plug-ins, even if those
+						// plug-ins are in the filesystem and therefore writable
+						result = true;
+					} else {
+						Map<?, ?> options = Map.of(URIConverter.OPTION_REQUESTED_ATTRIBUTES, Set.of(URIConverter.ATTRIBUTE_READ_ONLY));
+						result = Boolean.TRUE.equals(converter.getAttributes(uri, options).get(URIConverter.ATTRIBUTE_READ_ONLY));
+					}
+
+					resourceToReadOnlyMap.put(resource, result);
+				}
+
+				return Boolean.TRUE.equals(result);
+			}
+
+		};
+
+		// This editor relies on the traceability from the resource set to the editing domain
+		resourceSet.eAdapters().add(new AdapterFactoryEditingDomain.EditingDomainProvider(editingDomain));
 	}
 }
diff --git a/plugins/views/properties/org.eclipse.papyrus.views.properties.toolsmiths/src/org/eclipse/papyrus/views/properties/toolsmiths/editor/preview/Preview.java b/plugins/views/properties/org.eclipse.papyrus.views.properties.toolsmiths/src/org/eclipse/papyrus/views/properties/toolsmiths/editor/preview/Preview.java
index e5ad11d2f1a..552660e8df7 100644
--- a/plugins/views/properties/org.eclipse.papyrus.views.properties.toolsmiths/src/org/eclipse/papyrus/views/properties/toolsmiths/editor/preview/Preview.java
+++ b/plugins/views/properties/org.eclipse.papyrus.views.properties.toolsmiths/src/org/eclipse/papyrus/views/properties/toolsmiths/editor/preview/Preview.java
@@ -1,5 +1,5 @@
 /*****************************************************************************
- * Copyright (c) 2010, 2014 CEA LIST and others.
+ * Copyright (c) 2010, 2021 CEA LIST, Christian W. Damus, and others.
  *
  * All rights reserved. This program and the accompanying materials
  * are made available under the terms of the Eclipse Public License 2.0
@@ -12,6 +12,7 @@
  *  Camille Letavernier (CEA LIST) camille.letavernier@cea.fr - Initial API and implementation
  *  Christian W. Damus (CEA) - Use URIs to support non-URL-compatible storage (CDO)
  *  Christian W. Damus (CEA) - bug 417409
+ *  Christian W. Damus - bug 574094
  *
  *****************************************************************************/
 package org.eclipse.papyrus.views.properties.toolsmiths.editor.preview;
@@ -34,13 +35,10 @@ import java.util.Set;
 import org.eclipse.core.runtime.IPath;
 import org.eclipse.emf.common.util.URI;
 import org.eclipse.emf.ecore.EObject;
+import org.eclipse.emf.ecore.resource.Resource;
 import org.eclipse.jface.viewers.ISelectionChangedListener;
 import org.eclipse.jface.viewers.IStructuredSelection;
 import org.eclipse.jface.viewers.SelectionChangedEvent;
-import org.eclipse.papyrus.views.properties.model.xwt.resource.XWTResource;
-import org.eclipse.papyrus.views.properties.toolsmiths.Activator;
-import org.eclipse.papyrus.views.properties.toolsmiths.editor.UIEditor;
-import org.eclipse.papyrus.views.properties.toolsmiths.messages.Messages;
 import org.eclipse.papyrus.emf.facet.custom.metamodel.v0_2_0.internal.treeproxy.TreeElement;
 import org.eclipse.papyrus.infra.emf.utils.EMFHelper;
 import org.eclipse.papyrus.infra.properties.contexts.Section;
@@ -49,6 +47,10 @@ import org.eclipse.papyrus.infra.properties.contexts.View;
 import org.eclipse.papyrus.infra.properties.ui.runtime.DefaultDisplayEngine;
 import org.eclipse.papyrus.infra.properties.ui.runtime.DisplayEngine;
 import org.eclipse.papyrus.infra.properties.ui.widgets.layout.PropertiesLayout;
+import org.eclipse.papyrus.views.properties.model.xwt.resource.XWTResource;
+import org.eclipse.papyrus.views.properties.toolsmiths.Activator;
+import org.eclipse.papyrus.views.properties.toolsmiths.editor.UIEditor;
+import org.eclipse.papyrus.views.properties.toolsmiths.messages.Messages;
 import org.eclipse.swt.SWT;
 import org.eclipse.swt.custom.CTabFolder;
 import org.eclipse.swt.custom.CTabItem;
@@ -106,7 +108,7 @@ public class Preview extends ViewPart implements ISelectionChangedListener, IPar
 
 	private Label previewDisabled;
 
-	private Set<UIEditor> currentEditors = new HashSet<UIEditor>();
+	private Set<UIEditor> currentEditors = new HashSet<>();
 
 	private IWorkbenchPage activePage;
 
@@ -154,7 +156,7 @@ public class Preview extends ViewPart implements ISelectionChangedListener, IPar
 		GridData data;
 
 		// Label preview = new Label(controls, SWT.NONE);
-		//		preview.setImage(Activator.getDefault().getImage("/icons/preview.png")); //$NON-NLS-1$
+		// preview.setImage(Activator.getDefault().getImage("/icons/preview.png")); //$NON-NLS-1$
 		// data = new GridData(SWT.CENTER, SWT.BEGINNING, false, false);
 		// preview.setLayoutData(data);
 
@@ -259,9 +261,10 @@ public class Preview extends ViewPart implements ISelectionChangedListener, IPar
 				xwtFile.createNewFile();
 			}
 
-			OutputStream os = new FileOutputStream(xwtFile);
-			try {
-				Map<Object, Object> options = new HashMap<Object, Object>();
+			Resource widgetResource = section.getWidget().eResource();
+			final boolean wasModified = widgetResource.isModified();
+			try (OutputStream os = new FileOutputStream(xwtFile)) {
+				Map<Object, Object> options = new HashMap<>();
 				// The outputstream cannot be formatted. If format is true, this is
 				// the real file (and not the preview file) that will be formatted
 				options.put(XWTResource.OPTION_FORMAT, false);
@@ -271,7 +274,8 @@ public class Preview extends ViewPart implements ISelectionChangedListener, IPar
 				section.getWidget().eResource().save(os, options);
 				return xwtFile.toURI().toURL();
 			} finally {
-				os.close();
+				// Restore the modified state that bug 574094 relies on
+				widgetResource.setModified(wasModified);
 			}
 		} catch (IOException ex) {
 			Activator.log.error(ex);
@@ -337,7 +341,7 @@ public class Preview extends ViewPart implements ISelectionChangedListener, IPar
 		}
 
 		displayEngine = new DefaultDisplayEngine();
-		Map<Tab, Composite> tabs = new HashMap<Tab, Composite>();
+		Map<Tab, Composite> tabs = new HashMap<>();
 
 		contents = new CTabFolder(parent, SWT.NONE);
 		contents.setLayoutData(new GridData(SWT.FILL, SWT.FILL, true, true));
@@ -390,7 +394,7 @@ public class Preview extends ViewPart implements ISelectionChangedListener, IPar
 
 	private Collection<Tab> getTabs(View view) {
 
-		List<Tab> tabs = new LinkedList<Tab>();
+		List<Tab> tabs = new LinkedList<>();
 
 		for (Section section : view.getSections()) {
 			Tab tab = section.getTab();