com.codename1.ui.Component.preferredSizeImpl() is unsafe as it sets the preferredSize from calcPreferredSize() which may be overridden

Original issue 1364 created by codenameone on 2015-02-19T14:27:12.000Z:

com.codename1.ui.Component.preferredSizeImpl() is unsafe as it sets the preferredSize from calcPreferredSize() which may be overridden.

This can be troublesome because on the other side getPreferredSize() returns a reference to the private member preferredSize. If it springs to mind to override calcPreferredSize() of a component and to return the result of getPreferredSize of another Component one can get very funny results which could easily drive one crazy.

Hence I suggest that within com.codename1.ui.Component.preferredSizeImpl() the result of calcPreferredSize() should better be transferred into the preferredSize member by using its com.codename1.ui.geom.Dimension.setWidth(int) and com.codename1.ui.geom.Dimension.setHeight(int) methods.

Comment #1 originally posted by codenameone on 2015-02-19T14:41:20.000Z:

com.codename1.ui.list.ContainerList.Entry.calcPreferredSize() does this crazy thing which enforces all entries to the same size as the renderer. That surely is not intended.

Comment #2 originally posted by codenameone on 2015-02-21T09:50:03.000Z:

I don't understand this issue at all. calcPreferredSize is and should be overriden and that works fine.
I looked at ContainerList and have yet to spot the crazy thing that forces all entries to the same size.

Comment #3 originally posted by codenameone on 2015-02-21T09:56:50.000Z:

Well, what happens is that the mathod com.codename1.ui.list.ContainerList.Entry.calcPreferredSize() returns the preferredSize Dimension instance of the renderer which then gets injected into all the ContainerList entries because of the behavior of com.codename1.ui.Component.preferredSizeImpl().
All the preferredSize attributes of the ContainerList then share the same Dimension instance, as does the renderer component.
Was that clear enough?

Comment #4 originally posted by codenameone on 2015-02-21T10:08:52.000Z:

My suggested fix:

Comment #5 originally posted by codenameone on 2015-02-21T15:36:01.000Z:

I don't see the logic in this change. It doesn't solve the issue and FYI the suggested fix also causes a null pointer exception (which is easily fixed).
I did see there is an issue with ContainerList sizing incorrectly but I don't see why that is the reason for that.

Comment #6 originally posted by codenameone on 2015-02-21T16:05:32.000Z:

Yes. It certainly solves the issue.
Look at com.codename1.ui.Component.setPreferredSize(Dimension). It does the right thing by not just assigning the Dimension instance.

Sorry I missed the check for null in my code example. I did not test it. It was merely meant as a hint.

With Codename One the preferredSize is cached and only calculated when requested by the shouldCalcPreferredSize attribute. com.codename1.ui.Component.preferredSizeImpl() just assigns the Dimension instance which is returned by the calcPreferredSize() method. The ContainerList overrides calcPreferredSize() for its entries and returns the renderer conponents preferredSize. Therefore all the ContainerList entries end up with a reference to the single Dimension instance taken from the renderer components preferredSize attribute.
That causes all the ContainerList entries to have the same size as the entry for which the perferredSize has been calculated most recently. Because all entries share the same Dimension object as their preferredSize attribute.

On the other hand one might even question whether it is a good idea to return the reference to the private attribute preferredSize's Dimension instance in the com.codename1.ui.Component.getPreferredSize(). Would one really want to be able to alter width and height of the preferredSize without setting it through the public setter method?

Comment #7 originally posted by codenameone on 2015-02-22T07:52:14.000Z:

I tried this before answering and it didn't solve the issue (that's how I knew there was an exception).

Preferred size is invoked for every Entry instance which invokes a preferred size on a different renderer instance hence calling calcPreferredSize() for every entry.

Returning an internal instance is something we do since the library is statically linked so the security aspect of exposing internal state is irrelevant.

Comment #8 originally posted by codenameone on 2015-02-23T10:49:33.000Z:

The problem will only arise, of course, when the calcPreferredSize does not create a new Dimension instance for every invocation. Therefore to test it you must use a component derived from Component, but not a Label, for instance, which returns a new Dimension instance every time calcPreferredSize is called, since it overrides calcPreferredSize and returns getUIManager().getLookAndFeel().getLabelPreferredSize(this).

Comment #9 originally posted by codenameone on 2015-02-23T13:41:47.000Z:

I posted son code to demonstrate the issue in https://groups.google.com/forum/#!topic/codenameone-discussions/Px4DJWito_U