OFBIZ-11900: Approach so far for MacroFormRenderer refactoring

[Daniel Watford]

Hello,

OFBIZ-11900 covers the work-in-progress effort to refactor
MacroFormRenderer by extracting code which builds strings to be executed as
FTL macros into a separate builder class.

This github branch [1] contains the work-in-progress changes for this
issue. Please take a look at this branch and highlight any concerns with
the approach. This will give me a chance to address concerns or change
approach before too much further work is done.

Further to simply extracting code from MacroFormRenderer to a separate
builder I wanted to avoid the use of strings to represent FTL macro calls.
Instead I added an interface called RenderableFtl to represent an element
that can be rendered as part of a FreeMarker template.

So far the main RenderableFtls in use are RenderableFtlMacroCall and
RenderableFtlString.

RenderableFtlString can be used to represent a pre-rendered HTML string as
used to construct hidden forms by WidgetWorker.

RenderableFtlMacroCall is probably the most important of the RenderableFtl
classes as it can be used in place of the strings previously built in
MacroFormRenderer to capture an FTL macro call, but without the need for
the caller to append quoting and whitespace delimiters for each additional
macro call parameter.

All the RenderableFtl classes know how to render themselves to a String
which will be used by FtlWriter to process the string as an FTL element and
then write the result to an Appendable.

The reason for these changes are to separate the various concerns involved
in rendering a form:
- RenderableFtl objects are responsible for rendering their data to an FTL
compliant string.
- FtlWriter is responsible for processing FTL compliant strings as part of
an FTL template and writing the results to an Appendable.
- RenderableFtlFormElementsBuilder is responsible for creating
RenderableFtl objects for various FieldInfo objects, replacing the FTL
macro call strings which were previously built in MacroFormRenderer.
- MacroFormRenderer is responsible for orchestrating calls to
RenderableFtlFormElementsBuilder to create multiple RenderableFtl objects
as needed for rendering a field and passing them to FtlWriter. For example,
rendering a DisplayEntityField produces RenderableFtl objects representing
the displayed field, an associated hyperlink and a tooltip.

Separating these concerns makes testing easier:
- MacroFormRendererTest can test orchestration of MacroFormRender's calls
to RenderableFtlFormElementsBuilder, rather than having to analyse the
strings previously written.
- Testing of RenderableFtlFormElementsBuilder focuses on the production of
correctly configured RenderableFtl elements based on the given FieldInfo
objects, rather than testing the eventual string rendered.
- The RenderableFtl objects can be tested by examining the FTL strings
produced.

To support testing of RenderableFtlFormElementsBuilder a number of Hamcrest
Matchers have been created to check that RenderableFtlMacroCall objects
have expected name and parameter values.

Please excuse the long message :) and please let me know if you have any
comments or concerns about the approach I have taken.

Thanks,

Dan.

[1] - https://github.com/danwatford/ofbiz-framework/tree/OFBIZ-11900-WIP

--
Daniel Watford

[Nicolas Malin-2]

Hello Daniel,

I'm a little confuse to take more than 6 month to response.

You push me on my currently java knowledge limit, and after analyze I
didn't understand all you done.

However, your works feel good.

During my works on the OFBIZ-11810 [1], I confronted to many duplicated
code between Screen, Form and Menu render. This case help me to
understand an advantage of your refacto.

I'm clearly in favor to continue the refactoring, and I have a question.

Because we works on the same code what do you prefer ?

 * I commit my work with the current code and we migrate it after

 * We migrate before the code and I update my work on it

 * I try to convert my current improvement with your refactoring proposal.

Nicolas

[1] https://issues.apache.org/jira/browse/OFBIZ-11810

On 28/07/2020 19:49, Daniel Watford wrote:

