jonathanlocke commented on PR #1605: URL: https://github.com/apache/wicket/pull/1605#issuecomment-5733578715
It's been a long time since I looked at a PR for Wicket, but I've been getting back into things over the past couple years. I feel a bit "on the fence" on this PR. I can see it both ways. Arguably, you could say that a component should have one IModel and that cases where you have multiple models are usually a case where things could be decomposed better. For example, you could have a composite IModel that detaches its sub-models or in some cases you could simply break up the component into sub-panels. But there are cases where this PR addresses a real flaw, including one that's already in Wicket. For example, the multi-select list choice component quite legitimately has two unrelated models (the options model and the selection model). What we wound up doing 15-20 years ago with this is to have a component have one primary or "default" model (that's why it's getDefaultModelObject() instead of getModelObject() if I remember correctly) and then your component can have more models th at you manage yourself. If this PR gets approved, it seems like the multi-select choice component is a poster child for the issue this PR addresses and that it would benefit by adopting this API. My first worry when I saw the PR title was negative because I didn't want the IModel field on Component to become a list, but I think this PR recognizes that this is an unusual case and using metadata seems one good solution to keep components slim while implementing support for detaching multiple models. Another one would be be creating a composite model that absorbs and manages the primary and additional models. That might be worth prototyping, actually to see if it is cleaner/simpler. I can't yet see a reason to vote "no" on this, but it would be good to do some thinking about what this does to Wicket overall in terms of how people code in it. It's a pretty deep change, so it would be good to get a lot of eyes on this, but I can't see a good reason to say "no" at first glance. If approve d, I'd like to see any core components in wicket that already have multiple models adopt the API in a follow-on PR. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
