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