> Hello,
>
> OFBIZ-11900 covers the work-in-progress effort to refactor
> MacroFormRenderer by extracting code which builds strings to be executed as
> FTL macros into a separate builder class.
>
> This github branch [1] contains the work-in-progress changes for this
> issue. Please take a look at this branch and highlight any concerns with
> the approach. This will give me a chance to address concerns or change
> approach before too much further work is done.
>
> Further to simply extracting code from MacroFormRenderer to a separate
> builder I wanted to avoid the use of strings to represent FTL macro calls.
> Instead I added an interface called RenderableFtl to represent an element
> that can be rendered as part of a FreeMarker template.
>
> So far the main RenderableFtls in use are RenderableFtlMacroCall and
> RenderableFtlString.
>
> RenderableFtlString can be used to represent a pre-rendered HTML string as
> used to construct hidden forms by WidgetWorker.
>
> RenderableFtlMacroCall is probably the most important of the RenderableFtl
> classes as it can be used in place of the strings previously built in
> MacroFormRenderer to capture an FTL macro call, but without the need for
> the caller to append quoting and whitespace delimiters for each additional
> macro call parameter.
>
> All the RenderableFtl classes know how to render themselves to a String
> which will be used by FtlWriter to process the string as an FTL element and
> then write the result to an Appendable.
>
> The reason for these changes are to separate the various concerns involved
> in rendering a form:
> - RenderableFtl objects are responsible for rendering their data to an FTL
> compliant string.
> - FtlWriter is responsible for processing FTL compliant strings as part of
> an FTL template and writing the results to an Appendable.
> - RenderableFtlFormElementsBuilder is responsible for creating
> RenderableFtl objects for various FieldInfo objects, replacing the FTL
> macro call strings which were previously built in MacroFormRenderer.
> - MacroFormRenderer is responsible for orchestrating calls to
> RenderableFtlFormElementsBuilder to create multiple RenderableFtl objects
> as needed for rendering a field and passing them to FtlWriter. For example,
> rendering a DisplayEntityField produces RenderableFtl objects representing
> the displayed field, an associated hyperlink and a tooltip.
>
> Separating these concerns makes testing easier:
> - MacroFormRendererTest can test orchestration of MacroFormRender's calls
> to RenderableFtlFormElementsBuilder, rather than having to analyse the
> strings previously written.
> - Testing of RenderableFtlFormElementsBuilder focuses on the production of
> correctly configured RenderableFtl elements based on the given FieldInfo
> objects, rather than testing the eventual string rendered.
> - The RenderableFtl objects can be tested by examining the FTL strings
> produced.
>
> To support testing of RenderableFtlFormElementsBuilder a number of Hamcrest
> Matchers have been created to check that RenderableFtlMacroCall objects
> have expected name and parameter values.
>
> Please excuse the long message :) and please let me know if you have any
> comments or concerns about the approach I have taken.
>
> Thanks,
>
> Dan.
>
> [1] - https://github.com/danwatford/ofbiz-framework/tree/OFBIZ-11900-WIP
>

[Daniel Watford]

Hi Nicolas,

OFBIZ-11900 [1] has been merged now, so there shouldn't be any sort of
clash with OFBIZ-11810.

OFBIZ-11900 is part of the OFBIZ-11456 [2] refactoring effort, but none of
the other child tasks from OFBIZ-11456 are currently being worked on.
Therefore I think you should be able to proceed with your changes without
concern of impacting any changes that might be in flight.

I had a quick read of the patch in OFBIZ-11810 and cannot see anything that
needs to change with regard to the MacroScreenRenderer  refactoring effort.
I did double check MacroCommonRenderer#createAjaxParamsFromUpdateAreas
again RenderableFtlFormElementsBuilder#createAjaxParamsFromUpdateAreas, but
I believe MCR#cAPFUA is an unaltered extraction of
MacroScreenRenderer#createAjaxParamsFromUpdateAreas so shouldn't be any
problem there.

I don't fully understand how update areas work yet, so I would be grateful
if you could have a look at the public methods in
RenderableFtlFormElementsBuilder to see if any of the RenderableFtl
produced there for form elements need to be altered to support your area
update changes. In particular methods makeHyperlinkString,
makeHyperlinkByType and hyperlinkMacroCall should probably be checked.

Thank you for asking about this. It will renew my efforts to understand
update areas and continue with the refactor once your changes have been
applied to trunk.

Thanks,

Dan.

[1] https://issues.apache.org/jira/browse/OFBIZ-11900

[2] https://issues.apache.org/jira/browse/OFBIZ-11456



On Tue, 9 Feb 2021 at 16:01, Nicolas Malin <[hidden email]> wrote:

