Bug 276466

Comment 1 Raksha Vasisht 2009-05-15 06:14:21 EDT

Created attachment 135958 [details]
Initial Fix

Comment 2 Raksha Vasisht 2009-08-14 09:38:10 EDT

Created attachment 144529 [details]
Patch on top of latest code

Added the existing check box for All anonymous types to the preferences dialog and new option to add a method as well to the list that is expanded with constructors by default.

Markus, presently we only check for the 'simple name' of a method using the method

boolean typeNameMatches(String nameA, String nameB) . Is it okay to do this? or do we need to match the 'qualified name' of a method too? Because I found a scenario where even if the qualified name of the method is entered wrong , the logic considers the simple name and expands it by default .

Comment 3 Markus Keller 2009-08-25 20:36:45 EDT

Comment on attachment 144529 [details]
Patch on top of latest code

The new dialog looks strange with the left pane that only ever has just one entry. It also makes the impression like it would show a preference page, but the page shown does not (and should not) show up in the main preferences dialog. AFAIK, it's not possible to show a preferences dialog without the left pane, so you have to take the implementation out of the preference page and put it into your own dialog (similar to how the current dialog does it).


> Markus, presently we only check for the 'simple name' of a method using the
> method
> 
> boolean typeNameMatches(String nameA, String nameB) . Is it okay to do this? or
> do we need to match the 'qualified name' of a method too? Because I found a
> scenario where even if the qualified name of the method is entered wrong , the
> logic considers the simple name and expands it by default .

It wouldn't make sense to treat methods with different qualifiers as the same (likewise with types). If you found a scenario that fails, you should fix the code. Did it already fail before with qualified types? Please post a concrete example so we're sure we talk about the same thing.


Your patch contains some unnecessary changes that you should revert:

* CallHierarchyContentProvider:
- Why did you rename the first field to CALL_HIERARCHY_PREFERENCES? It's still just one preference, and the name should tell what the preference stands for. What's not OK with the original name PREF_DEFAULT_EXPAND_WITH_CONSTRUCTORS?
- PREF_ANONYMOUS_EXPAND_WITH_CONSTRUCTORS is still @since 3.5

* PreferenceConstants:
- the following line got two unnecessary tabs at the end:
store.setDefault(CallHierarchyContentProvider.PREF_ANONYMOUS_EXPAND_WITH_CONSTRUCTORS, true);


Other problems I found:

* CallHierarchyContentProvider:
- With the addition of members, the meaning of CALL_HIERARCHY_PREFERENCES has been expanded, so you should update the Javadoc.

* PreferenceConstants:
- CALL_HIERARCHY_PREFERENCES is not a good name, since it repeats "preferences" (that's already in the class name) and it does not match the naming of existing constants in that class. Furthermore, this is API, so the Javadoc must be of good quality (correct English, describe the value like all other constants, etc.). Note that the string constant is also API, so its name must also be well-chosen ("call_hierarchy_preferences_page", but it's about default ExpandWithConstructors).

* CallHierarchyViewPart:
- "PreferencesActionGroup fPreferences;": You should mimic the naming of fields in the vicinity. This field does not hold preferences. Choose a name that conveys the full meaning and that is similar to the existing action group fields.

* Why do you duplicate CALL_HIERARCHY_PREFERENCES in CallHierarchyContentProvider and in PreferenceConstants? You should only store preferences under one key, and I suggest you use the old key we had in 3.5, so you don't have to write migration code.

* The "Preferences" menu item should keep its old name as long as we didn't move all preferences into that dialog. (And as "Preferences", it would also have needed a mnemonic).

