fix!: Add setter helpers to DownloadEvent and new method for inline#21438
fix!: Add setter helpers to DownloadEvent and new method for inline#21438
Conversation
| return fileName == null ? "" : fileName; | ||
| public void setFileName(String fileName) { | ||
| response.setHeader("Content-Disposition", | ||
| "attachment;filename=" + fileName); |
There was a problem hiding this comment.
I was thinking about adding overloaded methods to support inline vs attachment as well as encoding but skipped since it's just a convenient helper for people.
I would still suggest to change the filename to use "" around it so that spaces in the filename are correctly supported. See https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Content-Disposition for more details
It's up to you if you also wanna support filename* by default since Vaadin is a globally used framework
There was a problem hiding this comment.
Good comment! I've added the "" wrapping. filename* would make sense to do, but I'd prefer it in a separate PR (would need an internal deiscussion and a bit of insvestigation how to implement this).
There was a problem hiding this comment.
You might wanna create a test for "" - it's easy to miss (speaking from personal experience where I forgot about it and user complaint about bad file names)
# Conflicts: # flow-server/src/main/java/com/vaadin/flow/server/streams/FileDownloadHandler.java
| } catch (IOException ioe) { | ||
| // Set status before output is closed (see #8740) | ||
| downloadEvent.getResponse().setStatus( | ||
| response.setStatus( |
There was a problem hiding this comment.
Should be downloadEvent.setStatus(int) but can be in other PR just wouldn't change this here in that case.
There was a problem hiding this comment.
Let's add this helper separately.
|
|
||
| @Override | ||
| public void handleDownloadRequest(DownloadEvent downloadEvent) { | ||
| String resourceName = getUrlPostfix(); |
There was a problem hiding this comment.
resourceName should be taken from the DownloadResponse as it is dynamically determined when handler.appy(downloadEvent) is called.
| String resourceName = getUrlPostfix(); | ||
| downloadEvent.setContentType( | ||
| getContentType(resourceName, downloadEvent.getResponse())); | ||
| if (!isInline()) { | ||
| downloadEvent.setFileName(resourceName); | ||
| } |
There was a problem hiding this comment.
These should be set after the hasError check
# Conflicts: # flow-server/src/main/java/com/vaadin/flow/server/streams/DownloadHandler.java # flow-server/src/test/java/com/vaadin/flow/server/streams/AbstractDownloadHandlerTest.java
| response.setHeader("Content-Disposition", | ||
| "attachment; filename=\"" + fileName + "\""); |
There was a problem hiding this comment.
This should be
if (fileName.isEmpty()) {
response.setHeader("Content-Disposition", "attachment");
} else {
response.setHeader("Content-Disposition",
"attachment; filename=\"" + fileName + "\"");
}
mshabarov
changed the title
|
4 participants
Refactors the
DownloadEventto be a normal class.Removed the getters for file name, type and size and add setters instead that are basically helper methods for setting various attributes to a response.
Adds inline method so that you can call
DownloadHandler.forFile(new File("path/to/file")).inline()and the download will be inlined onto page.