Logic of determineIfAdvancedRendering should be revised to delegate to default renderer on more situations

[99sono]

In my opinion,

The startegy used in the method: determineIfAdvancedRendering Should be revised in order to deletegate the default renderer if the algorithm determines that the default renderer is more appropriate to handle the resource request.

In our case for example, we have a situation where we know that the default renderer would normally succeed were it not for the fact that bean providing the stream content is a view access scoped bean. So in this case we want to promote the usage of the advanced rendering stragegy and we use the new tag to say we want advanced rendering. However, the EL used in our case is not so trivial. We check a bean attribute that is of type stream content has anything on it or not and if not we display a standard no image static resource.

Which means, for a single p:graphic image component we initally show a static resource, and later we show a StreamContent resource after an image gets uploaded and is previewed.

For situations like this, I think the strategy of the advanced renderer should be refactored. Please consider the changes in the commit: https://github.com/99sono/AdvancedGraphicImageRenderer/commit/26b26363cb0e5379dc4d72b8ec660f6dde99f3bb

     private boolean determineIfAdvancedRendering(GraphicImage image) {
-        boolean result = false;
+        // (1) Regardles of the user asking to use advanced rendering explicitly by using the ui element
+        // we simply do not override the default primefaces behavior if the Image Is not returning streamed conent
+        if (image.getValue() == null) {
+            // otherwise the renderer would break with null pointer exception on get image src
+            // the user might want the advanced algorithm to run but we know already it would be doomed to fail
+            return false;
+        }
+        boolean weAreDealingWithSaticResource = !(image.getValue() instanceof StreamedContent);
+        if (weAreDealingWithSaticResource) {
+            return false;
+        }

-        boolean isStreamedContent = image.getValue() instanceof StreamedContent;
+        // (2) we are dealing with an attribute value that is a streamed content
+        // if the user is forcing us explictely to to use the advanced graphic image renderer algorithm
+        // then true
         Boolean advancedMarker = (Boolean) image.getAttributes().get(AdvancedRendererHandler.ADVANCED_RENDERING);
-
-        if (isStreamedContent && advancedMarker == null) {
-            result = determineSpecificParents(image);
+        if (advancedMarker != null && advancedMarker) {
+            return true;
         }

-        if (!result && advancedMarker != null && advancedMarker) {
-            result = true;
-        }
+        // (3) Streamed content but the user is not forcing us to absolutely use the AdvancedGraphicImageRneder
+        // algorithm than use the "smart" strategy to determine weather this special algorithm is needed
+        // the hope is to use this specific algorithm as little as possible and let primefaces run the show on its own
+        // however there are some ValueExpression that we know already that p:graphicImage will fail to evaluate during
+        // ResourceHandler call
+        return determineSpecificParents(image);

-        return result;
     }

Kindest regards.

[rdebusscher]

There is already a tag to force the usage of the advanced rendering

        <p:graphicImage value="#{data.image}">
            <x:advancedRenderering value="true"/>
        </p:graphicImage>

where x is defined as namespace in your page

xmlns:x="http://www.rubus.be/primefaces"

[99sono]

Hi, thank you for the reply. Yes, of that I am well aware.

However, in our case the scenario is more complex than that. You've implemented an algorithm that tries to determine - under normal conditions and without the use of that tag - weather or not the advanced graphic image rendering logic should take over. From my understanding this is to support the original scenario of Parameter Value expressions that cannot be resolved during resource handling ok.

And then you've made it also that if this is not enough you have this explicit tag that immediately forces the Advanced Graphic Image to take over - which would be the case of expressions that cannot be resolved because view access scoped beans, for example.

All of this makes perfect sense.

However, there can be more complex scenarios, such as what I described above. A scenario where under certain conditions the Bean Expression would evaluate to a static image resource, for example, on other situations (e.g. after an image upload) would evaluated to a StreamedContent coming out of the view access scoped bean. Under such a situation, I have to say "use the Advanced Graphic Image" rendereing for when I am dealing with a Streamed Content coming out a view scoped bean. However, please do not use it, when i am dealing with the static image resource or a bean property coming out null.

That is why the method I posted above, uses a logic that essentially says the following: (1) Unless the Value coming out the expression being rendered is StreamedContent value - I have no business of running the advanced logic, I can just let the default PrimeFaces Implemetntaion take Over as the advanced image rendering algorithm is for complex cases dealing with streamed content only.<---- This is static image resource coming out of the value expression, or null, for example, so I just ignore your tag request.

(2) However it it is a StreamedContent value - I may have to run the advanced image algorithm. And I do this if (a) Your original logic for detecting that expression could not be evaluated returns tur,e or (b) if the user explicitely says use the grpahic image rendereing algorithm. If the user is saying to use the algorithm it is because he knows somehting about that expresison going to blow up.

In eidhter case - it is better to delegate to primefaces standard implemtentation if the value expression comes out NULL or a value that is not a StreamContent value.

And in our case It was needed the tuning. Ohterwise I would not have touched it.

Many thanks.

[rdebusscher]

If you have such specific requirements, then I suggest that you continue in your fork of the repository. It is too specific to put in this codebase.

It is always a good thing that you take over the control of small utility code like this in your project to guarantee long term stability.

[99sono]

Hi, I understand your point of view.

However, I believe that the proposed fix does not impact negatively your main algorithm. I do not see how it could, because under the presence of StreamedContent your main logic is still present in it.

I would prefer to let my fork die and depent on your branch. You work closely with primefaces and have much better knowledge on what they are doing. Ultimately, I would prefer that your own branch could die by one day having the support of such scenarios withing primefaces base code as well. Less maintenance head-ache.

:) That is why I am so keen on deferring to your branch, provided It works for us.

Please consider the changes, if you think they are not harmful take them in ;).

Kindest regards, Nuno.