* NLS messages:
- Don't put the new messages into PreferencesMessages.properties. The call hierarchy preferences are local to the view, so their messages should also stay in CallHierarchyMessages.properties.
- The new description label in the preference dialog is hard to read. It misses a space between the sentences, and it's confusing that the first sentence is a complete sentence, but the second one is not. I also wouldn't cite the action name "Expand With Constructors" but refer to the functionality in prose (that's what we also do in other preference pages, and what the text in 3.5 did).

Comment 4 Raksha Vasisht 2009-09-04 11:28:22 EDT

Created attachment 146514 [details]
Updated patch with dialog instead of preference page

(In reply to comment #3)
> (From update of attachment 144529 [details])
> The new dialog looks strange with the left pane that only ever has just one
> entry. It also makes the impression like it would show a preference page, but
> the page shown does not (and should not) show up in the main preferences
> dialog. AFAIK, it's not possible to show a preferences dialog without the left
> pane, so you have to take the implementation out of the preference page and put
> it into your own dialog (similar to how the current dialog does it).

Extracted the code into a separate dialog. 
> 
> 
> > Markus, presently we only check for the 'simple name' of a method using the
> > method
> > 
> > boolean typeNameMatches(String nameA, String nameB) . Is it okay to do this? or
> > do we need to match the 'qualified name' of a method too? Because I found a
> > scenario where even if the qualified name of the method is entered wrong , the
> > logic considers the simple name and expands it by default .
> 
> It wouldn't make sense to treat methods with different qualifiers as the same
> (likewise with types). If you found a scenario that fails, you should fix the
> code. Did it already fail before with qualified types? Please post a concrete
> example so we're sure we talk about the same thing.

Here's an example : 
package p;
public class A {
	
	public void method(){
		
	}
}

class B extends A {
	
	 @Override
		public void method() {
		 
	 }
}
I was talking about the scenario where:  the user opens the EWC Dialog and enters the fully qualified name for a method as p.A.method. Then when he invokes call hierarchy on p.B.method, that method is also expanded with constructors since the last name matches. (The list in the dialog shows p.A.method though..) Wouldn't this be wrong behaviour? 

> 
> 
> Your patch contains some unnecessary changes that you should revert:
> 
> * CallHierarchyContentProvider:
> - Why did you rename the first field to CALL_HIERARCHY_PREFERENCES? It's still
> just one preference, and the name should tell what the preference stands for.
> What's not OK with the original name PREF_DEFAULT_EXPAND_WITH_CONSTRUCTORS?
> - PREF_ANONYMOUS_EXPAND_WITH_CONSTRUCTORS is still @since 3.5

Since the idea was to create a preference page initially, i renamed to CALL_HIERARCHY_PREFERENCES. I have now changed it back to PREF_DEFAULT_EXPAND_WITH_CONSTRUCTORS since the action is now renamed. 

> Other problems I found:
> 
Fixed the remaining issues.

Comment 5 Markus Keller 2009-09-07 13:34:55 EDT

(In reply to comment #4)
> Here's an example : [..]

That example is quite involved, since it mixes 2 scenarios:
- methods with equal names and different qualifiers
- methods in sub- and super classes

Let's have a look at an example with these scenarios separated:

package p;
public class T {
	public static void target() {}
}

package p;
public class A {
	public void method() { T.target(); }
}
class B extends A {
	public void method() { T.target(); }
}
class Independent {
	public void method() { T.target(); }
}
class C extends A {
	public void methodC() { T.target(); }
}

package x;
import p.T;
public class A {
	public void method() { T.target(); }
}
class X extends A {
	public void method() { T.target(); }
}

In the existing code with "p.A" in the list, open the caller hierarchy on T.target():
- p.A#method() was expanded with constructors (EWC) because declaring type was in the list
- p.B#method(), p.C#method() and x.X#method() were all EWC, because the supertype of the declaring types matched p.A (we matched with the unresolved, unqualified name of the supertype because we tried to get away without actually resolving the supertype)
- p.Independent#method() was not EWC (non-matching declaring types)
- x.A#method() was not EWC (non-matching declaring types)

In summary: Wherever it was not expensive to be correct we were of course fully correct and only matched types if their fully qualified names matched. With method names, that should be no different, i.e. with p.A.method in the list:
- p.A#method() should be EWC
- p.B#method(), p.C#method() and x.X#method() should all be EWC, because the supertype of the declaring types matches p.A (we currently keep matching with the unresolved, unqualified name of the supertype, but we can discuss doing a full type resolution later, i.e. after this bug has been closed)
- p.Independent#method() should not be EWC (non-matching declaring types)
- x.A#method() should not be EWC (non-matching declaring types)


Comments to the UI:
- The dialog must properly resize (list should grab excessive space)
- The border to the left of the list and to teh right of the buttons is too big (should be aligned with the checkbox and the OK / Cancel buttons).
- There's too much space on top and bottom of the checkbox.
- The labels in the 'New *...' dialogs don't make much sense for the Call Hierarchy settings. I would use use java.lang.Runnable and java.lang.Runnable.run as examples.
- 'Edit' button should be called 'Edit...' (actions that open a cancelable dialog should end with ellipsis).
- 'Edit' and 'Remove' buttons should also be enabled for the default entries (you could add a 'Restore Defaults' button if you want).

Comments to the implementation:
- It looks like users who configured their types in a 3.5 workspace lose those settings when they start with your patch. We should not lose the user's perferences.
- You always need to update Javadoc and identifiers (local variables, methods, ...) when you change code anywhere. E.g. the Javadoc for the preference key constants is outdated. You also call e.g. 'typeNameMatches(member, defaultType)'. It can't be right to compare a method name with a type name, right? Also, variables like serializedTypes and defaultTypes don't contain types any more, so their names should be corrected (use e.g. 'entry' if you don't find a better name for something that can be a type+".*" or a member name).

Comment 6 Raksha Vasisht 2009-09-10 07:57:35 EDT

(In reply to comment #5)

> In summary: Wherever it was not expensive to be correct we were of course fully
> correct and only matched types if their fully qualified names matched. With
> method names, that should be no different, i.e. with p.A.method in the list:
> - p.A#method() should be EWC
> - p.B#method(), p.C#method() and x.X#method() should all be EWC, because the
> supertype of the declaring types matches p.A (we currently keep matching with
> the unresolved, unqualified name of the supertype, but we can discuss doing a
> full type resolution later, i.e. after this bug has been closed)
> - p.Independent#method() should not be EWC (non-matching declaring types)
> - x.A#method() should not be EWC (non-matching declaring types)
> 

Here when you say 'supertype of the declaring types matches p.A ' , you also have p.A in the list right? Otherwise the current behaviour is : 
 with only p.A.method in the list :

- p.A#method() should be EWC
- p.B#method(), p.C#method() and x.X#method() should NOT be EWC
- p.Independent#method() should not be EWC 
- x.A#method() should not be EWC

Since we match the exact method name(appending the method name to the qualified name of the type) to the method in the list (p.A.method). So if p.A is not in the list, only p.A.method is EWC , the rest are all not EWC.

> 
> Comments to the UI:
 working on the changes... will attach a patch soon.

Comment 7 Markus Keller 2009-09-10 09:59:38 EDT

(In reply to comment #6)
> > - p.B#method(), p.C#method() and x.X#method() should all be EWC, because the
> > supertype of the declaring types matches p.A (we currently keep matching with
> > the unresolved, unqualified name of the supertype, but we can discuss doing a
> > full type resolution later, i.e. after this bug has been closed)
[..]
> Here when you say 'supertype of the declaring types matches p.A ' , you also
> have p.A in the list right? Otherwise the current behaviour is : 
>  with only p.A.method in the list :
> 
[..]
> - p.B#method(), p.C#method() and x.X#method() should NOT be EWC
> 
> Since we match the exact method name(appending the method name to the qualified
> name of the type) to the method in the list (p.A.method). So if p.A is not in
> the list, only p.A.method is EWC , the rest are all not EWC.

No, I really meant it the way I wrote even with only p.A.method in the list. The crux of the matter is that all 3 methods in question *override* p.A#method() (we don't consider the concrete overriding rules, since we also don't store the parameter types, so we assume all methods with the same name are overriding).

A real-life example of why I think that's the right way is:
- "java.lang.Runnable.run" is in the list
- you have a run() method from an anonymous new Runnable(){public void run(){}} in the call hierarchy
=> we should:
1. test whether the method name matches the list entry's method name
2. test whether the type name matches the list entry's type name (fully qualified), OR whether the supertype name matches the list entry's type name (fully qualified, with a fallback to simple names if the supertype's qualifier is not available).

Comment 8 Raksha Vasisht 2009-09-14 04:58:29 EDT

Created attachment 147082 [details]
Updated Patch 

(In reply to comment #7)
> 
> No, I really meant it the way I wrote even with only p.A.method in the list.
> The crux of the matter is that all 3 methods in question *override*
> p.A#method() (we don't consider the concrete overriding rules, since we also
> don't store the parameter types, so we assume all methods with the same name
> are overriding).
> 
> A real-life example of why I think that's the right way is:
> - "java.lang.Runnable.run" is in the list
> - you have a run() method from an anonymous new Runnable(){public void run(){}}
> in the call hierarchy
> => we should:
> 1. test whether the method name matches the list entry's method name
> 2. test whether the type name matches the list entry's type name (fully
> qualified), OR whether the supertype name matches the list entry's type name
> (fully qualified, with a fallback to simple names if the supertype's qualifier
> is not available).

Right, it makes sense to keep the behaviour same between the types and methods, since we cant follow the concrete overriding rules without having the fully qualified names for the methods and the super types. 

Attaching the updated patch.

Comment 9 Markus Keller 2009-10-07 14:56:11 EDT

(In reply to comment #8)

- We don't need a separate action group and separate group in the menu for this action. Please remove the ExpandWithConstructorsActionGroup and leave the action in the CallHierarchyFiltersActionGroup. 

ExpandWithConstructorsConfigurationBlock:

> - The dialog must properly resize (list should grab excessive space)
Not fixed. Fix is to do this at the end of #createPreferenceList(Composite):
		gd.verticalAlignment= GridData.FILL;
		gd.grabExcessVerticalSpace= true;

> - There's too much space on top and bottom of the checkbox.
Still too much on the bottom. That's because ExpandWithConstructorsDialog is a StatusDialog. Since your implementation never sets a status, the gap is unnecessary. Please change to a TrayDialog.

- The 'Restore Defaults' button should restore all defaults (also the 'All in anonymous' checkbox).

- We still have problems with default and old preferences:
  - The defaults should be set in org.eclipse.jdt.ui.PreferenceConstants#initializeDefaultValues(IPreferenceStore). That's the only place where the defaults should appear, i.e. remove #defaultTypes and use PreferenceConstants.getPreferenceStore().setToDefault(..) like we do it for other preferences.
  - DEFAULT_EXPAND_WITH_CONSTRUCTORS is used as a preference *key*, but in the initalizer, you actually store the *value* of the old preference settings into the constant!
  - #getExpandWithConstructorsKey() is of course a wrong name for a method that returns an array of entries

  - When starting with a new workspace, the defaults in the list are rendered as members, but they do behave as if they were types.
  - When starting with a 3.5 workspace, the entries in the list are members although they should be types (since 3.5 only supported types).
  => You have come up with a strategy to *convert* 3.5-style settings to the new format. I guess the easiest way is to use a new preference key for the new format, and convert old settings when you read them in:
    - in PreferenceConstants, define a new preference key for the new format and make sure it is properly documented (new API; we couldn't add API in 3.5M7 when we added the feature)
    - set the defaults for the new preference in PreferenceConstants
    - while you're at it, also make PREF_ANONYMOUS_EXPAND_WITH_CONSTRUCTORS API in PreferenceConstants (keep the key unchanged such that stored preferences continue to work)
    - when you need to read the default EWC list, first try to read the old settings from CallHierarchyContentProvider.PREF_DEFAULT_EXPAND_WITH_CONSTRUCTORS.
    - if there are no old settings, just use the new ones and everything's fine
    - if there are old settings, convert them to the new format, store them under the new key, and then delete the old settings (such that you won't convert them again the next time).

Comment 10 Raksha Vasisht 2009-10-13 01:54:58 EDT

Created attachment 149404 [details]
Patch with review changes

Incorporated the mentioned changes.

Added code to read from the old preference key to be compatible with 3.5 settings.
Added two new keys as API.
Added default settings for the checkbox.

Comment 11 Markus Keller 2009-10-14 08:15:39 EDT

- PreferenceConstants:
  - remove the comments about "New API: ...". We don't justify APIs and the information does not really help clients (and it's not really correct, since we knew this was not API-quality yet, and we indeed had to adjust it a bit).
  - the value description for PREF_DEFAULT_EXPAND_WITH_CONSTRUCTORS_MEMBERS is not correct

- CallHierarchyContentProvider:
  - rename PREF_DEFAULT_EXPAND_WITH_CONSTRUCTORS to OLD_... and add a link to the replacing preference

- ExpandWithConstructorsConfigurationBlock:
  - remove @author
  - fix @param of serializeMembers(List)
  - fix formatting of getDefaultExpandWithConstructorsMembers()
  - the first time I tried to open the dialog, I got:
java.lang.UnsupportedOperationException
	at java.util.AbstractList.add(AbstractList.java:131)
	at java.util.AbstractList.add(AbstractList.java:91)
	at org.eclipse.jdt.internal.ui.callhierarchy.ExpandWithConstructorsConfigurationBlock.getDefaultExpandWithConstructorsMembers(ExpandWithConstructorsConfigurationBlock.java:611)
	at org.eclipse.jdt.internal.ui.callhierarchy.ExpandWithConstructorsConfigurationBlock.initializeFields(ExpandWithConstructorsConfigurationBlock.java:515)
	at org.eclipse.jdt.internal.ui.callhierarchy.ExpandWithConstructorsConfigurationBlock.initialize(ExpandWithConstructorsConfigurationBlock.java:507)
	at org.eclipse.jdt.internal.ui.callhierarchy.ExpandWithConstructorsConfigurationBlock.createContents(ExpandWithConstructorsConfigurationBlock.java:459)
	at org.eclipse.jdt.internal.ui.callhierarchy.ExpandWithConstructorsDialog.createDialogArea(ExpandWithConstructorsDialog.java:69)
...
  - to avoid confusion, also append "_MEMBERS" to the name of Key DEFAULT_EXPAND_WITH_CONSTRUCTORS


- Conversion of old preferences:
  - we should not initialize the old preference with a value we never use. In PreferenceConstants, change 'store.setDefault(CallHierarchyContentProvider.PREF_DEFAULT_EXPAND_WITH_CONSTRUCTORS, ...' to set the default for the old preference to "".
  - after converting the old value, you can then restore the default via setToDefault(...), rather than using putValue(..., "")
  - the code that converts old to new preferences is currently in ExpandWithConstructorsConfigurationBlock#getDefaultExpandWithConstructorsMembers(), so it is never used unless the user opens the preference dialog. Please move this into PreferenceConstants#initializeDefaultValues(..).

Comment 12 Raksha Vasisht 2009-11-03 01:14:35 EST

Created attachment 151152 [details]
Patch with review changes

Have done the renaming of preference constants, 
Fixed formatting, 
Made changes to javadoc and 
Moved the compatiblity code up to PreferenceConstants.java

Comment 13 Markus Keller 2009-11-04 12:44:38 EST

* PreferenceConstants:
- PREF_DEFAULT_EXPAND_WITH_CONSTRUCTORS_MEMBERS: Description should end with: ' or "." + method name.'
- Javadocs: A single empty line before '@since 3.6' is enough
- Would you agree with changing the default list to "java.lang.Runnable.run;java.util.concurrent.Callable.call;org.eclipse.swt.widgets.Listener.handleEvent"?

* CallHierarchyContentProvider:
- isInTheDefaultExpandWithConstructorList(CallerMethodWrapper) should take the wrapped member (the wrapper is not necessary otherwise). Since the previous call to canExpandWithConstructors(..) guarantees that it's an IMethod, you should just cast it in ensureDefaultExpandWithConstructors and then pass it as an IMethod to isInTheDefaultExpandWithConstructorList.
Please make a more careful pass over the implementation to make sure it really works as it should and not only by chance. This code is not trivial, so it's even more important that its correctness can be verified by reading the code (since we will not be able to test all possibles cases). Things that jumped to my eyes when I read the code:
  - 'String member= wrapper.getMember().getElementName();' => this is a member name, not a member. After the fix I outlined above, you even know it's a methodName.
  - You call 'typeNameMatches(member, defaultTypeOrMember)' where 'member' is a method name.
  - In the for loops, you have calls like 'superClass.concat(".").concat(member)'. These should be extracted so that they are executed only once. And please use + for string concatenations, rather than concat(..). That's the style we use everywhere else, and that's easier to read. When you only need to add a single character, use '.' rather than ".".
  - declaringType.getSuperclassName() contains type arguments for parameterized types, so you have to strip these away (e.g. for Callable). That's already wrong in HEAD. Same problem with getSuperInterfaceNames().
  - 'defaultTypeOrMember.endsWith(superClass.concat(".").concat(member))' looks wrong. I think you can construct cases where the expression is true but the suffix does not completely match the last identifier of the receiver (e.g. "org.MyRunnable.run".endsWith("Runnable.run")). And why is this check with the member name only done with the superclass but not with the superinterfaces?

* ExpandWithConstructorsConfigurationBlock:
- initializeFields():
  - the additional ArrayList is unnecessary (and even otherwise, it would have been better to just pass the original collection in the constructor)
  - the method should be inlined into initialize()

Comment 14 Raksha Vasisht 2009-11-11 06:07:01 EST

Created attachment 151933 [details]
Updated Patch

(In reply to comment #13)
> * PreferenceConstants:
> - Would you agree with changing the default list to
>"java.lang.Runnable.run;java.util.concurrent.Callable.call;org.eclipse.swt.widgets.Listener.handleEvent"?

Yes, we can show methods now.

>   - 'defaultTypeOrMember.endsWith(superClass.concat(".").concat(member))' looks
> wrong. I think you can construct cases where the expression is true but the
> suffix does not completely match the last identifier of the receiver (e.g.
> "org.MyRunnable.run".endsWith("Runnable.run")). And why is this check with the
> member name only done with the superclass but not with the superinterfaces?
> 

Oops! looks like I missed the case where the superinterface method is listed as default EWC. Thanks for pointing it out, I have made the rest of the changes as well in the patch.

Comment 15 Markus Keller 2009-12-03 13:34:48 EST

Released the last patch to HEAD with a few fixes in CallHierarchyContentProvider:
- Compile error: String#contains(..) is 1.5. Furthermore, the test should not be done twice. Moved into stripTypeArguments(..).
- Some more code cleanup and rewritten isInTheDefaultExpandWithConstructorList(..). Your patch still had expressions like "typeNameMatches(methodName, defaultMemberName)" which are just wrong (a method name is not a type name).

Raksha, please take a look at the new CallHierarchyContentProvider and compare it with your version (e.g. via local history).

Comment 16 Deepak Azad 2009-12-08 14:03:31 EST

Verified for 3.6 M4 with I20091207-1800.
Used M20091202-0800 (3.5.2) for verifying that the preferences that have been stored in a 3.5 workspace are properly converted when the 3.5 workspace is started with the Eclipse 3.6.