You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@freemarker.apache.org by Woonsan Ko <wo...@apache.org> on 2018/01/03 04:36:16 UTC
[FM3] Continuing Spring MVC Form Directives Support
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.
- 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.
- 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 .../>.
- The other (remaining) tasks seem to be relatively more
straightforward: other inputs, button, textarea, select, etc.
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.
Kind regards,
Woonsan
Re: [FM3] Continuing Spring MVC Form Directives Support
Posted by Daniel Dekany <dd...@apache.org>.
Tuesday, July 24, 2018, 1:30:01 AM, Woonsan Ko wrote:
> Hi,
>
> Just for heads-up, I've come to an end of the first development
> iteration to support spring functions and directives. See the "Spring"
> section in FM3-CHANGE-LOG.txt for detail.
> This is not near perfect yet so I will spend more time improving (unit
> or integration) tests and finding/fixing problems.
> Please feel free to review and test.
I will, eventually. And thanks for all the work put into this! (I was
quite inactive in the recent... months. Will see when can I return to
continue FM3, and the new FM2 release.)
> Cheers,
>
> Woonsan
>
> On Tue, Jan 2, 2018 at 11:36 PM, Woonsan Ko <wo...@apache.org> 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.
>> - 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.
>> - 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 .../>.
>> - The other (remaining) tasks seem to be relatively more
>> straightforward: other inputs, button, textarea, select, etc.
>>
>> 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.
>>
>> Kind regards,
>>
>> Woonsan
>
--
Thanks,
Daniel Dekany
Re: [FM3] Continuing Spring MVC Form Directives Support
Posted by Woonsan Ko <wo...@apache.org>.
Hi,
Just for heads-up, I've come to an end of the first development
iteration to support spring functions and directives. See the "Spring"
section in FM3-CHANGE-LOG.txt for detail.
This is not near perfect yet so I will spend more time improving (unit
or integration) tests and finding/fixing problems.
Please feel free to review and test.
Cheers,
Woonsan
On Tue, Jan 2, 2018 at 11:36 PM, Woonsan Ko <wo...@apache.org> 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.
> - 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.
> - 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 .../>.
> - The other (remaining) tasks seem to be relatively more
> straightforward: other inputs, button, textarea, select, etc.
>
> 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.
>
> Kind regards,
>
> Woonsan
Re: [FM3] Continuing Spring MVC Form Directives Support
Posted by Daniel Dekany <dd...@apache.org>.
Thanks! Note that I have committed some related JavaDoc improvements.
Saturday, January 6, 2018, 5:38:51 AM, Woonsan Ko wrote:
> On Fri, Jan 5, 2018 at 3:31 PM, Daniel Dekany <dd...@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 <dd...@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
>>
>
--
Thanks,
Daniel Dekany
Re: [FM3] Continuing Spring MVC Form Directives Support
Posted by Woonsan Ko <wo...@apache.org>.
On Fri, Jan 5, 2018 at 3:31 PM, Daniel Dekany <dd...@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 <dd...@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
>
Re: [FM3] Continuing Spring MVC Form Directives Support
Posted by Daniel Dekany <dd...@apache.org>.
Friday, January 5, 2018, 8:34:42 PM, Woonsan Ko wrote:
> On Thu, Jan 4, 2018 at 6:13 PM, Daniel Dekany <dd...@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.
>> 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
Re: [FM3] Continuing Spring MVC Form Directives Support
Posted by Woonsan Ko <wo...@apache.org>.
On Thu, Jan 4, 2018 at 6:13 PM, Daniel Dekany <dd...@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
>
Re: [FM3] Continuing Spring MVC Form Directives Support
Posted by Daniel Dekany <dd...@apache.org>.
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
Re: [FM3] Continuing Spring MVC Form Directives Support
Posted by Woonsan Ko <wo...@apache.org>.
Thanks for sharing your insights, Daniel! Please see my comments inline.
On Wed, Jan 3, 2018 at 9:19 AM, Daniel Dekany <dd...@apache.org> 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
>
Re: [FM3] Continuing Spring MVC Form Directives Support
Posted by Daniel Dekany <dd...@apache.org>.
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.
> - 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.
> - 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'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.)
> Kind regards,
>
> Woonsan
>
--
Thanks,
Daniel Dekany