It's there: https://issues.apache.org/jira/browse/WICKET-5931
I'm happy to resolve this. The code in our apps is much cleaner now without the wildcards. Martijn On Mon, Jun 22, 2015 at 9:50 PM, Sven Meier <[email protected]> wrote: >>Please create a new issue for this improvement. > > ... before applying your patch - IMHO WICKET-5350 is already confusing > enough with my changes ;) > > Thanks > Sven > > > > > On 22.06.2015 16:44, Sven Meier wrote: >> >> I don't mind the change. Please create a new issue for this improvement. >> >> Many thanks >> Sven >> >> >> On 22.06.2015 16:05, Martijn Dashorst wrote: >>> >>> 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
