On Thu, Jan 4, 2018 at 6:13 PM, Daniel Dekany <ddek...@apache.org> 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. > > 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. 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 >
