Comment 1 Tomasz Zarna 2009-01-19 12:29:08 EST

Strange, I don't remember anything I've committed recently that could cause this (last commits in this area are: bug 260362 and bug bug 259719). I guess it's not a permutation of bug 260374, but will check it tomorrow morning.

Comment 2 Dani Megert 2009-01-20 04:31:32 EST

Created attachment 123053 [details]
test workspace

Comment 3 Dani Megert 2009-01-20 04:45:08 EST

The bugs seems to depend on either the concrete code example or some settings in my workspace.

Here are reproducible steps:
1. extract the attached workspace to c:\workspaces\
2. start Eclipse with that workspace (c:\workspaces\eoe)
3. select C.java in the Package Explorer
   ==> History view gets filled
4. in History view select the second entry (the one just below 20.1.2009 10:21)
5. context menu > Compare Current with Local
6. press key 'a' to make the file dirty
7. press Ctrl+S
==> nothing happens.

In the right (read-only) side, Ctrl+S works.

It looks like an activation issue: when I then focus e.g. the Outline or the Package Explorer (just by clicking on the header) and go back to the Java compare editor by clicking on the editor tab, I get Ctrl+S working.

Comment 4 Dani Megert 2009-01-20 05:17:47 EST

It got broken by our enhanced Java compare work (with I20090114-1322).

The reason why we didn't see it is because we have enabled General > Always run in background or use single-click mode. In that cases the compare editor doesn't get the focus hence we give the focus manually and the problem doesn't surface.

