primefaces-extensions / primefaces-extensions.github.com

Organization repo, only for homepage, wiki and issue tracker
https://primefaces-extensions.github.io/
70 stars 22 forks source link

DocumentViewer: take file name from StreamedContent #796

Closed mauromol closed 4 years ago

mauromol commented 4 years ago

See https://github.com/primefaces-extensions/primefaces-extensions.github.com/issues/436#issuecomment-629162285

When using the pe:documentViewer with a StreamedContent instance, you have to use the download attribute to specify the file name. This is completely unexpected: StreamedContent has org.primefaces.model.StreamedContent.getName() whose purpose is exactly to provide the file name of the streamed content. This is how p:fileDownload works, for instance.

Perhaps the download attribute may be used to override StreamedContent.getName(), but as a first choice I would expect the StreamedContent instance to be enough to completely describe the document.

melloware commented 4 years ago

Fixed with commit: https://github.com/primefaces-extensions/core/commit/27e2dd0725390478aac933ed7d6095b1b05ded6d

mauromol commented 4 years ago

I just had a quick look at your commit, but isn't it giving precedence to StreamedContent.getName() over the download attribute value? If so, I think the download attribute value becomes completely useless. I would use the download attribute value if non-null and non-empty, otherwise the StreamedContent.getName().

Or else, remove the download attribute as a whole, but you'll break backward compatibility.

Just my 2 cents.

melloware commented 4 years ago

I looked at DynamicContentSrcBuilder and it does this...

