Revision: 3479
Author: [email protected]
Date: Thu Apr 29 08:28:11 2010
Log: NEW - bug 2765: NPE when dding a column to a table that had previously
been deleted
http://trillian.sqlpower.ca/bugzilla/show_bug.cgi?id=2765
The root cause of this bug was the listener on the model from the table
pane was being removed, then when it was revived
from the play pen's graveyard it was no longer listening to its model.
A side effect of this fix is the DBTreeModel's listener needed to be
corrected to not pool events. This is because fixing
the listener caused child columns to not be removable from the tables on
undo. To populate SQLObjects it now works closer
to the original implementation which added all of the objects and set
populate flags, then fired events. To make updates
from the server work a special case had to be added in to populate the
SQLObjects in the same fashion that they get
populated in the stand-alone client.
http://code.google.com/p/power-architect/source/detail?r=3479
Modified:
/trunk/regress/ca/sqlpower/architect/swingui/ArchitectSwingSessionImplTest.java
/trunk/regress/ca/sqlpower/architect/swingui/action/AlignTableActionTest.java
/trunk/src/main/java/ca/sqlpower/architect/ArchitectProject.java
/trunk/src/main/java/ca/sqlpower/architect/swingui/ColumnMappingPanel.java
/trunk/src/main/java/ca/sqlpower/architect/swingui/PlayPenContentPane.java
/trunk/src/main/java/ca/sqlpower/architect/swingui/TablePane.java
/trunk/src/main/java/ca/sqlpower/architect/swingui/dbtree/DBTreeModel.java
=======================================
---
/trunk/regress/ca/sqlpower/architect/swingui/ArchitectSwingSessionImplTest.java
Tue Apr 6 14:44:35 2010
+++
/trunk/regress/ca/sqlpower/architect/swingui/ArchitectSwingSessionImplTest.java
Thu Apr 29 08:28:11 2010
@@ -275,7 +275,6 @@
final ArchitectSwingSession session = context.createSession(false);
SQLDatabase db = new SQLDatabase();
session.setSourceDatabaseList(Collections.singletonList(db));
- session.getRootObject().addChild(session.getTargetDatabase());
assertEquals(session.getRootObject(),
session.getTargetDatabase().getParent());
final SQLTable sourceTable = new SQLTable(db, true);
=======================================
---
/trunk/regress/ca/sqlpower/architect/swingui/action/AlignTableActionTest.java
Thu Jan 29 12:02:55 2009
+++
/trunk/regress/ca/sqlpower/architect/swingui/action/AlignTableActionTest.java
Thu Apr 29 08:28:11 2010
@@ -92,7 +92,6 @@
}
public void testSelectSomeAlignTableHori(){
- pp.addTablePane(tp3,new Point(21,43));
List<SQLObject> selections = new ArrayList<SQLObject>();
selections.add(t1);
selections.add(t3);
@@ -110,7 +109,6 @@
}
public void testSelectSomeAlignTableVert(){
- pp.addTablePane(tp3,new Point(21,43));
List<SQLObject> selections = new ArrayList<SQLObject>();
selections.add(t1);
selections.add(t3);
=======================================
--- /trunk/src/main/java/ca/sqlpower/architect/ArchitectProject.java Tue
Apr 27 13:05:36 2010
+++ /trunk/src/main/java/ca/sqlpower/architect/ArchitectProject.java Thu
Apr 29 08:28:11 2010
@@ -99,13 +99,22 @@
unpopulateTreeMap(e.getChild());
}
- private void populateTreeMap(SPObject addedChild) {
+ private void populateTreeMap(SPObject addedChild) {
if (projectMap.put(addedChild.getUUID(), addedChild) != null) {
throw new IllegalStateException("Object added under
project with same UUID!");
}
addedChild.addSPListener(this);
- for (SPObject o : addedChild.getChildren()) {
- populateTreeMap(o);
+ //Be careful here. If calling getChildren adds children to the
object this
+ //listener will get called twice, once because the listener is
now on the parent
+ //and again for the loop.
+ if (addedChild instanceof SQLObject) {
+ for (SPObject o : ((SQLObject)
addedChild).getChildrenWithoutPopulating()) {
+ populateTreeMap(o);
+ }
+ } else {
+ for (SPObject o : addedChild.getChildren()) {
+ populateTreeMap(o);
+ }
}
}
@@ -115,8 +124,17 @@
"removed child's entry in map was either null, or
different object.");
}
removedChild.removeSPListener(this);
- for (SPObject o : removedChild.getChildren()) {
- unpopulateTreeMap(o);
+
+ //Removing a listener should not cause a SQLObject to populate
but we need to
+ //remove the listener from all descendants and clear the map .
+ if (removedChild instanceof SQLObject) {
+ for (SPObject o : ((SQLObject)
removedChild).getChildrenWithoutPopulating()) {
+ unpopulateTreeMap(o);
+ }
+ } else {
+ for (SPObject o : removedChild.getChildren()) {
+ unpopulateTreeMap(o);
+ }
}
}
};
=======================================
---
/trunk/src/main/java/ca/sqlpower/architect/swingui/ColumnMappingPanel.java
Thu Apr 22 12:03:52 2010
+++
/trunk/src/main/java/ca/sqlpower/architect/swingui/ColumnMappingPanel.java
Thu Apr 29 08:28:11 2010
@@ -332,6 +332,8 @@
PlayPen pp = new PlayPen(session);
lhsTable = new TablePane(r.getPkTable(), pp.getContentPane());
rhsTable = new TablePane(r.getFkTable(), pp.getContentPane());
+ pp.getContentPane().addChild(lhsTable, 0);
+ pp.getContentPane().addChild(rhsTable, 0);
SQLPowerUtils.listenToHierarchy(lhsTable.getModel().getParent(),
this);
// The playpen constructor hooks the playpen in as a hierarchy
listener
@@ -415,13 +417,21 @@
} catch (SQLObjectException e) {
throw new RuntimeException(e);
} finally {
- cleanup();
+ try {
+ cleanup();
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
}
return true;
}
public void discardChanges() {
- cleanup();
+ try {
+ cleanup();
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
}
public JComponent getPanel() {
@@ -435,11 +445,13 @@
/**
* Detaches listeners from the SQLTable and SQLRelationship that were
added
* during the creation of this instance.
+ * @throws ObjectDependentException
+ * @throws IllegalArgumentException
*/
- private void cleanup() {
+ private void cleanup() throws IllegalArgumentException,
ObjectDependentException {
SQLPowerUtils.unlistenToHierarchy(lhsTable.getModel().getParent(),
this);
- lhsTable.destroy();
- rhsTable.destroy();
+ lhsTable.getParent().removeChild(lhsTable);
+ rhsTable.getParent().removeChild(rhsTable);
}
public void childAdded(SPChildEvent e) {
=======================================
---
/trunk/src/main/java/ca/sqlpower/architect/swingui/PlayPenContentPane.java
Thu Apr 22 12:03:52 2010
+++
/trunk/src/main/java/ca/sqlpower/architect/swingui/PlayPenContentPane.java
Thu Apr 29 08:28:11 2010
@@ -235,6 +235,9 @@
if (getPlayPen() != null) {
ppc.addSelectionListener(getPlayPen());
}
+ if (ppc instanceof TablePane) {
+ ((TablePane) ppc).connect();
+ }
fireChildAdded(ppc.getClass(), ppc, pos);
ppc.revalidate();
}
=======================================
--- /trunk/src/main/java/ca/sqlpower/architect/swingui/TablePane.java Tue
Apr 6 14:44:35 2010
+++ /trunk/src/main/java/ca/sqlpower/architect/swingui/TablePane.java Thu
Apr 29 08:28:11 2010
@@ -101,6 +101,18 @@
* A special column index that represents the gap between the PK line
and the first non-PK column.
*/
public static final int COLUMN_INDEX_START_OF_NON_PK = -4;
+
+ /**
+ * This listener will disconnect this pane from the model if the pane
is removed from the
+ * container.
+ */
+ private final AbstractSPListener containerPaneListener = new
AbstractSPListener() {
+ public void childRemoved(SPChildEvent e) {
+ if (e.getChild() == TablePane.this) {
+ destroy();
+ }
+ }
+ };
/**
* This is the column index at which to the insertion point is
@@ -205,6 +217,26 @@
}
// ---------------------- utility methods ----------------------
+
+ /**
+ * You must call this method when you are adding a TablePane component
after
+ * the parent is defined. It will register the necessary listeners to
all
+ * necessary parties.
+ * <p>
+ * TODO : When there are multiple {...@link PlayPenContentPane}s each
+ * {...@link PlayPenComponent} will need to be able to re-connect to its
model
+ * when it is added back into its parent content pane. At this point
the
+ * interface will need a connect method and this will be implementing
that
+ * connect method.
+ */
+ public void connect() {
+ //Disconnect first in case the object is already connected. This
ensures
+ //a listener isn't addd twice.
+ destroy();
+
+ SQLPowerUtils.listenToHierarchy(model, columnListener);
+ getParent().addSPListener(containerPaneListener);
+ }
/**
* You must call this method when you are done with a TablePane
@@ -218,6 +250,7 @@
*/
public void destroy() {
SQLPowerUtils.unlistenToHierarchy(model, columnListener);
+ getParent().removeSPListener(containerPaneListener);
}
// -------------------- sqlobject event support ---------------------
@@ -368,14 +401,7 @@
logger.error("Error getting children on new model", e);
//$NON-NLS-1$
}
- SQLPowerUtils.listenToHierarchy(model, columnListener);
- getParent().addSPListener(new AbstractSPListener() {
- public void childRemoved(SPChildEvent e) {
- if (e.getChild() == TablePane.this) {
- destroy();
- }
- }
- });
+ connect();
}
@Override
=======================================
---
/trunk/src/main/java/ca/sqlpower/architect/swingui/dbtree/DBTreeModel.java
Wed Apr 14 07:49:36 2010
+++
/trunk/src/main/java/ca/sqlpower/architect/swingui/dbtree/DBTreeModel.java
Thu Apr 29 08:28:11 2010
@@ -229,18 +229,6 @@
}
}
- /**
- * List to hold all of the events. The events are to be pooled
during a
- * transaction and only acted upon when the transaction is
complete.
- * This gives methods like populate the ability to fire multiple
child
- * events but not cause the tree model to get the children of the
- * objects it is looking at which would cause populate to start
over
- * again when it is already in the middle of populating.
- */
- private List<TreeEventWithType> allEvents = new
ArrayList<TreeEventWithType>();
-
- private int transactionCount = 0;
-
public void childAdded(SPChildEvent e) {
if
(!SQLPowerUtils.getAncestorList(e.getSource()).contains(root)
&& !e.getSource().equals(root)) return;
@@ -258,11 +246,7 @@
Set<TreeModelEvent> events = createTreeEvents(e);
for (TreeModelEvent evt : events) {
- if (transactionCount > 0) {
- allEvents.add(new TreeEventWithType(evt,
EventType.INSERT));
- } else {
- fireTreeNodesInserted(evt);
- }
+ fireTreeNodesInserted(evt);
}
if (e.getChild() instanceof SQLTable &&
foldersInTables.get(e.getChild()) == null) {
@@ -275,11 +259,7 @@
}
final TreeModelEvent evt = new TreeModelEvent(table,
getPathToNode(table),
positions, folderList.toArray());
- if (transactionCount > 0) {
- allEvents.add(new TreeEventWithType(evt,
EventType.INSERT));
- } else {
- fireTreeNodesInserted(evt);
- }
+ fireTreeNodesInserted(evt);
} else {
setupTreeForNode((SQLObject) e.getChild());
}
@@ -305,11 +285,7 @@
Set<TreeModelEvent> events = createTreeEvents(e);
for (TreeModelEvent evt : events) {
- if (transactionCount > 0) {
- allEvents.add(new TreeEventWithType(evt,
EventType.REMOVE));
- } else {
- fireTreeNodesRemoved(evt);
- }
+ fireTreeNodesRemoved(evt);
}
}
@@ -317,40 +293,18 @@
if (!root.getRunnableDispatcher().isForegroundThread())
throw new IllegalStateException("Transaction ended for " +
e.getSource() +
" while not on the foreground thread.");
- if (transactionCount == 0) {
- throw new IllegalStateException("Transaction ended outside
of a transaction.");
- }
- transactionCount--;
- if (transactionCount == 0) {
- List<TreeEventWithType> currentEvents = new
ArrayList<TreeEventWithType>(allEvents);
- for (TreeEventWithType evt : currentEvents) {
- if (evt.getType() == EventType.INSERT) {
- fireTreeNodesInserted(evt.getEvt());
- } else if (evt.getType() == EventType.REMOVE) {
- fireTreeNodesRemoved(evt.getEvt());
- } else if (evt.getType() == EventType.CHANGE) {
- fireTreeNodesChanged(evt.getEvt());
- } else {
- throw new IllegalStateException("Unknown event
type " + evt.getType());
- }
- }
- allEvents.clear();
- }
}
public void transactionRollback(TransactionEvent e) {
if (!root.getRunnableDispatcher().isForegroundThread())
throw new IllegalStateException("Transaction rolled back
for " + e.getSource() +
" while not on the foreground thread.");
- transactionCount = 0;
- allEvents.clear();
}
public void transactionStarted(TransactionEvent e) {
if (!root.getRunnableDispatcher().isForegroundThread())
throw new IllegalStateException("Transaction started for "
+ e.getSource() +
" while not on the foreground thread.");
- transactionCount++;
}
public void propertyChanged(PropertyChangeEvent e) {
@@ -405,11 +359,7 @@
logger.info("Changing a UUID. This should only be done
during load.");
} else {
final TreeModelEvent evt = new TreeModelEvent(this,
getPathToNode(source));
- if (transactionCount > 0) {
- allEvents.add(new TreeEventWithType(evt,
EventType.CHANGE));
- } else {
- fireTreeNodesChanged(evt);
- }
+ fireTreeNodesChanged(evt);
}
}
