Thanks! Note that I have committed some related JavaDoc improvements.
Saturday, January 6, 2018, 5:38:51 AM, Woonsan Ko wrote: > On Fri, Jan 5, 2018 at 3:31 PM, Daniel Dekany <[email protected]> wrote: >> Friday, January 5, 2018, 8:34:42 PM, Woonsan Ko wrote: >> >>> On Thu, Jan 4, 2018 at 6:13 PM, Daniel Dekany <[email protected]> wrote: >>>> Thursday, January 4, 2018, 9:01:24 PM, Woonsan Ko wrote: >>>> >>>> [snip] >>>>>> Maybe, instead of >>>>>> SomeSuperTemplateDirectiveModel.NAMED_ARGS_ENTRY_LIST, your could >>>>>> just expose ARGS_LAYOUT, and get the highest named argument index >>>>>> from that. Then NAMED_ARGS_ENTRY_LIST need not exist at all >>>>>> (especially as a List instead of as an array), unless I miss >>>>>> something there. >>>>> >>>>> I change the list to array and mark it as private, exposing >>>>> ARGS_LAYOUT only, as you suggested. Now, it's cleaner. >>>>> As the Entry array of parent directive class needs to be prepended to >>>>> the predefinedNamedArgumentsMap (StringToIndexMap) in the child, I >>>>> added StringToIndexMap#getInputEntries(). Hope it should be fine. >>>> >>>> I believe the concatenated Entry[] array shouldn't be constructed in >>>> the custom directive (with _ArrayUtils.addAll and getInputEntries()) >>>> on the first place. Such low level things could be pushed on >>>> StringToIndexMap. StringToIndexMap could have a constructor method >>>> like `of(StringToIndexMap inherited, Entry... additionalEntries)`. >>>> This constructs a concatenated Entry[] internally (see >>>> StringToIndexMap.checkIndexRange to see how to traverse all inherited >>>> entries efficiently), and calls `of(Entry... entries)` with it. Now >>>> the need for `inputEntries` is gone as well. In the custom directive >>>> you just call this new overload of `StringToIndexMap.of`, instead of >>>> the usual `StringToIndexMap.of`. So it looks almost the same as when >>>> there's no named parameter inheritance. >>> >>> Yes. That's where I was a bit doubtful in the first place, I fixed it >>> based on your suggestion. >> >> Small thing, but in the new `of` method you could get away just with a >> single array, without creating a List and the converting it back to an >> array. > > Nice catch! > >> >>>> Also, I think NAMED_ARGS_ENTRIES can be removed; you can just call >>>> `StringToIndexMap.of` directly in the ArgumentArrayLayout.create >>>> parameter. >>> >>> Done as well. >>> >>>> >>>> I wonder if you should just move getLastPredefinedNamedArgumentIndex() >>>> into ArgumentArrayLayout. Or if not, into CallableUtils at least. The >>>> problem is not specific to Spring after all. >>> >>> Due to the assumption described in the javadoc, I moved it to >>> CallableUtils. All the callable implementation could understand the >>> assumptions. >> >> Couldn't it simply give the correct result for all possible >> ArgumentArrayLayout, instead of assuming things though? Also, now that >> I'm thinking about it, it should rather be >> getPredefinedNamedArgumentsEndIndex, with an exclusive end, otherwise >> it will be strange when there are 0 predefined named arguments. > > You're right. I overlooked the javadoc of ArgumentArrayLayout#create() > for the predefinedNamedArgumentsMap param which has already contained > the hint. > And, yes, ArgumentArrayLyaout#getPredefinedNamedArgumentsEndIndex() is better. > I pushed two commits to fix those. > > Thanks, > > Woonsan > >> >>> Cheers, >>> >>> Woonsan >>> >>>> >>>>>>> - spring:* tags are mapped to "spring.*" models, whereas form:* tags >>>>>>> are mapped to "spring.form.*" models. I didn't want to expose "form" >>>>>>> model separately in a higher level. I found out it's convenient to >>>>>>> write <#assign form=spring.form /> in order to use <@form.input .../> >>>>>>> instead of <@spring.form.input .../>. >>>>>> >>>>>> While that's the theoretically correct way, I think people will either >>>>>> end up starting the templates with <#assign form=spring.form />, or >>>>>> dislike FreeMarker for forcing them to write "spring.form" again and >>>>>> again. Then it's better just to expose "form" directly. >>>>> >>>>> Makes sense. I updated that. >>>>> >>>>>> >>>>>>> - The other (remaining) tasks seem to be relatively more >>>>>>> straightforward: other inputs, button, textarea, select, etc. >>>>>> >>>>>> Something I have noticed that the Spring JSP taglib uses >>>>>> all-lower-case convention for attributes, like "onselect" instead of >>>>>> "onSelect". That's alien from FreeMarker 3, and from modern Java and >>>>>> JavaScript conventions too. So it might be worth it to change that. >>>>>> Then we should make error message generation more intelligent in >>>>>> freemarker-core, so that it tells you the correct parameter name >>>>>> automatically if only the case differs. >>>>> >>>>> I guess Spring decided to use the same default HTML attribute names, >>>>> with an exception for non-HTML attributes such as "cssErrorClass" or >>>>> conflicting names such as "cssClass". >>>>> - https://www.w3.org/TR/html4/index/attributes.html >>>> >>>> You are right. Let's leave it that way then. >>>> >>>>> This principle seems fine to me, and I guess Spring MVC devs might >>>>> prefer the original style for html attributes. >>>>> >>>>>> >>>>>>> I'll probably merge the PR by myself in a day or two and continue with >>>>>>> other directives while being willing to hear any feedback and >>>>>>> suggestions. >>>>>> >>>>>> BTW, especially in this part where you are the main author, I surely >>>>>> don't expect you to "stage" a change before merging, so just go ahead >>>>>> whenever you feel like it. (The added delay just increase the chance >>>>>> of conflicts.) >>>>> >>>>> OK. >>>>> >>>>> Thanks again, >>>> >>>> Thank you! >>>> >>>>> Woonsan >>>>> >>>>>> >>>>>>> Kind regards, >>>>>>> >>>>>>> Woonsan >>>>>>> >>>>>> >>>>>> -- >>>>>> Thanks, >>>>>> Daniel Dekany >>>>>> >>>>> >>>> >>>> -- >>>> Thanks, >>>> Daniel Dekany >>>> >>> >> >> -- >> Thanks, >> Daniel Dekany >> > -- Thanks, Daniel Dekany