else if (value instanceof String) {
            src = ResourceUtils.getResourceURL(context, (String) value);
        }
        else if (value instanceof StreamedContent) {
            StreamedContent streamedContent = (StreamedContent) value;
            else {
                byte[] bytes = toByteArray(streamedContent.getStream());
                String base64 = DatatypeConverter.printBase64Binary(bytes);
                return "data:" + streamedContent.getContentType() + ";base64," + base64;

So it looks like build can take a String url, a StreamedContent, or a Base64 byteArray. I didn't want to make that assumption that is why i originally made it a download param. So this way ONLY if your type is a StreamedContent will I use the name.

mauromol commented 4 years ago

Ok, I trust you, I just saw the following code in your commit:

final Object value = documentViewer.getValue();
String downloadName = documentViewer.getDownload();
if (value instanceof StreamedContent) {
  final StreamedContent streamedContent = (StreamedContent) value;
  downloadName = streamedContent.getName();
}
return DynamicContentSrcBuilder.build(context, value, documentViewer,
  documentViewer.isCache(), DynamicContentType.STREAMED_CONTENT, true) + "&download="
  + downloadName;

This code seems to suggest streamedContent.getName() is used whenever value instanceof StreamedContent, disregarding documentViewer.getDownload() completely. For me it's fine, it's just breaking backward compatibility. Also, documentation says that download attribute is: "If streamed content this will be the name of the download file". But if you're not using it any more for StreamedContent, probably the documentation should also be updated.

melloware commented 4 years ago

right but your point was if you are providing a StreamContent you should use the file name of that streamed Content no?

Not have to declare a download name when the StreamContent already has it. So now it will only use the download property if its NOT a streamed content

mauromol commented 4 years ago

right but your point was if you are providing a StreamContent you should use the file name of that streamed Content no?

My point was: the StreamedContent already carries a name, why am I forced to supply one with the download attribute? I should not be forced to do that. #436 says this.

Not have to declare a download name when the StreamContent already has it. So now it will only use the download property if its NOT a streamed content

OK for me, but then the documentation should be fixed, because it says exactly the opposite.

melloware commented 4 years ago

Documentation and Taglib fixed: https://github.com/primefaces-extensions/core/commit/00a6f28e51b5d788394ddada479ab30180c8873b

jeromebridge commented 2 years ago

I think this change may have broken the link sent to download the PDF file. In the past the download attribute was set to the value resolved by the download attribute. Now it always seems to be null

Example:

http://localost:8080/yweb/javax.faces.resource/dynamiccontent.properties.xhtml?ln=primefaces&v=10.0.11&e=10.0.7&pfdrid=4acc3a9116a66977173de6cdc08b185b&pfdrt=sc&pfdrid_c=false&uid=4409f9f5-52ca-48c7-ad3f-56719b5b46cb&download=null
melloware commented 2 years ago

Can you post your streamed content code? I feel like you are missing the name in your StreamedContent?

melloware commented 2 years ago

@jeromebridge I just tested this and my download was named "jack.pdf".

 content = DefaultStreamedContent.builder().stream(() -> new ByteArrayInputStream(out.toByteArray()))
                        .contentType("application/pdf").name("jack.pdf").build();
jeromebridge commented 2 years ago

This is the code that creates the stream:

final DefaultStreamedContent content = DefaultStreamedContent.builder()
            .contentType( "application/pdf" )
            .name( "report.pdf" )
            .stream( () -> new ByteArrayInputStream( generated.pdf().bytes() ) ).build();

This is my XHTML component:

<pe:documentViewer height="500" value="#{experimentPdfPage.content}" download="report.pdf" zoom="page-width" />

I don't think the Document Viewer can get its content from a "View Scoped" bean. So I put the content into the session and call the experimentPdfPage bean which is a session bean. I'll include that code below. It checks for the "Download Id" to identify the document to return.

FacesContext context = FacesContext.getCurrentInstance();
final String id = context.getExternalContext().getRequestParameterMap().get( "download" );
final StreamedContent result = JSFUtils.session().<StreamedContent>get( id ).orElseGet( DefaultStreamedContent::new );
melloware commented 2 years ago

Perfectly working example: pfe-796.zip

Just unzip and run mvn clean jetty:run and navigate to http://localhost:8080/primefaces-test/

When you click Download you will see it is jack.pdf.

jeromebridge commented 2 years ago

@melloware Thank you for the example.

I see that you are using a "Request Scope" bean to generate the PDF. I'm using a "View Scoped" bean to generate the PDF. I don't think I can use the "Request Scope" bean to generate the PDF as you did.

Do you have a way that I can use a "View Scope" bean to generate the PDF and get it to the Document Viewer component?

melloware commented 2 years ago

I don't understand why you can't use RequestScoped? Can I ask why are you storing your document in the ViewScoped bloating the session?

melloware commented 2 years ago

Here is the answer from the master himself BalusC on why you can't use ViewScoped with Dynamic Streaming Content: https://stackoverflow.com/a/18994746/502366

You have to use RequestScoped or SessionScoped.

jeromebridge commented 2 years ago

@melloware I don't really want to put it in the session. That was the workaround I came up to provide a session scoped property to the component.

I would like to (if possible) use the view scope to generate the PDF and make it accessible to the component somehow.

melloware commented 2 years ago

Its not possible with Streaming if you read BalusC's stack overflow on why ViewScoped won't work.

jeromebridge commented 2 years ago

Just looking at the link you gave me. It appears they did something like this:

<p:media value="#{mediaManager.stream}" width="100%" height="700px" player="pdf">
    <f:param name="id" value="#{documentsBean.mediaId}" />
</p:media>

I tried to do something similar to see if I could get that identifier out of the request context:

<pe:documentViewer height="500" value="#{experimentPdfPage.content}" zoom="page-width">
                    <f:param name="id2" value="#{experimentPage14.downloadId}" />
                </pe:documentViewer>

I didn't see the parameter in the request context of the backing bean though.

Just as a side note: That example is very similar to how I had it working in previous versions of primefaces / pf-extensions.

melloware commented 2 years ago

Notice his bean is ApplicationScoped. Trust me ViewScope is NOT possible.

jeromebridge commented 2 years ago

I agree (I know View Scoped doesn't work)

I put my content parameter on a SessionScoped. (I believe that works). Do you think ApplicationScoped vs SessionScoped would affect the parameters passed to the experimentPdfPage.content call?

melloware commented 2 years ago

Not sure I am not even sure if what you are trying to do an pass the is even possible with DocumentViewer.

jeromebridge commented 2 years ago

That's how BalusC get's the idenifier of the media to display in the content.

https://stackoverflow.com/questions/18994288/primefaces-pmedia-not-working-with-streamedcontent-in-a-viewscoped-bean/18994746#18994746

From his example:

String id = context.getExternalContext().getRequestParameterMap().get("id");
            Media media = service.find(Long.valueOf(id));