>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