Now, the case of double-click + running in background not giving focus to the editor is another bug we introduced :-(, filed bug 261613 to track this.

Comment 5 Boris Bokowski 2009-01-22 09:27:16 EST

Yes, I think I am seeing exactly this bug. Whenever Ctrl+S is not working, I can make it work by activating a view and then going back to the compare editor.

Comment 6 Boris Bokowski 2009-01-28 10:13:35 EST

My guess is that this is going to affect many users who choose to pick up M5. Is there no workaround that you could put in for M5? (e.g. detect when the user hit Ctrl+S but no save happened and trigger it yourself?)

Comment 7 Dani Megert 2009-01-28 10:24:00 EST

Tomasz, I agree with Boris.

Comment 8 Tomasz Zarna 2009-01-29 04:15:56 EST

Guys, you're right, but so far the only workaround I have found is fixing bug 261613 by setting the focus on Compare Editor without checking if it's active.

Comment 9 Tomasz Zarna 2009-01-29 05:36:36 EST

Referring to comment 3: The issue here might be caused by the fact that two SaveActions created for both embedded Java editors get initialised in disabled state (which is fine in this case), but when editor gets dirty they are not enabled. This could be related to the fact that SaveAction extends Part/PageEventAction which reacts to part/page activation, things we are missing (imitate) in embedded Java editors in Compare. 

Fixing bug 261613 may help here but I'm afraid it won't fix the issue we're dealing here with.

Comment 10 Tomasz Zarna 2009-01-29 06:45:21 EST

Created attachment 124139 [details]
Workaround v01

The workaround suggested by Boris in comment 6. Please note, I'm not sure if this is the best way of catching the SaveAction's accelerator.

Comment 11 Tomasz Zarna 2009-01-29 06:45:28 EST

Created attachment 124140 [details]
mylyn/context/zip

Comment 12 Tomasz Zarna 2009-01-29 08:10:50 EST

*** Bug 260374 has been marked as a duplicate of this bug. ***

Comment 13 Paul Webster 2009-01-29 09:39:40 EST

Would listening for the org.eclipse.ui.file.save command not work (or is the problem that CTRL+S is not firing the command at all)?  if it is working, you might have to use some of the other methods in the execution listener to catch your usecase.

public void createPartControl(Composite parent) {
  ICommandService cmdService = (ICommandService) 
	getSite().getService(ICommandService.class);
  cmdService.addExecutionListener(new IExecutionListenerWithChecks() {
	public void preExecute(String commandId, ExecutionEvent event) {
		if ("org.eclipse.ui.file.save".equals(commandId)) {
			// save if appropriate
		}
	}

	public void postExecuteSuccess(String commandId, Object returnValue) {
	}

	public void postExecuteFailure(String commandId,
			ExecutionException exception) {
	}

	public void notHandled(String commandId,
			NotHandledException exception) {
	}

	public void notEnabled(String commandId,
			NotEnabledException exception) {
	}

	public void notDefined(String commandId,
			NotDefinedException exception) {
	}
  });
  // create part control
}

Comment 14 Tomasz Zarna 2009-01-29 10:06:57 EST

Thanks Paul, but the problem is that the action is not executed, because it's disabled (see comment 9). Can I use this snippet to check that the action is disabled and when it shouldn't be (editor is dirty, so save should be enabled) force save?

Comment 15 Tomasz Zarna 2009-01-29 10:31:35 EST

Created attachment 124153 [details]
Workaround v02

In the previous comment I meant something like this.

Comment 16 Tomasz Zarna 2009-01-29 11:34:15 EST

The SaveActions added by embedded java editors are not being enabled because they are not listening to property change events on CompareEditor(WorkbenchPart)[1]. The other 5 SaveActions update their state properly.

[1] org.eclipse.ui.part.WorkbenchPart.firePropertyChange(int)

Comment 17 Tomasz Zarna 2009-02-12 06:53:34 EST

Update: When Compare Editor starts up both save actions (from wrapped Java editors) aren't added to its event listener list[1]. The actions are added to the list on part's deactivation, when leaving Compare Editor. The actions are added and activated properly with JavaMergeViewer.setActionsActivated[2] the only issue is that they initially don't react to property changes (comment 16). Once they are added by leaving and then going back to the Java compare editor they work fine (get updated), as observed in comment 3 in last paragraph.

[1] org.eclipse.core.commands.common.EventManager.listenerList, base type of every Editor(Part) including Compare Editor
[2] org.eclipse.jdt.internal.ui.compare.JavaMergeViewer.setActionsActivated(SourceViewer, boolean)

Comment 18 Tomasz Zarna 2009-02-12 07:07:26 EST

The finding above also explains why we didn't see it right away. As Dani said in comment 4, there are cases when "the compare editor doesn't get the focus[1] hence we give the focus manually" thus activate the compare editor and add save actions to the listener list mentioned in comment 17. The question still remains: why the actions are not added to the list when opening the compare editor?

[1] bug 261613

Comment 19 Tomasz Zarna 2009-02-12 07:45:04 EST

Actions are added to a part's listener list (from EventManager) on partOpened or partBroughtToTop events, this unfortunately doesn't apply to the save actions from wrapped Java editors as they are not yet created when both events take place.

Comment 20 Tomasz Zarna 2009-02-23 06:31:51 EST

Created attachment 126439 [details]
Workaround v03

Another workaround, this one activates the Compare Editor part when focus has been successfully set[1]. This seems to solve the problem since on the manually triggered activation the save actions are added to the part's listener list and this is what we were apparently missing[2]. However, the UI ignores redundant calls to activate currently active part, so I had to remove couple of checks to make the workaround work[3].

As it looks as we have pinpointed the issue, the question now is what can we do about it. Reactivating the editor makes sense to me, but I might be wrong as I'm not an expert in this area. I would be very happy to hear what somebody like Boris think about it. Is there a better way to reactivate the part without messing with UI internals?

Please note, that this is just a workaround not a real patch. Furthermore, to make it work you will have to apply "Patch v01" from bug 261613, since it removes the initial-focus issue and the current, ctrl-s problem gets exposed.

[1] see org.eclipse.compare.internal.CompareEditor.setFocus()
[2] comment 19
[3] org.eclipse.ui.internal.PartList.setActivePart(IWorkbenchPartReference),
org.eclipse.ui.internal.PartService.setActivePart(IWorkbenchPartReference), org.eclipse.ui.internal.WorkbenchPage.setActivePart(IWorkbenchPart), org.eclipse.ui.internal.WorkbenchPage.makeActiveEditor(IEditorReference)

Comment 21 Tomasz Zarna 2009-03-03 10:53:48 EST

Created attachment 127332 [details]
Workaround v04

A cleaner solution that on each inner viewer/editor activation updates the save action of the wrapped editor. In fact, this is only needed when setActionsActivated is called for the very first time, ie when a viewer is gaining focus just after the compare editor has been initialized. Further attempts to update the save action will be redundant since the action has been already added to event listener list (see comment 17).

Comment 22 Tomasz Zarna 2009-03-09 10:27:00 EDT

(In reply to comment #21)
> Further attempts to update the save action will be redundant since the action has been
> already added to event listener list (see comment 17).

This applies to the line "partEventAction.partDeactivated(compareEditorPart);" which may be omitted, we don't need to manually update the action when deactivating.

Comment 23 Dani Megert 2009-03-09 13:14:20 EDT

Thanks for the patch.
Committed to HEAD.
Available in builds > I20090309-0100.