Bug 329319

Comment 1 Deepak Azad 2010-12-04 07:35:57 EST

(In reply to comment #0)
> 3.7 M3.
> 
> 1. Copy the next line:
> RenameJavaElementAction.run_aroundBody1$advice(RenameJavaElementAction.java:41)

The action does not handle '$' character in the copied string. Will take a look in M5.

Comment 2 Deepak Azad 2011-01-11 04:51:23 EST

Created attachment 186468 [details]
fix+test

With this patch Open from clipboard action works with the following
-org.eclipse.jdt.internal.corext.refactoring.generics.InferTypeArgumentsRefactoring$2.run(InferTypeArgumentsRefactoring.java:197)

-org.eclipse.jdt.internal.corext.refactoring.generics.InferTypeArgumentsRefactoring$2.run

Note that if we use the following snippet
----------------------------------------------------------
package p;
class A {
    void foo() {
        Runnable runnable = new Runnable() {
            public void run() {

            }
        };
    }

    void run() {

    }
}
----------------------------------------------------------
 and Open 'p.A$1.run()' a dialog opens with both the run methods. Earlier no match was found, I think it is better to return an extra match in some cases than none at all.

Comment 3 Dani Megert 2011-01-12 10:23:18 EST

> I think it is better to return an extra match in some cases
> than none at all.
Not if it's that wrong and especially not since the following case still doesn't find a match:

1. paste this:
package pp;
public class A$ {
    void a() {
    }
}
2. copy "pp.A$.a()" to clipboard
3. Navigate > Open from Clipboard

Comment 4 Deepak Azad 2011-01-13 03:43:05 EST

(In reply to comment #3)
> > I think it is better to return an extra match in some cases
> > than none at all.
> Not if it's that wrong and especially not since the following case still
> doesn't find a match:
> 
> 1. paste this:
> package pp;
> public class A$ {
>     void a() {
>     }
> }
> 2. copy "pp.A$.a()" to clipboard
> 3. Navigate > Open from Clipboard

ah well, 'A$' also does not work :)

Based on the discussion in Bug 334119, I assume it is ok if Open from Clipboard does not work for 'A$', no ?

Comment 5 Dani Megert 2011-01-13 03:46:22 EST

> Based on the discussion in Bug 334119, I assume it is ok if Open from Clipboard
> does not work for 'A$', no ?
Wrong. '$' is discouraged but still allowed like default packages.

Comment 6 Dani Megert 2011-01-14 03:36:57 EST

We should do the following:
1. fix the exception
2. make sure it finds the existing types that also Open Type and Search find
3. nice to have: feature request: support to map binary class names

Comment 7 Deepak Azad 2011-01-16 09:20:45 EST

Created attachment 186885 [details]
fix + tests

(In reply to comment #6)
> We should do the following:
> 1. fix the exception
> 2. make sure it finds the existing types that also Open Type and Search find
The action will now find types like 'A$' (example from comment 3). But it will not find methods like p.A$1.run()(example from comment 2), Search also does not find this.

> 3. nice to have: feature request: support to map binary class names
This is a bit more work and should be done as part of a separate bug.

I have also added a test which invokes Open from Clipboard command programmatically. This new test is to be run manually, as a dialog could open as a result of the command and stop further execution of tests. Initially I thought that the dialog can also be closed programmatically but later I realized that when the dialog is opened the control is still in the command code.

Comment 8 Dani Megert 2011-01-17 05:25:01 EST

(In reply to comment #7)
> Created attachment 186885 [details] [diff]
> fix + tests
> 
> (In reply to comment #6)
> > We should do the following:
> > 1. fix the exception
> > 2. make sure it finds the existing types that also Open Type and Search find
> The action will now find types like 'A$' (example from comment 3). But it will
> not find methods like p.A$1.run()(example from comment 2), Search also does not
> find this.
+1. Verified that the fix works and committed it to HEAD (without tests).

> > 3. nice to have: feature request: support to map binary class names
> This is a bit more work and should be done as part of a separate bug.
It's not so important to me. Feel free to file the RFE or not.

> I have also added a test which invokes Open from Clipboard command
> programmatically. This new test is to be run manually, as a dialog could open
> as a result of the command and stop further execution of tests. Initially I
> thought that the dialog can also be closed programmatically but later I
> realized that when the dialog is opened the control is still in the command
> code.
I don't like these too much. Such manual tests tend to stay around and no one will ever run them (and BTW, see also ManualSuite). Note that the test does not say what exactly has to be done manually. Maybe we could restructure the code in OpenFromClipboardAction a bit so that result computation and dialog opening gets separated. We could then at least test the logic of the action by calling those methods.

Comment 9 Dani Megert 2011-01-17 05:27:01 EST

Comment on attachment 186885 [details]
fix + tests

+1 on the "real" code without the tests.

Comment 10 Dani Megert 2011-01-17 05:52:17 EST

(In reply to comment #9)
> Comment on attachment 186885 [details] [diff]
> fix + tests
> 
> +1 on the "real" code without the tests.
Actually one of the two test classes is automated - committed that part to HEAD too.

Comment 11 Deepak Azad 2011-01-24 06:10:17 EST

Created attachment 187408 [details]
tests + tweaks needed for tests

I have tweaked OpenFromClipboardAction a bit to separate the code that actually finds the matches into a static method. This lets us test the whole logic of parsing the input text and then finding corresponding matches. (Of course the user interaction with dialogs is still not tested.)

Dani, the changes look ok to you ?

Comment 12 Dani Megert 2011-01-24 08:53:47 EST

Patch looks good except for a wrong copyright in OpenFromClipboardTests.

Committed to HEAD including fix for Javadoc problem in OpenFromClipboardAction.

Comment 13 Dani Megert 2011-01-25 03:46:36 EST

Verified in I20110124-1800.