Do we want to do a similar thing for factory methods around models, such as LoadableDetachableModel#of?
Andrew On Wed, Feb 8, 2017 at 2:53 PM, Andrea Del Bene <[email protected]> wrote: > I think we all (more or less :-) ) agree about moving factory methods away > from components. I don't think we need a dedicated and experimental module > for them. I'd rather move them to class Lamdas and, if we need it, we can > deprecate them in future. > > > > On 08/02/2017 17:24, Sven Meier wrote: > >> I like Edmond's suggestion: >> >> "We can also move them all to a factory class and place this class in >> experimental. >> That way we can do with it whatever we like." >> >> So we have a place to refer questioners to when they are looking for >> these factory methods. >> >> Regards >> Sven >> >> >> On 08.02.2017 16:55, Martin Grigorov wrote: >> >>> https://github.com/apache/wicket/pull/211 :-) >>> Let's drop them all! >>> >>> Martin Grigorov >>> Wicket Training and Consulting >>> https://twitter.com/mtgrigorov >>> >>> On Wed, Feb 8, 2017 at 4:34 PM, Emond Papegaaij < >>> [email protected]> >>> wrote: >>> >>> I can live with the following: >>>> >>>> Factory methodes should: >>>> * accept exactly 1 lambda (and possibly an id) >>>> * be a static method on the component or behavior they create >>>> * be given meaningfull names >>>> * be limited to at most 1 or 2 per type >>>> * not pass the instance they create to the lambda >>>> >>>> This would mean all methods should be moved from Lambdas and duplicates >>>> like >>>> AjaxLink.onClick should be removed. >>>> >>>> Still, I like the idea to remove them all better. We can also move them >>>> all to >>>> a factory class and place this class in experimental. That way we can do >>>> with >>>> it whatever we like. >>>> >>>> Best regards, >>>> Emond >>>> >>>> On woensdag 8 februari 2017 16:20:59 CET Andrea Del Bene wrote: >>>> >>>>> I agree with Martijn, especially when he warns about the huge risk we >>>>> >>>> take >>>> >>>>> overusing lambdas and making API inconsistent. But I don't agree to >>>>> take >>>>> such an extreme choice like removing EVERY factory methods. Like Martin >>>>> suggested these methods can be very useful when they are limited to the >>>>> very basic functionality of a component (link onClick for a Link). >>>>> More in general I would say that a lambda factory method should not >>>>> take >>>>> more than one lambda as parameter. >>>>> >>>>> On Wed, Feb 8, 2017 at 3:17 PM, Martin Grigorov <[email protected]> >>>>> >>>>> wrote: >>>>> >>>>>> On Wed, Feb 8, 2017 at 3:08 PM, Martijn Dashorst < >>>>>> [email protected] >>>>>> >>>>>> wrote: >>>>>>> >>>>>>> There are many things to consider when designing an API. Usability of >>>>>>> the API, readability of the resulting code, maintenance burden for >>>>>>> >>>>>> the >>>> >>>>> core developers, but also performance, memory consumption and >>>>>>> serialization overhead. >>>>>>> >>>>>>> Adding lifecycle fields to Component for any extension point will >>>>>>> dramatically increase the footprint of any Wicket application. >>>>>>> Lambda's are not a free lunch and add considerable memory and >>>>>>> serialization overhead to Java code. >>>>>>> >>>>>>> Wicket is not only used for small demos of how fancy lambda's can be, >>>>>>> but also for applications where 10's of thousands or even millions of >>>>>>> users concurrently use the application. Adding a byte to the >>>>>>> >>>>>> footprint >>>> >>>>> of Component can increase the memory footprint of the application >>>>>>> >>>>>> with >>>> >>>>> 10's of megabytes. >>>>>>> >>>>>>> So yes we have considered adding these things but find the problem >>>>>>> they solve not worth the extra footprint. Furthermore the clarity of >>>>>>> the code does not increase when you have 3 or 4 methods that need >>>>>>> implementing, but those add considerable memory footprint (8 bytes >>>>>>> >>>>>> per >>>> >>>>> reference when unused, much more when used) when provided through >>>>>>> lambdas. >>>>>>> >>>>>>> Calculation >>>>>>> >>>>>>> Say you have 1 million components in memory. Say we add lambda's for >>>>>>> the primary life cycle events and enabled/visible flags. This is: >>>>>>> >>>>>>> - onInitialize >>>>>>> - onEvent >>>>>>> - onReAdd >>>>>>> - onRemove >>>>>>> - onRender >>>>>>> - onConfigure >>>>>>> - onBeforeRender >>>>>>> - onBeforeRenderChildren >>>>>>> - onComponentTag >>>>>>> - onComponentTagBody >>>>>>> - onAfterRender >>>>>>> - onAfterRenderChildren >>>>>>> - onDetach >>>>>>> - isVisible >>>>>>> - isEnabled >>>>>>> >>>>>>> This is without onFoo methods available in Link, Form etc. 15 >>>>>>> references that may or may not be used. 15*8 = 120 bytes per >>>>>>> component. Now we have added 120MB to an application without anyone >>>>>>> even using the functionality. The cost is considerably higher when >>>>>>> actually using the lambda's. See the measurements I did for my talk >>>>>>> >>>>>> at >>>> >>>>> ApacheCon 2016 in Sevilla [1] >>>>>>> >>>>>>> Of course we can try to minimize the memory footprint, and store the >>>>>>> event references in the data bucket of Component and store only those >>>>>>> events that are actually used. But this will add an O(n^2) for every >>>>>>> request because we need to loop through the data array at each life >>>>>>> cycle event to find if there's a registered reference, and if so use >>>>>>> that. While for a single request this is not too big an issue, it >>>>>>> >>>>>> adds >>>> >>>>> up when you consider that this must be done for millions of >>>>>>> >>>>>> components >>>> >>>>> for all lifecycle events. >>>>>>> >>>>>>> API design >>>>>>> >>>>>>> So let's consider the case when we were to add setters for lambda's >>>>>>> >>>>>> to >>>> >>>>> Component et al. We can't remove the old way of subclassing because >>>>>>> that would break the world. Thus we have two ways of how to add an >>>>>>> onConfigure or onDetach event to a component. Which one is canon? We >>>>>>> already have at least 3 ways to determine whether a component is >>>>>>> visible or not: >>>>>>> >>>>>>> - call setVisible (static visibility) >>>>>>> - override isVisible >>>>>>> - setVisibilityAllowed >>>>>>> - override onConfigure() and call setVisible >>>>>>> >>>>>>> This has harmed the Wicket API in my opinion and is a result of us >>>>>>> >>>>>> not >>>> >>>>> understanding at the time how, when and how often visibility needed >>>>>>> >>>>>> to >>>> >>>>> be evaluated. >>>>>>> >>>>>>> Adding setters for lambda's would at least double the API surface. >>>>>>> Furthermore you need to take into account that these setters can't be >>>>>>> hidden by subclasses. So when you use a component that has >>>>>>> >>>>>> onConfigure >>>> >>>>> overridden, what happens when you setOnConfigure()? >>>>>>> >>>>>>> public class FooPanel extends Panel { >>>>>>> >>>>>>> public FooPanel(String id) { super(id); } >>>>>>> >>>>>>> @Override >>>>>>> protected void onConfigure() { >>>>>>> >>>>>>> super.onConfigure(); >>>>>>> // if something set visiblity >>>>>>> >>>>>>> } >>>>>>> >>>>>>> } >>>>>>> >>>>>>> add(new FooPanel("foo").setOnConfigure(() -> { ... do something >>>>>>> completely different ... }); >>>>>>> >>>>>>> So which one should prevail? the lambda or the overridden method? Ah >>>>>>> >>>>>> I >>>> >>>>> hear you say "rewrite the FooPanel such that it doesn't override >>>>>>> onConfigure". But then we have 2 onConfigure lambda's set: >>>>>>> >>>>>>> public class FooPanel extends Panel { >>>>>>> >>>>>>> public FooPanel(String id) { >>>>>>> >>>>>>> super(id); >>>>>>> setOnConfigure(() -> { >>>>>>> >>>>>>> /* if something set visibility */ >>>>>>> >>>>>>> }); >>>>>>> >>>>>>> } >>>>>>> >>>>>>> } >>>>>>> >>>>>>> add(new FooPanel("foo").setOnConfigure(() -> { ... do something >>>>>>> completely different ... }); >>>>>>> >>>>>>> So which one should prevail? Well, call both! OK, this means that we >>>>>>> need to store both lambdas. So each lifecycle event lambda store >>>>>>> >>>>>> needs >>>> >>>>> to be a data structure that can hold multiple lambdas, which >>>>>>> >>>>>> increases >>>> >>>>> the memory footprint even further. Then we have to issue of which >>>>>>> lambda to call first. The one added in the constructor of FooPanel, >>>>>>> >>>>>> or >>>> >>>>> the one that was set after construction? >>>>>>> >>>>>>> This doesn't even take into account the semantics of for example the >>>>>>> onInitialize lifecycle event, which states that it is called when a >>>>>>> component is added to the hierarchy of a page (i.e. a parent-path >>>>>>> exists to the page). A setOnInitialize can be invoked at any moment >>>>>>> after construction. Therefore its semantics become even muddier than >>>>>>> they already are: >>>>>>> >>>>>>> Label label = new Label(...); >>>>>>> add(label); >>>>>>> >>>>>>> label.setOnInitialize(() -> { ... }); >>>>>>> >>>>>>> OK, so we don't add a lambda possibility for onInitialize (queue the >>>>>>> pull requests). >>>>>>> >>>>>>> Another thing to consider is that a component builder currently can >>>>>>> decide to close off particular events from being overridden or >>>>>>> modified. For example: >>>>>>> >>>>>>> public class FooPanel extends Panel { >>>>>>> >>>>>>> public FooPanel(String id) { super(id); } >>>>>>> >>>>>>> /** No longer overridable */ >>>>>>> @Override >>>>>>> protected final void onConfigure() { >>>>>>> >>>>>>> super.onConfigure(); >>>>>>> // if something set visiblity >>>>>>> >>>>>>> } >>>>>>> >>>>>>> } >>>>>>> >>>>>>> The component designer found it necessary to remove the ability to >>>>>>> override onConfigure(). >>>>>>> >>>>>>> However you can't remove the ability to add/set lambda's from >>>>>>> Component. These methods need to be public for them to be usable, and >>>>>>> you can't hide those methods. >>>>>>> >>>>>>> Summary >>>>>>> >>>>>>> Lambdas are a great tool but should be used wisely and we should be >>>>>>> careful about our API and take good design into consideration instead >>>>>>> of just papering our API with lambdas. There are many aspects to >>>>>>> consider, not just the fact that you can write 2 lines of >>>>>>> non-executable code less. >>>>>>> >>>>>> YES! >>>>>> That's why I think the current factories we have is the best we can >>>>>> >>>>> give - >>>> >>>>> they are very handy for (my?!) the trivial case and they cannot be >>>>>> extended >>>>>> further with more functionality, it is just not practical to add more >>>>>> >>>>> to >>>> >>>>> them. >>>>>> For more inspiration take a look at other UI libs like >>>>>> >>>>> JavaFX/TornadoFX, >>>> >>>>> Vaadin 8, Scalatags/Kotlinx.html, ... >>>>>> >>>>>> Martijn >>>>>>> >>>>>>> [1] http://www.slideshare.net/dashorst/whats-up-with-wicket- >>>>>>> >>>>>> 8-and-java-8 >>>> >>>>> On Wed, Feb 8, 2017 at 11:45 AM, Andrew Geery < >>>>>>> >>>>>> [email protected]> >>>> >>>>> wrote: >>>>>>> >>>>>>>> Rather than using static factory methods, would we ever consider >>>>>>>> >>>>>>> pushing >>>>>> >>>>>> the lambdas into the component classes themselves? >>>>>>>> >>>>>>>> For example, if we did this with the Button class, the change would >>>>>>>> be: >>>>>>>> >>>>>>>> - Add two private fields, SerializableConsumer<Button> >>>>>>>> >>>>>>> submitHandler, >>>> >>>>> errorHandler >>>>>>>> - Add "setters" for these fields -- e.g., Button >>>>>>>> submitHandler(SerializableConsumer<Button> submitHandler) >>>>>>>> - Modify the onSubmit() and onError() methods to call the handler >>>>>>>> >>>>>>> methods >>>>>> >>>>>> if they are not null >>>>>>>> >>>>>>>> A call would be something like: >>>>>>>> >>>>>>>> add(new Button("button").submitHandler(b -> >>>>>>>> >>>>>>> System.out.println("clicked")); >>>>>>> >>>>>>> Obviously, it is still possible to sub-class Button and override >>>>>>>> onSubmit(), so existing code will continue to work. However, for >>>>>>>> >>>>>>> code >>>> >>>>> going forward, it should be much less needed and, in my opinion, >>>>>>>> >>>>>>> much >>>> >>>>> clearer. >>>>>>>> >>>>>>>> Another advantage to doing things this way is that sub-classes >>>>>>>> >>>>>>> inherit >>>> >>>>> these methods and there is no need to add static factory methods >>>>>>>> >>>>>>> for >>>> >>>>> every >>>>>>> >>>>>>> sub-class (sub-classes of AjaxButton would be a better example of >>>>>>>> >>>>>>> this). >>>>>> >>>>>> Thanks >>>>>>>> Andrew >>>>>>>> >>>>>>>> On Wed, Feb 8, 2017 at 3:42 AM, Martijn Dashorst < >>>>>>>> >>>>>>> [email protected] >>>>>>> >>>>>>> wrote: >>>>>>>>> >>>>>>>>> It is that your trivial use case is not my trivial use case and >>>>>>>>> >>>>>>>> that >>>> >>>>> we will end up with a 100,000 trivial use cases. >>>>>>>>> >>>>>>>>> And no, confusion is not the big issue (though for >>>>>>>>> >>>>>>>> onsubmit+onerror >>>> >>>>> it >>>>>>>>> is) but creating a good API is hard. It takes time and >>>>>>>>> >>>>>>>> understanding. >>>> >>>>> Minimizing lines of code is not the only metric for a good API. >>>>>>>>> >>>>>>>>> Just as using inheritance/annotations/generics for everything is >>>>>>>>> >>>>>>>> bad, >>>> >>>>> introducing factory methods everywhere will not age well. >>>>>>>>> >>>>>>>>> These methods are trivial for anyone to implement and should we >>>>>>>>> >>>>>>>> reach >>>> >>>>> the conclusion that we actually need the factory methods in core, >>>>>>>>> >>>>>>>> it >>>> >>>>> is trivial to refactor them towards the Wicket supplied factories. >>>>>>>>> >>>>>>>>> Are the factory methods in the way? Yes they are, because once we >>>>>>>>> >>>>>>>> add >>>> >>>>> them, we can't evolve them without adding many more (introduce >>>>>>>>> >>>>>>>> bloat) >>>> >>>>> or having to wait until a new major release. >>>>>>>>> >>>>>>>>> Martijn >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> On Tue, Feb 7, 2017 at 11:40 PM, Martin Grigorov < >>>>>>>>> >>>>>>>> [email protected]> >>>>>> >>>>>> wrote: >>>>>>>>> >>>>>>>>>> Hi, >>>>>>>>>> >>>>>>>>>> These methods are factories for the trivial use cases where >>>>>>>>>> >>>>>>>>> nothing >>>> >>>>> else >>>>>>> >>>>>>> needs to be overridden but the action method (methods, when >>>>>>>>>> >>>>>>>>> there >>>> >>>>> is >>>>>>>>>> onError()). They do help to reduce the verbosity! >>>>>>>>>> There are plenty of those cases. You can see many usages in >>>>>>>>>> wicket-examples. I have used such methods from >>>>>>>>>> >>>>>>>>> wicketstuff-scala in >>>> >>>>> one >>>>>>> >>>>>>> of >>>>>>>>> >>>>>>>>> my projects in the past and I use similar ones in a project with >>>>>>>>>> >>>>>>>>> Kotlin >>>>>>> >>>>>>> now. >>>>>>>>> >>>>>>>>> A builder that provides methods like onConfigure(), >>>>>>>>>> >>>>>>>>> onComponentTag(), >>>>>> >>>>>> onDetach(), ... would look really strange! >>>>>>>>>> Who will use it instead of just creating a (inner) class ?! >>>>>>>>>> >>>>>>>>>> But if those factory methods confuse core developers then they >>>>>>>>>> >>>>>>>>> will >>>> >>>>> be >>>>>> >>>>>> even >>>>>>>>> >>>>>>>>> more confusing to normal users :-/ >>>>>>>>>> >>>>>>>>>> 0 from me. >>>>>>>>>> >>>>>>>>>> Martin Grigorov >>>>>>>>>> Wicket Training and Consulting >>>>>>>>>> https://twitter.com/mtgrigorov >>>>>>>>>> >>>>>>>>>> On Tue, Feb 7, 2017 at 5:07 PM, Martijn Dashorst < >>>>>>>>>> >>>>>>>>> [email protected] >>>>>>>>> >>>>>>>>> wrote: >>>>>>>>>>> >>>>>>>>>>> All, >>>>>>>>>>> >>>>>>>>>>> I want to remove all component factory methods that are added >>>>>>>>>>> >>>>>>>>>> for >>>> >>>>> lambda's. >>>>>>>>> >>>>>>>>> My reasoning is that: >>>>>>>>>>> - removing them in 8.x (x > 0) is impossible >>>>>>>>>>> - adding them in 8.x (x > 0) is trivial >>>>>>>>>>> - we currently don't have a way to know what good idioms are >>>>>>>>>>> - having these factories push (new) developers to use the wrong >>>>>>>>>>> >>>>>>>>>> idiom >>>>>> >>>>>> - factory methods don't allow for additional overriding, thus a >>>>>>>>>>> combinatorial explosion of API >>>>>>>>>>> - I tend to think that builders are better suited as component >>>>>>>>>>> >>>>>>>>>> factories >>>>>>> >>>>>>> Because it is trivial to re-introduce these factories or their >>>>>>>>>>> replacements at a later time, I propose to remove them now and >>>>>>>>>>> >>>>>>>>>> figure >>>>>> >>>>>> out in our actual applications what works, and what doesn't. I >>>>>>>>>>> also >>>>>>>>>>> think that just doing the conversion from W7 to W8 isn't >>>>>>>>>>> >>>>>>>>>> enough to >>>> >>>>> figure this out. >>>>>>>>>>> >>>>>>>>>>> Example 1: >>>>>>>>>>> >>>>>>>>>>> Button.java has two factory methods: >>>>>>>>>>> >>>>>>>>>>> Button#onSubmit(String, SerializableConsumer<Button>), and >>>>>>>>>>> Button#onSubmit(String, SerializableConsumer<Button>, >>>>>>>>>>> SerializableConsumer<Button>) >>>>>>>>>>> >>>>>>>>>>> It is not possible to see without resorting to parameter >>>>>>>>>>> >>>>>>>>>> naming, >>>> >>>>> hovering etc to see which functionality is bound by which >>>>>>>>>>> >>>>>>>>>> parameter. >>>>>> >>>>>> I >>>>>>> >>>>>>> find the naming confusing: onSubmit and onSubmit. >>>>>>>>>>> >>>>>>>>>>> Example 2: >>>>>>>>>>> >>>>>>>>>>> Behavior.java has two factory methods: >>>>>>>>>>> >>>>>>>>>>> Behavior onTag(SerializableBiConsumer<Component, >>>>>>>>>>> >>>>>>>>>> ComponentTag>) >>>> >>>>> Behavior onAttribute(String name, >>>>>>>>>>> SerializableFunction<String, CharSequence> onAttribute) >>>>>>>>>>> >>>>>>>>>>> These achieve basically the same functionality, but other life >>>>>>>>>>> >>>>>>>>>> cycle >>>>>> >>>>>> events of Behavior don't have factory methods. NOR should they. >>>>>>>>>>> >>>>>>>>>>> Example 3: >>>>>>>>>>> >>>>>>>>>>> Lambdas.java has many factory methods, most of which are better >>>>>>>>>>> implemented by just using an anonymous inner class. For >>>>>>>>>>> >>>>>>>>>> example, >>>> >>>>> Lambdas.link: often times you need to override onconfigure or >>>>>>>>>>> oncomponenttag as well as onclick. >>>>>>>>>>> >>>>>>>>>>> Martijn >>>>>>>>>>> >>>>>>>>>> -- >>>>>>>>> Become a Wicket expert, learn from the best: >>>>>>>>> >>>>>>>> http://wicketinaction.com >>>>>> >>>>>> -- >>>>>>> Become a Wicket expert, learn from the best: >>>>>>> >>>>>> http://wicketinaction.com >>>> >>>> >>>> >>>> >> >