> Hello Daniel,
>
> I'm a little confuse to take more than 6 month to response.
>
> You push me on my currently java knowledge limit, and after analyze I
> didn't understand all you done.
>
> However, your works feel good.
>
> During my works on the OFBIZ-11810 [1], I confronted to many duplicated
> code between Screen, Form and Menu render. This case help me to
> understand an advantage of your refacto.
>
> I'm clearly in favor to continue the refactoring, and I have a question.
>
> Because we works on the same code what do you prefer ?
>
>  * I commit my work with the current code and we migrate it after
>
>  * We migrate before the code and I update my work on it
>
>  * I try to convert my current improvement with your refactoring proposal.
>
> Nicolas
>
> [1] https://issues.apache.org/jira/browse/OFBIZ-11810
>
> On 28/07/2020 19:49, Daniel Watford wrote:
> > Hello,
> >
> > OFBIZ-11900 covers the work-in-progress effort to refactor
> > MacroFormRenderer by extracting code which builds strings to be executed
> as
> > FTL macros into a separate builder class.
> >
> > This github branch [1] contains the work-in-progress changes for this
> > issue. Please take a look at this branch and highlight any concerns with
> > the approach. This will give me a chance to address concerns or change
> > approach before too much further work is done.
> >
> > Further to simply extracting code from MacroFormRenderer to a separate
> > builder I wanted to avoid the use of strings to represent FTL macro
> calls.
> > Instead I added an interface called RenderableFtl to represent an element
> > that can be rendered as part of a FreeMarker template.
> >
> > So far the main RenderableFtls in use are RenderableFtlMacroCall and
> > RenderableFtlString.
> >
> > RenderableFtlString can be used to represent a pre-rendered HTML string
> as
> > used to construct hidden forms by WidgetWorker.
> >
> > RenderableFtlMacroCall is probably the most important of the
> RenderableFtl
> > classes as it can be used in place of the strings previously built in
> > MacroFormRenderer to capture an FTL macro call, but without the need for
> > the caller to append quoting and whitespace delimiters for each
> additional
> > macro call parameter.
> >
> > All the RenderableFtl classes know how to render themselves to a String
> > which will be used by FtlWriter to process the string as an FTL element
> and
> > then write the result to an Appendable.
> >
> > The reason for these changes are to separate the various concerns
> involved
> > in rendering a form:
> > - RenderableFtl objects are responsible for rendering their data to an
> FTL
> > compliant string.
> > - FtlWriter is responsible for processing FTL compliant strings as part
> of
> > an FTL template and writing the results to an Appendable.
> > - RenderableFtlFormElementsBuilder is responsible for creating
> > RenderableFtl objects for various FieldInfo objects, replacing the FTL
> > macro call strings which were previously built in MacroFormRenderer.
> > - MacroFormRenderer is responsible for orchestrating calls to
> > RenderableFtlFormElementsBuilder to create multiple RenderableFtl objects
> > as needed for rendering a field and passing them to FtlWriter. For
> example,
> > rendering a DisplayEntityField produces RenderableFtl objects
> representing
> > the displayed field, an associated hyperlink and a tooltip.
> >
> > Separating these concerns makes testing easier:
> > - MacroFormRendererTest can test orchestration of MacroFormRender's calls
> > to RenderableFtlFormElementsBuilder, rather than having to analyse the
> > strings previously written.
> > - Testing of RenderableFtlFormElementsBuilder focuses on the production
> of
> > correctly configured RenderableFtl elements based on the given FieldInfo
> > objects, rather than testing the eventual string rendered.
> > - The RenderableFtl objects can be tested by examining the FTL strings
> > produced.
> >
> > To support testing of RenderableFtlFormElementsBuilder a number of
> Hamcrest
> > Matchers have been created to check that RenderableFtlMacroCall objects
> > have expected name and parameter values.
> >
> > Please excuse the long message :) and please let me know if you have any
> > comments or concerns about the approach I have taken.
> >
> > Thanks,
> >
> > Dan.
> >
> > [1] - https://github.com/danwatford/ofbiz-framework/tree/OFBIZ-11900-WIP
> >
>

--
Daniel Watford

[Nicolas Malin-2]

Hi Daniel,

Thanks for your check. I agree with the different function to create a
link would be analyzed and maybe refactored with another logic.

To move ahead, I will push my current code for the OFBIZ-11810 [1]

as it and convert to the your refactoring effort after.

I like to ensure to not introduce some regression, I sweat a lot for
this first version, I prefer to make small step by small step :)

Nicolas

[1] https://issues.apache.org/jira/browse/OFBIZ-11810

On 10/02/2021 12:32, Daniel Watford wrote:

