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. Also, I think NAMED_ARGS_ENTRIES can be removed; you can just call `StringToIndexMap.of` directly in the ArgumentArrayLayout.create parameter. 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. >>> - 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
