Bug 349665

Comment 1 Markus Keller 2011-06-24 13:25:48 EDT

+1. I'd call it 'Surround with try/multi-catch'.

Comment 2 Curtis Windatt 2011-06-24 14:34:04 EDT

I was going to file the same request.

Comment 3 Deepak Azad 2011-07-01 08:19:56 EDT

Created attachment 198959 [details]
fix v0.9

The patch adds a new action to 'Surround With' menu - 'Try/multi-catch Block'. The existing 'Try/catch Block' action is untouched. 

Markus, please try out the patch. Is the behavior ok ?

TODO: 
- Clean up javadoc
- Prevent/Disable the action for compliance level < 1.7
- Add tests

Comment 4 Markus Keller 2011-07-01 09:53:18 EDT

Looks good so far.

More TODO:
- make the protected methods in the action classes package-private.
- SurroundWithTryMultiCatchAction#fEditor is unused
- 'Try/multi-catch Block' needs a different mnemonic

- SurroundWithTryCatchRefactoring:
  - getCatchBody("Exception", ..) //TODO
    -> OK for me. I wouldn't go into the LUB business. Alternative would be to spell out the union type as 'Exception1 | Ex2 | Ex3'.

  - instead of:
    String type= fImportRewrite.addImport(exception, context);
better do this:
    Type type= fImportRewrite.addImport(exception, getAST(), context);


After playing with the example below, I think it would be good to have a quick fix 'Surround with try/multi-catch' that shows up in addition to the 'Surround with try/catch' quick fix (only for source >= 1.7 and only if there are multiple exceptions).

    void xxx() /*throws Exception*/ {
    	getClass().getDeclaredMethod("xxx").invoke(this, (Object[]) null);
    }

Comment 5 Deepak Azad 2011-07-02 01:41:27 EDT

Created attachment 198992 [details]
fix + tests

Finished all the TODOs and committed to BETA_JAVA7

(In reply to comment #4)
> After playing with the example below, I think it would be good to have a quick
> fix 'Surround with try/multi-catch' that shows up in addition to the 'Surround
> with try/catch' quick fix (only for source >= 1.7 and only if there are
> multiple exceptions).
> 
>     void xxx() /*throws Exception*/ {
>         getClass().getDeclaredMethod("xxx").invoke(this, (Object[]) null);
>     }
Yeah, makes sense. I will add this.

Comment 6 Deepak Azad 2011-07-02 04:31:19 EDT

Created attachment 198994 [details]
fix + tests - II

Added the quick fix as well.

Comment 7 Deepak Azad 2011-07-02 04:32:13 EDT

.

Comment 8 Markus Keller 2011-07-04 08:37:24 EDT

I think the "Surround with try/multi-catch" quick fix should have higher priority (7), since this is probably used more often than the other one.

The "Source > Surround With > Try/multi-catch Block" menu item should not be shown for source level < 1.7. It's OK to show the message for pre-1.7 projects (e.g. when invoked via key binding), but you could just as well disable the action in that case.

Comment 9 Deepak Azad 2011-07-05 08:49:52 EDT

(In reply to comment #8)
Done.

Comment 10 Markus Keller 2011-07-05 10:54:49 EDT

Created attachment 199133 [details]
Fix III (enablement)

Overriding isEnabled() in SurroundWithTryMultiCatchAction is not good. This could be a performance bottleneck (called too often), and the action is supposed to fire property change events when the enablement changes.

Released the attached fix.

Comment 11 Satyam Kandula 2011-07-19 10:55:27 EDT

(In reply to comment #8)
> I think the "Surround with try/multi-catch" quick fix should have higher
> priority (7), since this is probably used more often than the other one.
> 
"Surround with try/multi-catch" is being proposed after "Surround with try/catch". Am I missing something?

Comment 12 Satyam Kandula 2011-07-19 11:15:57 EDT

(In reply to comment #11)
> (In reply to comment #8)
> > I think the "Surround with try/multi-catch" quick fix should have higher
> > priority (7), since this is probably used more often than the other one.
> > 
> "Surround with try/multi-catch" is being proposed after "Surround with
> try/catch". Am I missing something?

Thanks Deepak for explaining me. I was looking at the "Surround with" context menu and the order is not important there.

Comment 13 Satyam Kandula 2011-07-19 11:16:22 EDT

Verified using patch 1.0.0.v20110714-1400

Comment 14 Dani Megert 2011-07-20 06:24:50 EDT

This does not work for me using 3.7 + 

Paste this:
import java.nio.file.AccessDeniedException;
import java.nio.file.FileAlreadyExistsException;

public class Test {

	void f(int i) {
		if (i == 1)
			throw new FileAlreadyExistsException("");
		else
			throw new AccessDeniedException("");
	}
}
==> I get errors but none of them offers a 'Surround with Try/multi-catch Block' quick fix.

Comment 15 Deepak Azad 2011-07-20 07:51:53 EDT

Dani, please try with this
----------------------------------------------------------------------------
void xxx() /* throws Exception */{
	getClass().getDeclaredMethod("xxx").invoke(this, (Object[]) null);
}
----------------------------------------------------------------------------

(In reply to comment #14)
> ==> I get errors but none of them offers a 'Surround with Try/multi-catch
> Block' quick fix.
There are 2 errors on 2 different statements and both errors are for a single uncaught exception. The quick fix is offered if there is more than one uncaught exception in a single statement. 

You could argue that the quick fix can be smarter and try to find if there are uncaught exceptions in the nearby statements. But I don't think this is a good idea for 2 reasons
- We will also have to do this for 'Surround with Try/multi-catch'. But this quick fix has been around for some time, and people are used to the current behavior.
- What if the developer wants the current behavior i.e. wants to surround only one statement with a try catch block.

The correct solution in this case is for the developer to use 'Source > Surround with > Try/multi-catch block' :)

Comment 16 Dani Megert 2011-07-20 08:01:18 EDT

(In reply to comment #15)
Point taken!