> Hi Nicolas,
>
> OFBIZ-11900 [1] has been merged now, so there shouldn't be any sort of
> clash with OFBIZ-11810.
>
> OFBIZ-11900 is part of the OFBIZ-11456 [2] refactoring effort, but none of
> the other child tasks from OFBIZ-11456 are currently being worked on.
> Therefore I think you should be able to proceed with your changes without
> concern of impacting any changes that might be in flight.
>
> I had a quick read of the patch in OFBIZ-11810 and cannot see anything that
> needs to change with regard to the MacroScreenRenderer  refactoring effort.
> I did double check MacroCommonRenderer#createAjaxParamsFromUpdateAreas
> again RenderableFtlFormElementsBuilder#createAjaxParamsFromUpdateAreas, but
> I believe MCR#cAPFUA is an unaltered extraction of
> MacroScreenRenderer#createAjaxParamsFromUpdateAreas so shouldn't be any
> problem there.
>
> I don't fully understand how update areas work yet, so I would be grateful
> if you could have a look at the public methods in
> RenderableFtlFormElementsBuilder to see if any of the RenderableFtl
> produced there for form elements need to be altered to support your area
> update changes. In particular methods makeHyperlinkString,
> makeHyperlinkByType and hyperlinkMacroCall should probably be checked.
>
> Thank you for asking about this. It will renew my efforts to understand
> update areas and continue with the refactor once your changes have been
> applied to trunk.
>
> Thanks,
>
> Dan.
>
> [1] https://issues.apache.org/jira/browse/OFBIZ-11900
>
> [2] https://issues.apache.org/jira/browse/OFBIZ-11456
>
>
>
> On Tue, 9 Feb 2021 at 16:01, Nicolas Malin <[hidden email]> wrote:
>
>> Hello Daniel,
>>
>> I'm a little confuse to take more than 6 month to response.
>>
>> You push me on my currently java knowledge limit, and after analyze I
>> didn't understand all you done.
>>
>> However, your works feel good.
>>
>> During my works on the OFBIZ-11810 [1], I confronted to many duplicated
>> code between Screen, Form and Menu render. This case help me to
>> understand an advantage of your refacto.
>>
>> I'm clearly in favor to continue the refactoring, and I have a question.
>>
>> Because we works on the same code what do you prefer ?
>>
>>  * I commit my work with the current code and we migrate it after
>>
>>  * We migrate before the code and I update my work on it
>>
>>  * I try to convert my current improvement with your refactoring proposal.
>>
>> Nicolas
>>
>> [1] https://issues.apache.org/jira/browse/OFBIZ-11810
>>
>> On 28/07/2020 19:49, Daniel Watford wrote:
>>> Hello,
>>>
>>> OFBIZ-11900 covers the work-in-progress effort to refactor
>>> MacroFormRenderer by extracting code which builds strings to be executed
>> as
>>> FTL macros into a separate builder class.
>>>
>>> This github branch [1] contains the work-in-progress changes for this
>>> issue. Please take a look at this branch and highlight any concerns with
>>> the approach. This will give me a chance to address concerns or change
>>> approach before too much further work is done.
>>>
>>> Further to simply extracting code from MacroFormRenderer to a separate
>>> builder I wanted to avoid the use of strings to represent FTL macro
>> calls.
>>> Instead I added an interface called RenderableFtl to represent an element
>>> that can be rendered as part of a FreeMarker template.
>>>
>>> So far the main RenderableFtls in use are RenderableFtlMacroCall and
>>> RenderableFtlString.
>>>
>>> RenderableFtlString can be used to represent a pre-rendered HTML string
>> as
>>> used to construct hidden forms by WidgetWorker.
>>>
>>> RenderableFtlMacroCall is probably the most important of the
>> RenderableFtl
>>> classes as it can be used in place of the strings previously built in
>>> MacroFormRenderer to capture an FTL macro call, but without the need for
>>> the caller to append quoting and whitespace delimiters for each
>> additional
>>> macro call parameter.
>>>
>>> All the RenderableFtl classes know how to render themselves to a String
>>> which will be used by FtlWriter to process the string as an FTL element
>> and
>>> then write the result to an Appendable.
>>>
>>> The reason for these changes are to separate the various concerns
>> involved
>>> in rendering a form:
>>> - RenderableFtl objects are responsible for rendering their data to an
>> FTL
>>> compliant string.
>>> - FtlWriter is responsible for processing FTL compliant strings as part
>> of
>>> an FTL template and writing the results to an Appendable.
>>> - RenderableFtlFormElementsBuilder is responsible for creating
>>> RenderableFtl objects for various FieldInfo objects, replacing the FTL
>>> macro call strings which were previously built in MacroFormRenderer.
>>> - MacroFormRenderer is responsible for orchestrating calls to
>>> RenderableFtlFormElementsBuilder to create multiple RenderableFtl objects
>>> as needed for rendering a field and passing them to FtlWriter. For
>> example,
>>> rendering a DisplayEntityField produces RenderableFtl objects
>> representing
>>> the displayed field, an associated hyperlink and a tooltip.
>>>
>>> Separating these concerns makes testing easier:
>>> - MacroFormRendererTest can test orchestration of MacroFormRender's calls
>>> to RenderableFtlFormElementsBuilder, rather than having to analyse the
>>> strings previously written.
>>> - Testing of RenderableFtlFormElementsBuilder focuses on the production
>> of
>>> correctly configured RenderableFtl elements based on the given FieldInfo
>>> objects, rather than testing the eventual string rendered.
>>> - The RenderableFtl objects can be tested by examining the FTL strings
>>> produced.
>>>
>>> To support testing of RenderableFtlFormElementsBuilder a number of
>> Hamcrest
>>> Matchers have been created to check that RenderableFtlMacroCall objects
>>> have expected name and parameter values.
>>>
>>> Please excuse the long message :) and please let me know if you have any
>>> comments or concerns about the approach I have taken.
>>>
>>> Thanks,
>>>
>>> Dan.
>>>
>>> [1] - https://github.com/danwatford/ofbiz-framework/tree/OFBIZ-11900-WIP
>>>
>