Thanks for sharing your insights, Daniel! Please see my comments inline. On Wed, Jan 3, 2018 at 9:19 AM, Daniel Dekany <[email protected]> wrote: > Wednesday, January 3, 2018, 5:36:16 AM, Woonsan Ko wrote: > >> Hi, >> >> It has been a long time since my last commit to support spring mvc >> functions/directives (FREEMARKER-55). >> Today, I've opened a pull request, containing an initial form related >> work (only form:form and form:input directives for now, but those are >> parts of the crucial basement): >> - https://github.com/apache/incubator-freemarker/pull/41 >> >> Please feel free to take a look and let me know if you have any >> comments or remarks. >> >> Some pointers: >> - Initially I tried to leverage dynamic varargs for almost every html >> attribute, but I ended up defining each available attribute as member >> like Spring taglibs do, which seems clearer in both argument layout >> and maintenance. > > It's also faster (unless you end up with a really long mostly-empty > argument array). > >> - Arguments for html attributes are kind of inherited from base >> classes such as AbstractHtmlElementTemplateDirectiveModel. I ended up >> merging named arg list in child directives by accessing static >> members, which is not totally clean - static members are visible >> anywhere, but that's what I think is the best for now. > > Yeah, I have noticed some quite involved "argument index arithmetic" > there. 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. > >> - 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 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, Woonsan > >> Kind regards, >> >> Woonsan >> > > -- > Thanks, > Daniel Dekany >
