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 <t...@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 <t...@decoded.de> wrote:
> >
> > See my inline comments
> >
> >> On 30.04.2015, at 04:29, Maxim Solodovnik <solomax...@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-unsubscr...@wicket.apache.org
> > For additional commands, e-mail: users-h...@wicket.apache.org
> >
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: users-unsubscr...@wicket.apache.org
> For additional commands, e-mail: users-h...@wicket.apache.org
>
>

Reply via email to