Bug 363765

Comment 1 Greg Amerson 2011-11-15 05:28:29 EST

Created attachment 207016 [details]
Patch v1

First attempt at patch.  Works for List property, don't yet know about Enum controlled pagebook version, haven't tested it.

Comment 2 Konstantin Komissarchik 2011-11-15 11:42:22 EST

Review comments on Patch v1:

1. The resolve() function that this logic replaced performed parameter substitution. The new logic must perform parameter substitution as well.

2. Probably should move this block to init() function in SapphireListControlledPageBook just like it is in SapphireEnumControlledPageBook.

3. The invalid path message text is incorrect. It makes a reference to a property editor, but a page book is not a property editor.

4. Contribution headers were not updated.

Comment 3 Greg Amerson 2011-11-15 20:42:10 EST

Created attachment 207075 [details]
Patch v2

Comment 4 Greg Amerson 2011-11-15 20:42:25 EST

(In reply to comment #2)
> Review comments on Patch v1:
> 
> 1. The resolve() function that this logic replaced performed parameter
> substitution. The new logic must perform parameter substitution as well.

In this new logic, it eventually calls the super.resolve(...) which looks like it does param substitution.  

> 
> 2. Probably should move this block to init() function in
> SapphireListControlledPageBook just like it is in
> SapphireEnumControlledPageBook.

Sure thing, its now moved to init().

> 
> 3. The invalid path message text is incorrect. It makes a reference to a
> property editor, but a page book is not a property editor.

Sorry I just blindly copied it, its adjusted now.

> 
> 4. Contribution headers were not updated.

Fixed.

Comment 5 Konstantin Komissarchik 2011-11-16 12:25:28 EST

Patch accepted with following changes:

1. Fixed logic error in parent nav for more than one parent nav in path.

2. Fixed some instances of incorrect element being used. Probably the most significant were the changes to listening logic necessary in enum controlled page book.

Please verify this feature in both page books.

Comment 6 Konstantin Komissarchik 2011-11-16 12:27:13 EST

Forgot one more item that I had to change...

> In this new logic, it eventually calls the super.resolve(...) which looks like
> it does param substitution.

This would preclude paths from being provided for substitution. You could only substitute a property name. I fixed the logic to do param substitution before parsing the path.

Comment 7 Greg Amerson 2011-11-17 00:20:42 EST

For the ListControlled pagebook its work correctly.  For enum controlled, I'm getting an exception:

java.lang.NullPointerException
	at org.eclipse.sapphire.modeling.ModelElement.read(ModelElement.java:220)
	at com.liferay.ide.eclipse.hook.core.model.internal.CustomJspDir.read(CustomJspDir.java:120)
	at org.eclipse.sapphire.modeling.ModelElement.read(ModelElement.java:228)
	at org.eclipse.sapphire.ui.SapphireEnumControlledPageBook.updateCurrentPage(SapphireEnumControlledPageBook.java:135)
	at org.eclipse.sapphire.ui.SapphireEnumControlledPageBook.init(SapphireEnumControlledPageBook.java:99)
	
	
Looking at the code, in line 134, its pulling the local model element once again to find the property instead of using the element that was found when it resolved the model path during init.  

So if I change line 134 of SapphireEnumControlledPageBook.java to

final IModelElement modelElement = this.element; 

then it seems to work ok.

Comment 8 Konstantin Komissarchik 2011-11-17 04:22:15 EST

Try it now.

Comment 9 Greg Amerson 2011-11-17 04:51:55 EST

Looks good.

Comment 10 Konstantin Komissarchik 2011-11-17 04:53:25 EST

Thanks. Closing.