On Fri, Jan 5, 2018 at 3:31 PM, Daniel Dekany <ddek...@apache.org> wrote: > Friday, January 5, 2018, 8:34:42 PM, Woonsan Ko wrote: > >> 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. > > 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 >
