Skip to content

Commit

Permalink
Fix FormatPainter not referencing original model object
Browse files Browse the repository at this point in the history
- We need to reference the source object and its parent model in case we are copying images

- When the source model is closed reset the FormatPainterInfo

- See #1075
  • Loading branch information
Phillipus committed Sep 10, 2024
1 parent 99cbef0 commit 0c6ddbf
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 16 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,18 @@

import java.beans.PropertyChangeListener;
import java.beans.PropertyChangeSupport;
import java.util.Objects;

import org.eclipse.swt.graphics.Cursor;
import org.eclipse.swt.graphics.ImageData;
import org.eclipse.swt.graphics.PaletteData;
import org.eclipse.swt.graphics.RGB;

import com.archimatetool.editor.model.IEditorModelManager;
import com.archimatetool.editor.ui.ColorFactory;
import com.archimatetool.editor.ui.IArchiImages;
import com.archimatetool.editor.ui.ImageFactory;
import com.archimatetool.model.IArchimateModel;
import com.archimatetool.model.IDiagramModelComponent;
import com.archimatetool.model.IDiagramModelConnection;
import com.archimatetool.model.IDiagramModelObject;
Expand Down Expand Up @@ -45,7 +48,17 @@ public class FormatPainterInfo {
private Cursor coloredCursor, defaultCursor;
private PropertyChangeSupport listeners = new PropertyChangeSupport(this);

private FormatPainterInfo() {}
private FormatPainterInfo() {
// Listen to a model being closed in case it's the model that we are referencing. If it is, reset.
IEditorModelManager.INSTANCE.addPropertyChangeListener(evt -> {
if(evt.getPropertyName() == IEditorModelManager.PROPERTY_MODEL_REMOVED) {
IArchimateModel model = (IArchimateModel)evt.getNewValue();
if(sourceComponent != null && Objects.equals(model, sourceComponent.getArchimateModel())) {
reset();
}
}
});
}

/**
* Reset source component to null and notify listeners.
Expand Down Expand Up @@ -99,18 +112,8 @@ RGB getCursorColor() {
* Set the source component from which we will copy the formatting
*/
private void setSourceComponent(IDiagramModelComponent component) {
if(component != null) {
// Make a snapshot copy of the source component so we don't reference the original object.
// Before this change we used to hold a reference to the original object
// but if the model was closed we still held a reference to it and it couldn't be garbage collected
// until the user cleared the FormatPainter which they might forget to do.
// Another side effect of that old way was if the user changed an attribute of the source component
// the FormatPainter would now hold that new value which might not be what the user expects.
sourceComponent = (IDiagramModelComponent)component.getCopy();
}
else {
sourceComponent = null;
}
// Reference the original object and its parent model in case we need to copy its images
sourceComponent = component;

cursorColor = null;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNotEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertSame;
Expand Down Expand Up @@ -57,8 +56,7 @@ public void testUpdateWithSourceComponent() throws Exception {
info.updateWithSourceComponent(sourceComponent);

assertNotNull(info.getSourceComponent());
// FormatPaintInfo will make a copy of the source component
assertNotEquals(sourceComponent, info.getSourceComponent());
assertEquals(sourceComponent, info.getSourceComponent());

// Colored cursor
assertSame(info.getCursor(), TestUtils.getPrivateField(info, "coloredCursor"));
Expand Down

0 comments on commit 0c6ddbf

Please sign in to comment.