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 > >
