Move up null check in GEF viewer when the selected editpart is updated. #442
Conversation
Removes the usage of raw types as much as possible and simplifies the selection algorithm.
ptziegler
changed the title
|
The problem can be observed in e.g. the following lines of If the editpart is null, it is added to the internal list before an exception is thrown. When the selection is changed, an exception is thrown again, when the null-selection is removed again. I don't see how this can be the desired behavior and I don't believe this is something clients should rely on. |
| EditPart part = itr.next(); | ||
| if (part == null) { | ||
| LOGGER.error(GEFMessages.SelectionManager_Selection_Is_Null); | ||
| continue; |
There was a problem hiding this comment.
Not really happy about this one, as it might cause issue when the last element is null and therefore SELECTED_PRIMARY is never set.
There was a problem hiding this comment.
how about adding after the loop an
if(!selection.isEmpty()){
selection.getLast().setSelected(EditPart.SELECTED_PRIMARY);
}
There was a problem hiding this comment.
After sleeping about it, I'm not quite sure if it's really a good idea to just not throw the exception anymore. I think recent commits already showed that seemingly safe changes can break someones editor.
Instead, it makes more sense to throw the exception before the internal selection is updated. Just so that everything remains consistent.
There was a problem hiding this comment.
I think the new tests illustrate the desired behavior: The list of selected edit parts should never contain null.
There was a problem hiding this comment.
As I had last week a bug in 4diac IDE where I got an NPE at that point I agree with you. The NPE showed me an important bug in our code which otherwise would have gone unnoticed and with no user feedback. Would have been hard to notice and debug. Therefore I think your proposed improvement goes in the right direction.
ptziegler
changed the title
The selection inside a GEF viewer is assumed to always be non-null. But because the (implicit) null check is done after the editpart is added to the internal list, it leaves the viewer in an inconsistent state. To avoid this, the null check is done earlier, so that the exception is thrown before the internal model is modified. Resolves eclipse#433
The selection inside a GEF viewer is assumed to always be non-null. But
because the (implicit) null check is done after the editpart is added to
the internal list, it leaves the viewer in an inconsistent state.
To avoid this, the null check is done earlier, so that the exception is
thrown before the internal model is modified.