Bug 333865

Comment 1 Alexander Nyßen 2011-01-10 11:45:57 EST

As part of bug #330859 I changed the body of AbstractEditPart#isSelectable() to return true iff the EditPart is active. This causes problems in your scenario, because deselection is performed within removeNotify, and deactivation is performed in advance. I would tend to move the deselection code from removeNotify to deactivate, but I fear this might introduce regressions. As such, I have removed the change and isSelectable() within AbstractEditPart returns true by default.

Committed changes to cvs HEAD (3.7).

Comment 2 Romain Raugi 2011-01-11 02:53:27 EST

Hi Alex,

The Assert itself can be a problem. In the Graphiti framework for example, AbstractEditPart.isSelectable() is overridden and returns false if the object is being removed. It demonstrates that the Assert can break the compatibility.

What do you plan? If you keep the Assert, I can create a BR for Graphiti.

Thanks,

Romain

Comment 3 Alexander Nyßen 2011-01-11 05:26:43 EST

(In reply to comment #2)
> Hi Alex,
> 
> The Assert itself can be a problem. In the Graphiti framework for example,
> AbstractEditPart.isSelectable() is overridden and returns false if the object
> is being removed. It demonstrates that the Assert can break the compatibility.
> 
> What do you plan? If you keep the Assert, I can create a BR for Graphiti.
> 
> Thanks,
> 
> Romain

I have added assertions to setSelected() and setFocus() to ensure only selectable edit parts may actually be selected or obtain focus (deselection or loosing focus is possible independent of this), which I think is reasonable and seems to always have been part of API contract, but - up to now - has not been enforced in the code. 

Rather than removing those assertions, I would tend to identify those places where this API contract is (implicitly) broken by the framework itself or may be broken by clients, and to align them with the API contract. We can always remove the assertions as the last resort. What incompatibility problem does Graphiti actually run into?

Comment 4 Romain Raugi 2011-01-11 07:48:19 EST

Graphiti shape edit parts override GEF edit parts through their ShapeEditPart implementation, which inherits from AbstractGraphicalEditPart. They have overridden isSelectable(). This implementation returns false when the edit part is being removed (an EMF object is not part of a resource anymore and a test returns false).

I've not tested GMF with GEF 3.7, but I see that isSelectable() can also return false in their GraphicalEditPart implementation.

It can become an incompatibility if you force client/other frameworks code to be aligned with the API contract.

Comment 5 Alexander Nyßen 2011-01-11 08:12:06 EST

(In reply to comment #4)
> Graphiti shape edit parts override GEF edit parts through their ShapeEditPart
> implementation, which inherits from AbstractGraphicalEditPart. They have
> overridden isSelectable(). This implementation returns false when the edit part
> is being removed (an EMF object is not part of a resource anymore and a test
> returns false).

Well, actually the method is there to enable clients to do exactly this. It's perfectly ok to return false there, as long as it is not also attempted to select such an edit part by calling setSelected with a selection state of SELECTED or SELECTED_PRIMARY. 

> 
> I've not tested GMF with GEF 3.7, but I see that isSelectable() can also return
> false in their GraphicalEditPart implementation.

As said, the question would be if parts of the GEF framework itself or any of the client frameworks (GMF, Graphiti) forces a selection on an unselectable edit part.

> 
> It can become an incompatibility if you force client/other frameworks code to
> be aligned with the API contract.

I thought that would be the sense of an API contract...

Comment 6 Alexander Nyßen 2011-01-11 13:38:50 EST

I made SelectionManager#deselect robust against the fact that the selection list may contain non-selectable edit parts at the point in time the method gets called. This way, the reported problem should not even occur, if clients decide to overwrite isSelectable() to bind selectability to the edit part's activation state.

The change was committed to cvs HEAD (3.7). The method is now implemented as follows:

public void deselect(EditPart editpart) {
		editpart.setSelected(EditPart.SELECTED_NONE);

		selection.remove(editpart);
		if (!selection.isEmpty()) {
			// IMPORTANT: it may (temporarily) happen that the selection list
			// contains edit parts, which are not selectable (any more) when
			// this method gets called. Consider e.g. that the selectable state
			// of an edit part may bound to its activation state (by overwriting
			// isSelectable()); in this case, when deleting a selected edit part
			// and its primary selected child simultaneously, the parent edit
			// part may have already become non selectable, while not having
			// been deselected yet (because deselection is performed within
			// removeNotify() after deactivation), when the child edit part gets
			// deselected. Therefore, we do not simply choose the last edit part
			// in the list as the new primary selection, but reverse-search the
			// list for the first that is (still) selectable.
			for (int i = selection.size() - 1; i >= 0; i--) {
				EditPart primaryCandidate = (EditPart) selection.get(i);
				if (primaryCandidate.isSelectable()) {
					primaryCandidate.setSelected(EditPart.SELECTED_PRIMARY);
					break;
				}
			}
		}
		fireSelectionChanged();
	}

Comment 7 Romain Raugi 2011-01-12 02:59:09 EST

Tested with Graphiti, it works fine.

Thanks,

Romain