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