Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DocumentViewer: take file name from StreamedContent #796

Closed
mauromol opened this issue May 15, 2020 · 22 comments
Closed

DocumentViewer: take file name from StreamedContent #796

mauromol opened this issue May 15, 2020 · 22 comments
Assignees
Milestone

Comments

@mauromol
Copy link

See #436 (comment)

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 melloware self-assigned this May 15, 2020
@melloware melloware added this to the 8.0.3 milestone May 15, 2020
@melloware
Copy link
Member

Fixed with commit: primefaces-extensions/primefaces-extensions@27e2dd0

@mauromol
Copy link
Author

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
Copy link
Member

melloware commented May 15, 2020

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
Copy link
Author

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
Copy link
Member

melloware commented May 15, 2020

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
Copy link
Author

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
Copy link
Member

Documentation and Taglib fixed: primefaces-extensions/primefaces-extensions@00a6f28

@jeromebridge
Copy link

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
Copy link
Member

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

@melloware
Copy link
Member

@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();

melloware added a commit to primefaces-extensions/primefaces-extensions that referenced this issue May 6, 2022
@jeromebridge
Copy link

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
Copy link
Member

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.

melloware added a commit to primefaces-extensions/primefaces-extensions that referenced this issue May 6, 2022
@jeromebridge
Copy link

@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
Copy link
Member

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
Copy link
Member

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
Copy link

@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
Copy link
Member

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

@jeromebridge
Copy link

jeromebridge commented May 6, 2022

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
Copy link
Member

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

@jeromebridge
Copy link

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
Copy link
Member

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

@jeromebridge
Copy link

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));

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants