You are viewing a plain text version of this content. The canonical link for it is here.
Posted to users@wicket.apache.org by Tom Götz <to...@decoded.de> on 2015/04/29 14:01:10 UTC
Re: Could not clear select2Choice component model value.
Hi Maxim,
I do not understand the changes you introduced to AbstractSelect2Choice.java in https://github.com/wicketstuff/core/commit/92851a253253849582a117d66dee5fcdb15c7353:
1.
you added a "private List<T> choices“ property, what is the purpose of this? Isn’t the ChoiceProvider supposed to provide the list of choices?
2.
in lines 130 and 133 you throw an IllegalStateException if either choices or renderer is null. Besides the fact that I didn’t get the purpose of the „choices“ list yet: this breaks several of our implementations, as we have select2 implementations where it’s not possible to determine the ChoiceProvider upon construction, but e.g. in onInitialize (we have several abstract select2 choice classes, where only the concrete implementation can know which choiceprovider to use …)
3.
you can now call the constructor in line 74 (public AbstractSelect2Choice(String id, IModel<M> model)) which generates above mentioned IllegalStateException …
4.
methods added starting in line 351: these use the newly introduced List<T> choices, could you explain the idea behind that?
Thanks in advance!
-Tom
> On 30.11.2014, at 16:32, Maxim Solodovnik <so...@gmail.com> wrote:
>
> Merged!
>
> Seems to work as expected in our application
> Thanks for the fix Martin!
>
> On Wed, Nov 26, 2014 at 10:20 AM, Maxim Solodovnik <so...@gmail.com>
> wrote:
>
>> Thanks a lot!
>> I'll backport!
>>
>> On Wed, Nov 26, 2014 at 1:34 AM, Martin Grigorov <mg...@apache.org>
>> wrote:
>>
>>> Fixed with
>>>
>>> https://github.com/wicketstuff/core/commit/b7e5b68b858336d85958663204166bd0852b43dd
>>> Hopefully all other use cases are still covered.
>>>
>>> I just noticed that there is
>>>
>>> https://github.com/wicketstuff/core/tree/wicket-6.x/jdk-1.6-parent/select2-parent
>>> (i.e. Wicket 6.x version).
>>> It would be good if someone backports the improvements from master branch
>>> to wicket-6.x:
>>>
>>> https://github.com/wicketstuff/core/commits/master/jdk-1.7-parent/select2-parent
>>> It seems Igor has been active last month at
>>> https://github.com/ivaynberg/wicket-select2/ and merged few PRs. I've
>>> ported them to WicketStuff 7.x but not to 6.x.
>>>
>>> Martin Grigorov
>>> Wicket Training and Consulting
>>> https://twitter.com/mtgrigorov
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@wicket.apache.org
For additional commands, e-mail: users-help@wicket.apache.org
Re: Could not clear select2Choice component model value.
Posted by Tom Götz <to...@richmountain.de>.
Done: https://github.com/wicketstuff/core/pull/402
Cheers,
-Tom
> On 30.04.2015, at 09:31, Martin Grigorov <mg...@apache.org> wrote:
>
> Hi Tom,
>
> Please send a PR with your suggested changes/improvements and we will
> discuss them.
> Thanks!
>
> Martin Grigorov
> Wicket Training and Consulting
> https://twitter.com/mtgrigorov
>
> On Thu, Apr 30, 2015 at 10:27 AM, Tom Götz <to...@decoded.de> wrote:
>
>> I found the source of the mentioned changes:
>>
>> https://github.com/wicketstuff/core/commit/79781d83cf11ac63d2e661328fa7176b93184c64
>>
>> -Tom
>>
>>
>>> On 30.04.2015, at 08:58, Tom Götz <to...@decoded.de> wrote:
>>>
>>> See my inline comments
>>>
>>>> On 30.04.2015, at 04:29, Maxim Solodovnik <so...@gmail.com> wrote:
>>>>
>>>>
>>>> The changes I have made were (most probably) merged from original Igor's
>>>> code (https://github.com/ivaynberg/wicket-select2)
>>>
>>> I can’t find the "List<T> choices“ property of AbstractSelect2Choice
>> anywhere in Igor’s original code, so I don’t know where it should have been
>> merged from.
>>>
>>> What should be the purpose of having to provide a ChoiceProvider *and* a
>> list of choices?! Currently you have to provide both to avoid an exception
>> upon construction, which is … weird ;) and breaks existing implementations.
>>>
>>>
>>>> 1) I believe this is since AbstractSelect2Choice is parent class for for
>>>> Single and Multi choices.
>>>
>>> That is so, but what does this explain? ;)
>>>
>>>
>>>> 2) the code was refactored a bit, so line numbers were changed. This
>>>> constructor:
>>>>
>> https://github.com/wicketstuff/core/blob/wicket-6.x/jdk-1.6-parent/select2-parent/select2/src/main/java/org/wicketstuff/select2/AbstractSelect2Choice.java#L109
>>>> seems to not throwing any exceptions
>>>
>>> Yeah, but another does. This pattern does not make any sense to me:
>>>
>>> class Foo {
>>>
>>> public Foo(Object a) {
>>> this(a, null);
>>> }
>>>
>>> public Foo(Object a, Object b) {
>>> if (b == null)
>>> throw new IllegalArgumentException();
>>> this.a = a;
>>> this.b = b;
>>> }
>>> }
>>>
>>>
>>>> 3) you can call constructor mentioned in 2 to avoid exception.
>>>
>>> …
>>>
>>>
>>>> 4) I guess renderChoice method is designed to render single choice
>> object,
>>>> you can override it (for ex. to add your own properties to each choice)
>>>> other 2 methods are private helpers
>>>
>>> renderChoice is called from within renderChoices which is called from
>> within renderHead. But only if !isAjax(), and:
>>>
>>> public boolean isAjax()
>>> {
>>> return provider != null;
>>> }
>>>
>>> …?
>>>
>>> That means: if no ChoiceProvider is set, then use the static List<T>
>> choices, but if you don’t provide a ChoiceProvider in the constructor
>> you’ll get an IllegalArgumentException.
>>>
>>> This code is broken and doesn’t make any sense *to me*, or I didn’t get
>> it yet ;-)
>>>
>>> My proposal is:
>>>
>>> if nobody can tell which problem should be solved with the introduced
>> List<T> choices I would revert it to get a working implementation again, as
>> it is currently unusable.
>>>
>>> Personally, I need to upgrade the select.js library in our project,
>> therefor I was looking at wicketstuff-select2, as we still use the original
>> wicket-select2 implementation. The current state of it leaves mit three
>> options, in order of priority:
>>>
>>> a)
>>> convince the wicketstuff-select2 maintainers to revert the code to a
>> working state, which I am currently doing ;-)
>>>
>>> b)
>>> fork wicketstuff-select2 and provide own (working) implementation
>>>
>>> c)
>>> stick with Igor’s original code, which means: no upgrades to select2.js
>>>
>>>
>>> Maybe someone else can shed some light on this?
>>>
>>> Cheers,
>>> -Tom
>>>
>>>
>>>
>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: users-unsubscribe@wicket.apache.org
>>> For additional commands, e-mail: users-help@wicket.apache.org
>>>
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: users-unsubscribe@wicket.apache.org
>> For additional commands, e-mail: users-help@wicket.apache.org
>>
>>
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@wicket.apache.org
For additional commands, e-mail: users-help@wicket.apache.org
Re: Could not clear select2Choice component model value.
Posted by Martin Grigorov <mg...@apache.org>.
Hi Tom,
Please send a PR with your suggested changes/improvements and we will
discuss them.
Thanks!
Martin Grigorov
Wicket Training and Consulting
https://twitter.com/mtgrigorov
On Thu, Apr 30, 2015 at 10:27 AM, Tom Götz <to...@decoded.de> wrote:
> I found the source of the mentioned changes:
>
> https://github.com/wicketstuff/core/commit/79781d83cf11ac63d2e661328fa7176b93184c64
>
> -Tom
>
>
> > On 30.04.2015, at 08:58, Tom Götz <to...@decoded.de> wrote:
> >
> > See my inline comments
> >
> >> On 30.04.2015, at 04:29, Maxim Solodovnik <so...@gmail.com> wrote:
> >>
> >>
> >> The changes I have made were (most probably) merged from original Igor's
> >> code (https://github.com/ivaynberg/wicket-select2)
> >
> > I can’t find the "List<T> choices“ property of AbstractSelect2Choice
> anywhere in Igor’s original code, so I don’t know where it should have been
> merged from.
> >
> > What should be the purpose of having to provide a ChoiceProvider *and* a
> list of choices?! Currently you have to provide both to avoid an exception
> upon construction, which is … weird ;) and breaks existing implementations.
> >
> >
> >> 1) I believe this is since AbstractSelect2Choice is parent class for for
> >> Single and Multi choices.
> >
> > That is so, but what does this explain? ;)
> >
> >
> >> 2) the code was refactored a bit, so line numbers were changed. This
> >> constructor:
> >>
> https://github.com/wicketstuff/core/blob/wicket-6.x/jdk-1.6-parent/select2-parent/select2/src/main/java/org/wicketstuff/select2/AbstractSelect2Choice.java#L109
> >> seems to not throwing any exceptions
> >
> > Yeah, but another does. This pattern does not make any sense to me:
> >
> > class Foo {
> >
> > public Foo(Object a) {
> > this(a, null);
> > }
> >
> > public Foo(Object a, Object b) {
> > if (b == null)
> > throw new IllegalArgumentException();
> > this.a = a;
> > this.b = b;
> > }
> > }
> >
> >
> >> 3) you can call constructor mentioned in 2 to avoid exception.
> >
> > …
> >
> >
> >> 4) I guess renderChoice method is designed to render single choice
> object,
> >> you can override it (for ex. to add your own properties to each choice)
> >> other 2 methods are private helpers
> >
> > renderChoice is called from within renderChoices which is called from
> within renderHead. But only if !isAjax(), and:
> >
> > public boolean isAjax()
> > {
> > return provider != null;
> > }
> >
> > …?
> >
> > That means: if no ChoiceProvider is set, then use the static List<T>
> choices, but if you don’t provide a ChoiceProvider in the constructor
> you’ll get an IllegalArgumentException.
> >
> > This code is broken and doesn’t make any sense *to me*, or I didn’t get
> it yet ;-)
> >
> > My proposal is:
> >
> > if nobody can tell which problem should be solved with the introduced
> List<T> choices I would revert it to get a working implementation again, as
> it is currently unusable.
> >
> > Personally, I need to upgrade the select.js library in our project,
> therefor I was looking at wicketstuff-select2, as we still use the original
> wicket-select2 implementation. The current state of it leaves mit three
> options, in order of priority:
> >
> > a)
> > convince the wicketstuff-select2 maintainers to revert the code to a
> working state, which I am currently doing ;-)
> >
> > b)
> > fork wicketstuff-select2 and provide own (working) implementation
> >
> > c)
> > stick with Igor’s original code, which means: no upgrades to select2.js
> >
> >
> > Maybe someone else can shed some light on this?
> >
> > Cheers,
> > -Tom
> >
> >
> >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: users-unsubscribe@wicket.apache.org
> > For additional commands, e-mail: users-help@wicket.apache.org
> >
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: users-unsubscribe@wicket.apache.org
> For additional commands, e-mail: users-help@wicket.apache.org
>
>
Re: Could not clear select2Choice component model value.
Posted by Tom Götz <to...@decoded.de>.
I found the source of the mentioned changes:
https://github.com/wicketstuff/core/commit/79781d83cf11ac63d2e661328fa7176b93184c64
-Tom
> On 30.04.2015, at 08:58, Tom Götz <to...@decoded.de> wrote:
>
> See my inline comments
>
>> On 30.04.2015, at 04:29, Maxim Solodovnik <so...@gmail.com> wrote:
>>
>>
>> The changes I have made were (most probably) merged from original Igor's
>> code (https://github.com/ivaynberg/wicket-select2)
>
> I can’t find the "List<T> choices“ property of AbstractSelect2Choice anywhere in Igor’s original code, so I don’t know where it should have been merged from.
>
> What should be the purpose of having to provide a ChoiceProvider *and* a list of choices?! Currently you have to provide both to avoid an exception upon construction, which is … weird ;) and breaks existing implementations.
>
>
>> 1) I believe this is since AbstractSelect2Choice is parent class for for
>> Single and Multi choices.
>
> That is so, but what does this explain? ;)
>
>
>> 2) the code was refactored a bit, so line numbers were changed. This
>> constructor:
>> https://github.com/wicketstuff/core/blob/wicket-6.x/jdk-1.6-parent/select2-parent/select2/src/main/java/org/wicketstuff/select2/AbstractSelect2Choice.java#L109
>> seems to not throwing any exceptions
>
> Yeah, but another does. This pattern does not make any sense to me:
>
> class Foo {
>
> public Foo(Object a) {
> this(a, null);
> }
>
> public Foo(Object a, Object b) {
> if (b == null)
> throw new IllegalArgumentException();
> this.a = a;
> this.b = b;
> }
> }
>
>
>> 3) you can call constructor mentioned in 2 to avoid exception.
>
> …
>
>
>> 4) I guess renderChoice method is designed to render single choice object,
>> you can override it (for ex. to add your own properties to each choice)
>> other 2 methods are private helpers
>
> renderChoice is called from within renderChoices which is called from within renderHead. But only if !isAjax(), and:
>
> public boolean isAjax()
> {
> return provider != null;
> }
>
> …?
>
> That means: if no ChoiceProvider is set, then use the static List<T> choices, but if you don’t provide a ChoiceProvider in the constructor you’ll get an IllegalArgumentException.
>
> This code is broken and doesn’t make any sense *to me*, or I didn’t get it yet ;-)
>
> My proposal is:
>
> if nobody can tell which problem should be solved with the introduced List<T> choices I would revert it to get a working implementation again, as it is currently unusable.
>
> Personally, I need to upgrade the select.js library in our project, therefor I was looking at wicketstuff-select2, as we still use the original wicket-select2 implementation. The current state of it leaves mit three options, in order of priority:
>
> a)
> convince the wicketstuff-select2 maintainers to revert the code to a working state, which I am currently doing ;-)
>
> b)
> fork wicketstuff-select2 and provide own (working) implementation
>
> c)
> stick with Igor’s original code, which means: no upgrades to select2.js
>
>
> Maybe someone else can shed some light on this?
>
> Cheers,
> -Tom
>
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: users-unsubscribe@wicket.apache.org
> For additional commands, e-mail: users-help@wicket.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@wicket.apache.org
For additional commands, e-mail: users-help@wicket.apache.org
Re: Could not clear select2Choice component model value.
Posted by Tom Götz <to...@decoded.de>.
See my inline comments
> On 30.04.2015, at 04:29, Maxim Solodovnik <so...@gmail.com> wrote:
>
>
> The changes I have made were (most probably) merged from original Igor's
> code (https://github.com/ivaynberg/wicket-select2)
I can’t find the "List<T> choices“ property of AbstractSelect2Choice anywhere in Igor’s original code, so I don’t know where it should have been merged from.
What should be the purpose of having to provide a ChoiceProvider *and* a list of choices?! Currently you have to provide both to avoid an exception upon construction, which is … weird ;) and breaks existing implementations.
> 1) I believe this is since AbstractSelect2Choice is parent class for for
> Single and Multi choices.
That is so, but what does this explain? ;)
> 2) the code was refactored a bit, so line numbers were changed. This
> constructor:
> https://github.com/wicketstuff/core/blob/wicket-6.x/jdk-1.6-parent/select2-parent/select2/src/main/java/org/wicketstuff/select2/AbstractSelect2Choice.java#L109
> seems to not throwing any exceptions
Yeah, but another does. This pattern does not make any sense to me:
class Foo {
public Foo(Object a) {
this(a, null);
}
public Foo(Object a, Object b) {
if (b == null)
throw new IllegalArgumentException();
this.a = a;
this.b = b;
}
}
> 3) you can call constructor mentioned in 2 to avoid exception.
…
> 4) I guess renderChoice method is designed to render single choice object,
> you can override it (for ex. to add your own properties to each choice)
> other 2 methods are private helpers
renderChoice is called from within renderChoices which is called from within renderHead. But only if !isAjax(), and:
public boolean isAjax()
{
return provider != null;
}
…?
That means: if no ChoiceProvider is set, then use the static List<T> choices, but if you don’t provide a ChoiceProvider in the constructor you’ll get an IllegalArgumentException.
This code is broken and doesn’t make any sense *to me*, or I didn’t get it yet ;-)
My proposal is:
if nobody can tell which problem should be solved with the introduced List<T> choices I would revert it to get a working implementation again, as it is currently unusable.
Personally, I need to upgrade the select.js library in our project, therefor I was looking at wicketstuff-select2, as we still use the original wicket-select2 implementation. The current state of it leaves mit three options, in order of priority:
a)
convince the wicketstuff-select2 maintainers to revert the code to a working state, which I am currently doing ;-)
b)
fork wicketstuff-select2 and provide own (working) implementation
c)
stick with Igor’s original code, which means: no upgrades to select2.js
Maybe someone else can shed some light on this?
Cheers,
-Tom
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@wicket.apache.org
For additional commands, e-mail: users-help@wicket.apache.org
Re: Could not clear select2Choice component model value.
Posted by Maxim Solodovnik <so...@gmail.com>.
Hello Tom,
sorry for the brief response (I need to go in 30 minutes, hopefully Martin
can also comment on this)
The changes I have made were (most probably) merged from original Igor's
code (https://github.com/ivaynberg/wicket-select2)
1) I believe this is since AbstractSelect2Choice is parent class for for
Single and Multi choices.
2) the code was refactored a bit, so line numbers were changed. This
constructor:
https://github.com/wicketstuff/core/blob/wicket-6.x/jdk-1.6-parent/select2-parent/select2/src/main/java/org/wicketstuff/select2/AbstractSelect2Choice.java#L109
seems to not throwing any exceptions
3) you can call constructor mentioned in 2 to avoid exception.
4) I guess renderChoice method is designed to render single choice object,
you can override it (for ex. to add your own properties to each choice)
other 2 methods are private helpers
On Wed, Apr 29, 2015 at 6:12 PM, Tom Götz <to...@decoded.de> wrote:
> Sorry, Github link was broken in last email (additional „:“ at the end),
> use this:
>
> https://github.com/wicketstuff/core/commit/92851a253253849582a117d66dee5fcdb15c7353
>
> -Tom
>
>
> > On 29.04.2015, at 14:01, Tom Götz <to...@decoded.de> wrote:
> >
> > Hi Maxim,
> >
> > I do not understand the changes you introduced to
> AbstractSelect2Choice.java in
> https://github.com/wicketstuff/core/commit/92851a253253849582a117d66dee5fcdb15c7353
> :
> > [...]
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: users-unsubscribe@wicket.apache.org
> For additional commands, e-mail: users-help@wicket.apache.org
>
>
--
WBR
Maxim aka solomax
Re: Could not clear select2Choice component model value.
Posted by Tom Götz <to...@decoded.de>.
Sorry, Github link was broken in last email (additional „:“ at the end), use this:
https://github.com/wicketstuff/core/commit/92851a253253849582a117d66dee5fcdb15c7353
-Tom
> On 29.04.2015, at 14:01, Tom Götz <to...@decoded.de> wrote:
>
> Hi Maxim,
>
> I do not understand the changes you introduced to AbstractSelect2Choice.java in https://github.com/wicketstuff/core/commit/92851a253253849582a117d66dee5fcdb15c7353:
> [...]
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@wicket.apache.org
For additional commands, e-mail: users-help@wicket.apache.org