Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 481234

Summary:	The CommandStack class has to mark some methods as deprecated
Product:	[Tools] GEF	Reporter:	Jose M Beleta <jose>
Component:	GEF-Legacy GEF (MVC)	Assignee:	Alexander Nyßen <nyssen>
Status:	RESOLVED FIXED	QA Contact:
Severity:	normal
Priority:	P3	CC:	jose, nyssen
Version:	3.10.1
Target Milestone:	4.0.0 / 3.11.0 (Neon) M4
Hardware:	All
OS:	All
Whiteboard:

Description Jose M Beleta

2015-11-02 08:03:03 EST

Since 3.2 the org.eclipse.gef.commands.CommandStack class has deprecated the field listener and introduced a new field eventListeners, this time private, and some new methods like notifyListener(Command command, int state).

The problem is that the new methods use the new field, eventListeners, but  still remains some methods that use the old field, addCommandStackListener or removeCommandStackListener for example, without being deprecated.

This lead to runtime misbehaviour when you replace the use of listener by notifyListener(Command command, int state) because notifyListener() is also deprecated and keep using addCommandStackListener and removeCommandStackListener that are not deprecated.

The interface CommandStackListener should also be deprecated because this functionality has been taken by CommandStackEventListener.

Comment 1 Alexander Nyßen

2015-11-02 14:56:23 EST

Well, CommandStackEventListener is not a complete replacement for CommandStackListener, as it will not be notified when flush() or markSaveLocation() is called.

Comment 2 Jose M Beleta

2015-11-03 06:42:29 EST

(In reply to Alexander Nyßen from comment #1)
> Well, CommandStackEventListener is not a complete replacement for
> CommandStackListener, as it will not be notified when flush() or
> markSaveLocation() is called.

Yes, this is in the code but not in the documentation, so I think that this is a bug. Even more notifyListeners() is deprecated what suggests that CommandStackEventListener is the way to go.

Comment 3 Alexander Nyßen

2015-11-03 09:06:15 EST

> Yes, this is in the code but not in the documentation, so I think that this
> is a bug. 

The code is the truth :-). We might need to update the documentation.

> Even more notifyListeners() is deprecated what suggests that
> CommandStackEventListener is the way to go.

I am not sure if this conclusion can really be made here. Deprecating the protected notifyListeners() just means that other framework classes (and subclasses) should not use it to fire notifications. It does not mean that the listener mechanism should no longer be used by clients. 

Anyway, I think we cannot simply deprecate the listener mechanism without providing the same functionality to clients.

Comment 4 Alexander Nyßen

2015-11-21 08:17:26 EST

Pushed the following changes to origin/master:

- Ensured CommandStack now fires PRE_FLUSH, POST_FLUSH, PRE_MARK_SAVE, and POST_MARK_SAVE events via the CommandStackEventListener interface in addition to the command-based events.
- Deprecated CommandStackListener and replaced its usages (except for TreeContentProvider, which is deprecated as well, and GraphicalEditor, which implements the interface directly) with CommandStackEventListener, using a POST_MASK filter.
- Added CommandStackTest to ensure notification events are properly fired.
- Increased version of gef bundles and features to 3.11.0 because API was extended.

Resolving as fixed in 3.11.0 M4.