Community
Participate
Working Groups
I've created a diagram that represents the same model element both as a connection and an edit part. When I do that, the code fails with a ClassCastException. See below. The issue is that the following code assumes that if an edit part exists for a model in the same registry, there will be one and only one and that if it exists it will be a connection. This is a reasonable enough assumption in the primary case, though of course there are cases where multiple edit parts exist for the same model object, and this needs to be over-riden for the specific edit part. However, it seems like it would be straightforward distinguish for the case of connections, perhaps by storing them in their another registry. There might even be performance advantages to doing so. protected ConnectionEditPart createOrFindConnection(Object model) { ConnectionEditPart conx = (ConnectionEditPart) getViewer() .getEditPartRegistry().get(model); if (conx != null) return conx; return createConnection(model); } java.lang.ClassCastException: com.tasktop.internal.sync.architect.editparts.ConnectedHostEditPart cannot be cast to org.eclipse.gef.ConnectionEditPart at org.eclipse.gef.editparts.AbstractGraphicalEditPart.createOrFindConnection(AbstractGraphicalEditPart.java:336) at org.eclipse.gef.editparts.AbstractGraphicalEditPart.refreshSourceConnections(AbstractGraphicalEditPart.java:696) at org.eclipse.gef.editparts.AbstractGraphicalEditPart.refresh(AbstractGraphicalEditPart.java:645) at org.eclipse.gef.editparts.AbstractEditPart.addNotify(AbstractEditPart.java:253) at org.eclipse.gef.editparts.AbstractGraphicalEditPart.addNotify(AbstractGraphicalEditPart.java:223) at org.eclipse.gef.editparts.AbstractEditPart.addChild(AbstractEditPart.java:212) at org.eclipse.gef.editparts.AbstractEditPart.refreshChildren(AbstractEditPart.java:781)
I assume that quite a few clients actually rely on this "reverse lookup" mechanism, so if we split the registry that could well break some of them. And I also fear that that will be hard to achieve without breaking API (although I will investigate it). As it further not not handles the case where there are two nodes or two connections representing the same model element, another option could be to change the way edit parts get registered. That is, by default they could still get registered via their model, but alternatively they could provide something that is "comparable as well as adaptable to their model" as a key for the registry instead. But I am not sure if we can achieve this without breaking API either...
(In reply to comment #1) > I assume that quite a few clients actually rely on this "reverse lookup" > mechanism, so if we split the registry that could well break some of them. And > I also fear that that will be hard to achieve without breaking API (although I > will investigate it). I'm not sure if this is a general enough usage to warrant going to deep into right now. I'd be interested to hear if other people have come across it. > > As it further not not handles the case where there are two nodes or two > connections representing the same model element, another option could be to > change the way edit parts get registered. That is, by default they could still > get registered via their model, but alternatively they could provide something > that is "comparable as well as adaptable to their model" as a key for the > registry instead. But I am not sure if we can achieve this without breaking API > either... That's a nice idea. Right now you can do that through the existing edit part factory, but if there are then multiple copies; say one finds an object through it's adapted class and then through the object itself, that would actually fail under the existing scheme. It's just occurred to me that what's really happening here is a kind of asymmetry between the EditPartFactory contract and what's happening on the actual edit part management implementation: The EditPartFactory provides for a context of the parent model object in order to qualify where the object is being used. And that works fine within a hierarchy of Editor nodes, because a given Model Object / Edit Part mapping is owned by the edit part itself; the edit part is the context. So that's why multiple modeled objects can appear as *nodes* within the model in two separate places. The reason that this becomes a problem for Connections is that connections don't really "belong" purely to one edit part; they span edit parts up to and including the scope of the entire diagram. And their end points can cross hierarchy boundaries. This means that essentially the context scope that EditPartFactories are providing through their interface are effectively being ignored in the case of connections. Of course, its likely as you say that clients have come to rely perhaps inadvertently on this behavior, but ignoring that issue a solution could be to actually use a context and model object class as they key for any contained connections.
Miles, that's a good point. Using the model and the context as the default key could indeed be an option. I took this as a cause to look into the Javadoc of EditPartViewer#getEditPartRegistry(), which reads as follows: /** * Returns the {@link Map} for registering <code>EditParts</code> by * <i>Keys</i>. EditParts may register themselves using any method, and may * register themselves with multiple keys. The purpose of such registration * is to allow an EditPart to be found by other EditParts, or by listeners * of domain notifiers. By default, EditParts are registered by their model. * <P> * Some models use a "domain" notification system, in which all changes are * dispatched to a single listener. Such a listener might use this map to * lookup editparts for a given model, and then ask the editpart to update. * * @return the registry map */ Seems to be that the initial intention was that the framework itself should not rely on the edit part registry , but to use it as an application specific means to access edit parts. However, some parts of the framework (CreationTool, SelectionSynchronizer, AbstractEditPart#createOrFindConnection, and TemplateTransferDropTargetListener) actually seem to be hard-wired on expecting the model to be used as a key and would have to be adopted accordingly.
(In reply to comment #3) > However, some parts of the framework (CreationTool, > SelectionSynchronizer, AbstractEditPart#createOrFindConnection, and > TemplateTransferDropTargetListener) actually seem to be hard-wired on expecting > the model to be used as a key and would have to be adopted accordingly. In fact, looking at it, the framework itself *does* assume that the key itself be the model: AbstractEditPart: protected void unregisterModel() { Map registry = getViewer().getEditPartRegistry(); if (registry.get(getModel()) == this) registry.remove(getModel()); } This means that the semantics don't actually match..I think. So you could: 1. Create a Node Edit Part for an object within one context / Edit Part parent. 2. Create a Node Edit Part for that same object within another context / Edit Part parent. 3. Remove the Node Edit Part in 1. 4. Now, the model is effectively removed from the Diagram but the edit parts for 2 are left dangling. I'd never really followed the logic of this through before, but I think this is *exactly* the source of a mysterious issue I keep having. Especially, things work great for me for pure EditPart constructions, but then failing in mysterious ways when I began to use other features. :) What I end up doing is forcing a rebuild of the entire model. It could be really inefficient for a naive algorithm, but what it seems like what the edit part finding mechanism should be doing is effectively traversing the hierarchy of edit parts against both the context and model. Again, that's really what GEF is advertising but it's not what the mechanism seems to be doing. Really the idea of having the EditPartRegistry at all seems suspicious to me, as you're relying on the EditPart implementations to maintain it properly.
(In reply to comment #4) > (In reply to comment #3) > > However, some parts of the framework (CreationTool, > > SelectionSynchronizer, AbstractEditPart#createOrFindConnection, and > > TemplateTransferDropTargetListener) actually seem to be hard-wired on expecting > > the model to be used as a key and would have to be adopted accordingly. > > In fact, looking at it, the framework itself *does* assume that the key itself > be the model: > > AbstractEditPart: > > protected void unregisterModel() { > Map registry = getViewer().getEditPartRegistry(); > if (registry.get(getModel()) == this) > registry.remove(getModel()); > } > > This means that the semantics don't actually match..I think. So you could: > > 1. Create a Node Edit Part for an object within one context / Edit Part parent. > 2. Create a Node Edit Part for that same object within another context / Edit > Part parent. > 3. Remove the Node Edit Part in 1. > 4. Now, the model is effectively removed from the Diagram but the edit parts > for 2 are left dangling. > > I'd never really followed the logic of this through before, but I think this is > *exactly* the source of a mysterious issue I keep having. Especially, things > work great for me for pure EditPart constructions, but then failing in > mysterious ways when I began to use other features. :) What I end up doing is > forcing a rebuild of the entire model. It could be really inefficient for a > naive algorithm, but what it seems like what the edit part finding mechanism > should be doing is effectively traversing the hierarchy of edit parts against > both the context and model. Again, that's really what GEF is advertising but > it's not what the mechanism seems to be doing. Really the idea of having the > EditPartRegistry at all seems suspicious to me, as you're relying on the > EditPart implementations to maintain it properly. Yes, I agree, seems to be inconsistent. Changing the registration to use the context and model seems to be adequate, as by this the edit part lookup process would perfectly correspond to the creation process.
An additional inconsistency is the fact that the LayerManager is stored and retrieved via the EditPartRegistry map as well. Not looking into any API compatibility constraints, I think the best would probably be to introduce a dedicated EditPartRegistry, which could enforce the contract of registration by context and model (similar to the EditPartFactory), and to refactor the layer manager access to not use this registry.
(In reply to comment #6) > An additional inconsistency is the fact that the LayerManager is stored and > retrieved via the EditPartRegistry map as well. > > Not looking into any API compatibility constraints, I think the best would > probably be to introduce a dedicated EditPartRegistry, which could enforce the > contract of registration by context and model (similar to the EditPartFactory), > and to refactor the layer manager access to not use this registry. Yes, I agree. That seems the least intrusive way to handle it and the most supportive of current (perhaps then deprecated) API uses. And simply making it clear that implementations of edit parts (I doubt many people have actually implemented EditPart independently.. :O) are responsible for maintaining the registry and preserving context information. Really the generic case of assuming one-to-one model to edit part mapping is by far the common case, but then why support it in the factory and ignore elsewhere? (BTW, my example above had the order of removal backward, but the basic issue holds.)
