[MGNLSTK-688] Minor generics improvement Created: 14/Oct/10  Updated: 19/Jan/11  Resolved: 19/Jan/11

Status: Closed
Project: Magnolia Standard Templating Kit (closed)
Component/s: base system
Affects Version/s: 1.3, 1.3.1, 1.3.2, 1.3.3, 1.3.4, 1.3.5
Fix Version/s: 1.4.2

Type: Improvement Priority: Neutral
Reporter: Roelof Jan Assignee: Federico Grilli
Resolution: Fixed Votes: 0
Labels: generics
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Attachments: Text File MGNLSTK-688-1.patch     Text File MGNLSTK-688-2.patch     Text File MGNLSTK-688-STK.patch     Text File MGNLSTK-688-Templating.patch     Text File RenderingModelImpl.java     Text File STKTemplateModel.java    
Issue Links:
dependency
depends upon MAGNOLIA-3497 Use generics in RenderingModelImpl Closed
Template:
Patch included:
Yes
Acceptance criteria:
Empty
Date of First Response:

 Description   

I ran into some issues concerning the use of generics:

public class RenderingModelImpl<RD extends RenderableDefinition> implements RenderingModel

shouldn't this be

public class RenderingModelImpl<RD extends RenderableDefinition> implements RenderingModel<RD>

When this is fixed it would be nice to be able to subclass both STKTemplateModel and STKTemplate while being able to access operations on the STKTemplate subclass by getDefinition() on the model class. After the previous fix this would possible without braking anything.

Change:

public class STKTemplateModel extends RenderingModelImpl<STKTemplate>

to

public class STKTemplateModel<RD extends STKTemplate> extends RenderingModelImpl<RD>

using this constructor:

public STKTemplateModel(Content content, RD definition, RenderingModel parent)

{ super(content, definition, parent); }

 Comments   
Comment by Philipp Bärfuss [ 20/Oct/10 ]

Seams to make much sense to me.

Comment by Magnolia International [ 17/Dec/10 ]

Patches for the above. Makes sense to me but needs an issue in MAGNOLIA. What are the consequences for existing classes, is this breaking binary compatbility ?

Comment by Roelof Jan [ 20/Dec/10 ]

The compiler uses "type erasure" to maintain binary compatibility. The actual type arguments are not available at runtime. As for the first issue, you might consider it a bug, since a generic interface is implemented with a raw type. The last fix is mainly a compile time issue as well. It could at best make some casts obsolete when subclassing STKTemplate.

Comment by Federico Grilli [ 13/Jan/11 ]

Hello Roelof,
I tried to apply the changes you suggested but had weird problems for instance in info.magnolia.module.templatingkit.style.BodyClassResolver.resolveBodyClass(STKTemplateModel) where at line 93

final STKTemplate def = model.getDefinition();

I needed to cast the result of getDefintion() to STKTemplate in order to compile the class. Same problem with info.magnolia.module.templatingkit.style.CssSelectorBuilder.createCssSelector(RenderingModelImpl) and info.magnolia.module.templatingkit.templates.SingletonParagraphTemplateModel.execute(). Did you, by chance, encounter the same problems? If so, how did you solve them?

Comment by Roelof Jan [ 17/Jan/11 ]

In the Hudson change log I only see a patch for STKTemplate. Did you fix RenderingModelImpl as well by appending <RD>:

public class RenderingModelImpl<RD extends RenderableDefinition> implements RenderingModel<RD> <-- !!!

In the old version subclasses of RenderingModelImpl return the raw type (RenderableDefinition) and not the declared generic subtype, in your case STKTemplate.

Comment by Federico Grilli [ 17/Jan/11 ]

Yes, we applied the patch for RenderingModelImpl too (rev. 41257) but for some strange reason hudson change log does not show it.

Comment by Roelof Jan [ 17/Jan/11 ]

I have an incomplete checkout, which projects (svn paths) should I checkout to reproduce this?

Comment by Federico Grilli [ 17/Jan/11 ]

OK, the reason why RenderingModelImpl changes were not shown here is not so strange as that class belongs to the magnolia-module-templating in the main project. So to reproduce the issue you need magnolia 4.4 branch and stk trunk. The patches were reverted, so you need to apply them again. Thanks.

Comment by Roelof Jan [ 17/Jan/11 ]

This applies to all STKTemplateModel subclasses which now can provide their own renderable definition.

Comment by Federico Grilli [ 17/Jan/11 ]

Thanks.

Comment by Federico Grilli [ 17/Jan/11 ]

Applied patch. Java compiler type erasure will keep binary compatibility.

Comment by Federico Grilli [ 17/Jan/11 ]

Maybe I was too rushed into resolving this. As far as I can understand, binary compatibility will not be an issue but backward compatibility will. If fact, if someone has extended STKTemplateModel like this

class Foo extends STKTemplateModel{

	public Foo(Content content, STKTemplate definition, RenderingModel parent) {
		super(content, definition, parent);
	}

	@Override
	public String execute() {
		STKTemplate template = getDefinition();
                //do something here
		return super.execute();
	}

}

when they will upgrade to the latest stk they will incur in compilation issues as getDefinition() will either need a cast or their custom template model will need to specify the type like this class Foo extends STKTemplateModel<STKTemplate> as we did in info.magnolia.module.templatingkit.templates.SingletonParagraphTemplateModel

Comment by Federico Grilli [ 17/Jan/11 ]

I also noticed that, strangely, overriding getDefinition() in info.magnolia.module.templatingkit.templates.STKTemplateModel<RD> like this

@Override
    public RD getDefinition() {
    	return super.getDefinition();
    }

will make the need for casting or type specification useless.

Comment by Magnolia International [ 17/Jan/11 ]

Roelof Jan, thanks for the new patches. I think this is +/- what we originally committed.. and reverted. We felt that introducing the need for casting somehow reduced the benefits of generics here. But I'm assuming you had other benefits elsewhere in your own modules gained with this. Would you be so kind so as to explain a little ? So we can make a sound decision

Comment by Roelof Jan [ 17/Jan/11 ]

Interesting. This was what I had expected as a default although this behavior is somewhat obvious when looking closely at it. This might facilitate backward compatibility when compiling against newer versions of your codebase, which certainly is an issue in the applied patches.

Comment by Roelof Jan [ 17/Jan/11 ]

@Gregory I Just missed your post. We often subclass STKTemplate with a subclass definition. Now with getDefinition we must check and cast to our subclass definition. Generics can solve this easily, I thought. Overriding getDefinition in STKTemplate is apparently necessary to keep the returns type in sync with the old statically defined return type.

This still is a minor issue. The RenderingDefinitionImpl declaration looks like a bug to me. STKTemplateModel improvements should not brake any code. Besides there is still a lot work to do in these parts, this is just a single issue.

Comment by Federico Grilli [ 18/Jan/11 ]

And so we went for applying the patch along with the overriding of getDefinition() so not have issues when compiling existing subclasses of STKTemplateModel against the new codebase.

Comment by Philipp Bärfuss [ 19/Jan/11 ]

Changes in BodyClassResolver and CSSSelectorBuilder were not undone.

Generated at Mon Feb 12 07:29:27 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.