[jira] Updated: (WICKET-3218) Component#onInitialize is broken for Pages
[ https://issues.apache.org/jira/browse/WICKET-3218?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Carl-Eric Menzel updated WICKET-3218: - Attachment: 0001-delay-oninitialize-until-just-before-onconfigure.patch A new attempt at resolving this. I moved the call to initialize() into internalBeforeRender(), since unlike configure() this can't be triggered by framework users. Components that are added before the Page itself has been initialized now are now simply initialized with the cascading fireInitialize() when the Page gets its initialize call. Components that are added after that will be initialized by Page#componentAdded. This way the initialization order is guaranteed to follow the component hierarchy. IMO, this restores a consistent and intuitive behavior for onInitialize for *all* components including Pages, without breaking any existing code. I adjusted ComponentInitializationTest to always use tester.startPage, since initialization is no longer triggered by add(). The tests that still made sense in this light still work. testPropagation was removed, since there is no immediate propagation anymore, and eventual propagation down the component tree is included in testInitializationOrder. One thing I'm not so happy about: I needed to distinguish between already initialized and currently initializing to delay initialization for components that are added in an onInitialize method. To do that I needed a new flag FLAG_INITIALIZING. Unfortunately the int space for flags was exhausted, so I had to turn Component#flags into a long. The cost of this is 4 extra bytes. I'm not sure whether that is acceptable. If not, it could probably be replaced with a single extra boolean in Component, which would reduce the cost but not eliminate it. Component#onInitialize is broken for Pages -- Key: WICKET-3218 URL: https://issues.apache.org/jira/browse/WICKET-3218 Project: Wicket Issue Type: Bug Components: wicket Affects Versions: 1.4.14 Reporter: Carl-Eric Menzel Attachments: 0001-delay-oninitialize-until-just-before-onconfigure.patch As first mentioned at http://mail-archives.apache.org/mod_mbox/wicket-dev/201012.mbox/%3c1291238699.1749.1408166...@webmail.messagingengine.com%3e , I think the current (Wicket 1.4.14) implementation of Component#onInitialize is broken for Pages. Pages get initialized as soon as the first component is added, which is correct. But this usually happens within the constructor of the page, which means that the page object isn't fully initialized yet. The entire point of having onInitialize, however, is to be able to do further work once all constructors have run. See https://github.com/duesenklipper/wicket-oninitialize for a quickstart that demonstrates the problem. Pedro Santos suggested in the above thread to just switch the entire object construction to onInitialize. I don't think this is a good idea, because 1) it is completely counter-intuitive 2) it is not always realistic to have an entire class hierarchy not using the constructor just because a subclass somewhere might want to use onInitialize 3) it is inconsistent with onInitialize behavior for all other (non-Page) components. Here I can easily mix work in the constructor with onInitialize. I propose the following patch: - override onInitialize in Page and make it final, so Pages can't use this any more. This should not cause any unnecessary breaking, since currently it's not working for pages anyway. - introduce Page#onPageInitialize to provide a safe alternative to onInitialize - make a special case for Page in Component's beforeRender to fire Page#onPageInitialize if necessary Yes, this is a bit of special casing for Page, but there's quite a lot of that needed for Page anyway. I think the impact of this should be minimal. My page includes documentation and a new testcase that verifies the new behavior. I modified the old ComponentInitializationTest to reflect the fact that Page doesn't get onInitialize any more. -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
[jira] Updated: (WICKET-3218) Component#onInitialize is broken for Pages
[ https://issues.apache.org/jira/browse/WICKET-3218?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Carl-Eric Menzel updated WICKET-3218: - Attachment: 0001-added-page-onpageinitialize.patch A patch against Wicket 1.4.14 that does what I outlined in the bug description. Includes appropriate test cases. Component#onInitialize is broken for Pages -- Key: WICKET-3218 URL: https://issues.apache.org/jira/browse/WICKET-3218 Project: Wicket Issue Type: Bug Components: wicket Affects Versions: 1.4.14 Reporter: Carl-Eric Menzel Attachments: 0001-added-page-onpageinitialize.patch As first mentioned at http://mail-archives.apache.org/mod_mbox/wicket-dev/201012.mbox/%3c1291238699.1749.1408166...@webmail.messagingengine.com%3e , I think the current (Wicket 1.4.14) implementation of Component#onInitialize is broken for Pages. Pages get initialized as soon as the first component is added, which is correct. But this usually happens within the constructor of the page, which means that the page object isn't fully initialized yet. The entire point of having onInitialize, however, is to be able to do further work once all constructors have run. See https://github.com/duesenklipper/wicket-oninitialize for a quickstart that demonstrates the problem. Pedro Santos suggested in the above thread to just switch the entire object construction to onInitialize. I don't think this is a good idea, because 1) it is completely counter-intuitive 2) it is not always realistic to have an entire class hierarchy not using the constructor just because a subclass somewhere might want to use onInitialize 3) it is inconsistent with onInitialize behavior for all other (non-Page) components. Here I can easily mix work in the constructor with onInitialize. I propose the following patch: - override onInitialize in Page and make it final, so Pages can't use this any more. This should not cause any unnecessary breaking, since currently it's not working for pages anyway. - introduce Page#onPageInitialize to provide a safe alternative to onInitialize - make a special case for Page in Component's beforeRender to fire Page#onPageInitialize if necessary Yes, this is a bit of special casing for Page, but there's quite a lot of that needed for Page anyway. I think the impact of this should be minimal. My page includes documentation and a new testcase that verifies the new behavior. I modified the old ComponentInitializationTest to reflect the fact that Page doesn't get onInitialize any more. -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
[jira] Updated: (WICKET-3218) Component#onInitialize is broken for Pages
[ https://issues.apache.org/jira/browse/WICKET-3218?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Igor Vaynberg updated WICKET-3218: -- Attachment: (was: 0001-added-page-onpageinitialize.patch) Component#onInitialize is broken for Pages -- Key: WICKET-3218 URL: https://issues.apache.org/jira/browse/WICKET-3218 Project: Wicket Issue Type: Bug Components: wicket Affects Versions: 1.4.14 Reporter: Carl-Eric Menzel As first mentioned at http://mail-archives.apache.org/mod_mbox/wicket-dev/201012.mbox/%3c1291238699.1749.1408166...@webmail.messagingengine.com%3e , I think the current (Wicket 1.4.14) implementation of Component#onInitialize is broken for Pages. Pages get initialized as soon as the first component is added, which is correct. But this usually happens within the constructor of the page, which means that the page object isn't fully initialized yet. The entire point of having onInitialize, however, is to be able to do further work once all constructors have run. See https://github.com/duesenklipper/wicket-oninitialize for a quickstart that demonstrates the problem. Pedro Santos suggested in the above thread to just switch the entire object construction to onInitialize. I don't think this is a good idea, because 1) it is completely counter-intuitive 2) it is not always realistic to have an entire class hierarchy not using the constructor just because a subclass somewhere might want to use onInitialize 3) it is inconsistent with onInitialize behavior for all other (non-Page) components. Here I can easily mix work in the constructor with onInitialize. I propose the following patch: - override onInitialize in Page and make it final, so Pages can't use this any more. This should not cause any unnecessary breaking, since currently it's not working for pages anyway. - introduce Page#onPageInitialize to provide a safe alternative to onInitialize - make a special case for Page in Component's beforeRender to fire Page#onPageInitialize if necessary Yes, this is a bit of special casing for Page, but there's quite a lot of that needed for Page anyway. I think the impact of this should be minimal. My page includes documentation and a new testcase that verifies the new behavior. I modified the old ComponentInitializationTest to reflect the fact that Page doesn't get onInitialize any more. -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
