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 <nicolas.ma...@nereide.fr> 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 >>> >
