This patch https://gist.github.com/dashorst/eb84199f86e109728dce fixes the API.
All in all it took for our 1.2M lines of code project roughly 10 minutes to fix the breakages, improving the API considerably, discovering and fixing 1 wrongly typed listview and in total 16 compile errors. This is with about 550 ListView occurrences in total across the whole application. I'm +1 for this patch. Martijn On Mon, Jun 22, 2015 at 3:46 PM, Emond Papegaaij <[email protected]> wrote: > Hi, > > As Martijn said: the current form limits the user in a way that is not needed. > The returned List is not read-only, forcing users to keep a reference to the > IModel to be able to update the List. Also, it's inconsistent. populateItem > still uses ListItem<T>, not <? extends T>. Changing the constructors, setList, > getList and getModelObject to List<T> and IModel<? extends List<T>> seems a > minor API to me, making the API much more consistent and flexible. > > Best regards, > Emond > > On Monday 22 June 2015 15:36:25 Sven Meier wrote: >> Hi, >> >> this is the relevant discussion why I reverted the ListView constructor >> to that of Wicket 6: >> >> http://mail-archives.apache.org/mod_mbox/wicket-dev/201409.mbox/%3CCAJmbs8gD >> a5mJgwbkoOZS3oH5TYZZ-Ap3_SFDjBHs5SYpn4zTkg%40mail.gmail.com%3E >> >> Changing it would mean an API break for existing applications. I don't mind >> >> > ListView exposes a read-write view on the contents of the list, via >> > ListView.getModelObject(), but also ListItem.setModelObject >> >> I wanted ListItem to be read-only, but Martin an I agreed on keeping it >> writeable for backwards-compatibility. Is this really a problem we have to >> fix in our API? >> >> Regards >> Sven >> >> On 22.06.2015 14:31, Emond Papegaaij wrote: >> > Hi Sven, >> > >> > It's easy to change the testcase to: >> > class NumberListView<N extends Number> extends ListView<N> >> > >> > and >> > >> > new NumberListView<Integer>("integers", integers) >> > >> > I think the constructor in Wicket 6 is wrong. It should be IModel<? >> > extends >> > List<T>> model: we don't care what type the list is, but we do care about >> > the type of the contents of the list. ListView exposes a read-write view >> > on the contents of the list, via ListView.getModelObject(), but also >> > ListItem.setModelObject. IMHO it should either be read-only with ? extends >> > T or read-write with T, but mixed. The first is a major API break, the >> > second is not, so I prefer the second. >> > >> > Best regards, >> > Emond >> > >> > On Monday 22 June 2015 14:22:25 Sven Meier wrote: >> >> Hi Martijn, >> >> >> >> the ListView constructor is now as it has been in Wicket 6: >> >> public ListView(final String id, final IModel<? extends List<? >> >> >> >> extends T>> model) >> >> >> >> ListViewTest#generics() shows a valid use case, that is not possible >> >> otherwise. >> >> >> >> Regards >> >> Sven >> >> >> >> On 22.06.2015 13:42, Martijn Dashorst wrote: >> >>> I'm not sure I'm fan of this change. Using these wildcards breaks all >> >>> kinds of code. What is the benefit? >> >>> >> >>> The way it is implemented currently is also inconsistent: ListItem is >> >>> typed as ListItem<T> but it should be ListItem<? extends T>. This >> >>> gist: https://gist.github.com/dashorst/4ee7ab1696321f290a24 shows how >> >>> this should be implemented. >> >>> >> >>> HOWEVER: I don't actually propose such a change, but rather have >> >>> adcb7a632af8225e86e09e398b8fb5430b143b18 be reverted. The linked patch >> >>> will break the world and for little to no benefit. >> >>> adcb7a632af8225e86e09e398b8fb5430b143b18 breaks API in a couple of >> >>> places, but I don't see the benefit of those breaks as well. >> >>> >> >>> Martijn > -- Become a Wicket expert, learn from the best: http://wicketinaction.com
