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

Bug 485201

Summary: Cancel button in "Ask via popup" strategy opens the selected editor
Product: [Eclipse Project] Platform Reporter: Kaloyan Raev <kaloyan>
Component: UIAssignee: Patrik Suzzi <psuzzi>
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: P3 CC: gautier.desaintmartinlacaze, Lars.Vogel, loskutov, mistria, psuzzi
Version: 4.6   
Target Milestone: 4.6 M5   
Hardware: All   
OS: All   
See Also: https://git.eclipse.org/r/63820
https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=fed29d30e755af1480939dc62a117c13073ab4c8
https://bugs.eclipse.org/bugs/show_bug.cgi?id=486665
Whiteboard:

Description Kaloyan Raev CLA 2016-01-05 05:23:09 EST 
I am testing the new feature of bug 90292 and I found a small issue.

Build id: I20151229-0800

Steps to reproduce:
1. Open the General > Editors > File Associations preference page.
2. Switch to the "Ask via pop-up" strategy for opening unknown files.
3. Click the OK button to close the page.
4. Double click on a file with unknown type. 
5. Select an editor in the Editor Selection dialog.
6. Click the Cancel button.

Executing step 6 closes the Editor Selection dialog and opens the file in the selected editor.

My expectation is that the file is not open in an editor.
Comment 1 Patrik Suzzi CLA 2016-01-08 02:26:20 EST 
The problem here is AskUserViaPopupUnknownEditorStrategy#getEditorDescriptor always return an editor: 

	dialog.open(); 
	// <1>
	return dialog.getSelectedEditor();

the caller is IDE#getEditorDescriptor. 
It always expects an editor, and when the editor it is null, 
it shows the user an Exception 


	IEditorDescriptor editorDesc = strategy.getEditorDescriptor(name, editorReg);

	// if no valid editor found, bail out
	if (editorDesc == null) {
		throw new PartInitException(
				IDEWorkbenchMessages.IDE_noFileEditorFound);
	}

So, if in <1> we return null on dialog.cancel
The IDE#getEditorDescriptor will throw an exception
This is not aceptable. 

Any suggestion before I start working on this ?
Comment 2 Mickael Istria CLA 2016-01-08 02:33:51 EST 
Because of how the underlying open action in the IDE class is implemented, we cannot return null as editor here, because the openEditor() operations do not support cancel. So the strategy cannot really support cancel neither.
Some possibilities:
* Hide/Remove Cancel button
* Fallback to the SystemEditor or TextEditor strategy in case Cancel is hit
Comment 3 Patrik Suzzi CLA 2016-01-08 02:56:01 EST 
Codewise, it could be simple to throw a different exception telling the user he didn't choose an editor
Comment 4 Eclipse Genie CLA 2016-01-08 02:58:52 EST 
New Gerrit change created: https://git.eclipse.org/r/63820
Comment 5 Mickael Istria CLA 2016-01-08 03:02:27 EST 
Indeed, but that would have to be propagated to IDE class (where about everything is an immutable API) and to UI then. Because of our concern in backward compatibility, it makes the addition of Exception not such a trivial task.
But if you manage to implement that as you suggest without breaking compatibility, then it's most likely the best implementation.

Another possibility is to stop assuming that openEditor returning null is  an error and to simply not pop-up an error when this happens.
Comment 6 Patrik Suzzi CLA 2016-01-10 03:46:23 EST 
As suggested in Gerrit reviews, 
I added the strategy a method to return IStatus. 
Now IDE can discriminate the state based on IStatus and, in case of error, it shows a specific exception message when the user cancels.
The exception is the same type of exception used in case the Editor is null, but the descriptive text explains that the user is responsible for mis-selection

Note the bug applies to two cases, and the solution works for both:
- when user opens a file with unknown extension, and cancels the selection
- when the user creates a new file with unknown extension
Comment 8 Andrey Loskutov CLA 2016-01-21 07:44:03 EST 
I've updated interface to document that the implementers can throw either Core or OperationCancelled exceptions. The merged fix respects user cancellation in the editor selection dialog: no editor is opened on cancel.