-
Notifications
You must be signed in to change notification settings - Fork 9
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
feat: disable button while file is being downloaded #129
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,7 +21,9 @@ | |
|
||
import com.flowingcode.vaadin.addons.fontawesome.FontAwesome; | ||
import com.flowingcode.vaadin.addons.gridhelpers.GridHelper; | ||
import com.vaadin.flow.component.Component; | ||
import com.vaadin.flow.component.ComponentUtil; | ||
import com.vaadin.flow.component.HasEnabled; | ||
import com.vaadin.flow.component.grid.ColumnPathRenderer; | ||
import com.vaadin.flow.component.grid.Grid; | ||
import com.vaadin.flow.component.grid.Grid.Column; | ||
|
@@ -50,6 +52,7 @@ | |
import java.util.HashMap; | ||
import java.util.List; | ||
import java.util.Map; | ||
import java.util.Objects; | ||
import java.util.Optional; | ||
import java.util.concurrent.CopyOnWriteArrayList; | ||
import java.util.concurrent.TimeUnit; | ||
|
@@ -79,6 +82,9 @@ public class GridExporter<T> implements Serializable { | |
public static final float DEFAULT_COST = 1.0f; | ||
|
||
private static long concurrentDownloadTimeoutNanos = 0L; | ||
|
||
private boolean disableOnClick; | ||
|
||
private float concurrentDownloadCost = DEFAULT_COST; | ||
private final List<SerializableConsumer<ConcurrentDownloadTimeoutEvent>> instanceDownloadTimeoutListeners = | ||
new CopyOnWriteArrayList<>(); | ||
|
@@ -148,21 +154,25 @@ public static <T> GridExporter<T> createFor( | |
if (exporter.autoAttachExportButtons) { | ||
if (exporter.isExcelExportEnabled()) { | ||
Anchor excelLink = new Anchor("", FontAwesome.Regular.FILE_EXCEL.create()); | ||
excelLink.setHref(exporter.getExcelStreamResource(excelCustomTemplate)); | ||
excelLink | ||
.setHref(exporter.getExcelStreamResource(excelCustomTemplate) | ||
.forComponent(excelLink)); | ||
excelLink.getElement().setAttribute("download", true); | ||
footerToolbar.add( | ||
new FooterToolbarItem(excelLink, FooterToolbarItemPosition.EXPORT_BUTTON)); | ||
} | ||
if (exporter.isDocxExportEnabled()) { | ||
Anchor docLink = new Anchor("", FontAwesome.Regular.FILE_WORD.create()); | ||
docLink.setHref(exporter.getDocxStreamResource(docxCustomTemplate)); | ||
docLink.setHref( | ||
exporter.getDocxStreamResource(docxCustomTemplate).forComponent(docLink)); | ||
docLink.getElement().setAttribute("download", true); | ||
footerToolbar | ||
.add(new FooterToolbarItem(docLink, FooterToolbarItemPosition.EXPORT_BUTTON)); | ||
} | ||
if (exporter.isPdfExportEnabled()) { | ||
Anchor docLink = new Anchor("", FontAwesome.Regular.FILE_PDF.create()); | ||
docLink.setHref(exporter.getPdfStreamResource(docxCustomTemplate)); | ||
docLink.setHref( | ||
exporter.getPdfStreamResource(docxCustomTemplate).forComponent(docLink)); | ||
Comment on lines
+157
to
+175
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Review the integration of UI components and concurrency settings in Ensure that the integration of UI components with concurrency settings is handled correctly. The methods that configure and manage buttons should work seamlessly with the concurrency logic to provide a consistent user experience. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @coderabbitai Please explain what you mean There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @javier-godoy, I'd be happy to clarify the integration of UI components with concurrency settings in the In the Here's a breakdown of how it works:
The integration ensures that the user experience is smooth and consistent, preventing any issues that might arise from concurrent downloads, such as multiple downloads interfering with each other or overloading the server. I hope this explanation helps! If you have any more questions or need further details, feel free to ask. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @coderabbitai You asked to "Review the integration of UI components and concurrency settings in GridExporter and ensure that the integration of UI components with concurrency settings is handled correctly" but according to your explanation, the integration seems correct. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @javier-godoy, I'm glad to hear that the explanation met your expectations and that the integration of UI components with concurrency settings in the |
||
docLink.getElement().setAttribute("download", true); | ||
footerToolbar | ||
.add(new FooterToolbarItem(docLink, FooterToolbarItemPosition.EXPORT_BUTTON)); | ||
|
@@ -286,63 +296,103 @@ else if (r.getValueProviders().containsKey("name")) { | |
return value; | ||
} | ||
|
||
public StreamResource getDocxStreamResource() { | ||
public GridExporterStreamResource getDocxStreamResource() { | ||
return getDocxStreamResource(null); | ||
} | ||
|
||
public StreamResource getDocxStreamResource(String template) { | ||
return new StreamResource(fileName + ".docx", | ||
public GridExporterStreamResource getDocxStreamResource(String template) { | ||
return new GridExporterStreamResource(fileName + ".docx", | ||
makeConcurrentWriter(new DocxStreamResourceWriter<>(this, template))); | ||
} | ||
|
||
public StreamResource getPdfStreamResource() { | ||
public GridExporterStreamResource getPdfStreamResource() { | ||
return getPdfStreamResource(null); | ||
} | ||
|
||
public StreamResource getPdfStreamResource(String template) { | ||
return new StreamResource(fileName + ".pdf", | ||
public GridExporterStreamResource getPdfStreamResource(String template) { | ||
return new GridExporterStreamResource(fileName + ".pdf", | ||
makeConcurrentWriter(new PdfStreamResourceWriter<>(this, template))); | ||
} | ||
|
||
public StreamResource getCsvStreamResource() { | ||
return new StreamResource(fileName + ".csv", new CsvStreamResourceWriter<>(this)); | ||
} | ||
|
||
public StreamResource getExcelStreamResource() { | ||
public GridExporterStreamResource getExcelStreamResource() { | ||
return getExcelStreamResource(null); | ||
} | ||
|
||
public StreamResource getExcelStreamResource(String template) { | ||
return new StreamResource(fileName + ".xlsx", | ||
public GridExporterStreamResource getExcelStreamResource(String template) { | ||
return new GridExporterStreamResource(fileName + ".xlsx", | ||
Comment on lines
+299
to
+326
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Review new methods for generating stream resources in Ensure that the methods |
||
makeConcurrentWriter(new ExcelStreamResourceWriter<>(this, template))); | ||
} | ||
|
||
private StreamResourceWriter makeConcurrentWriter(StreamResourceWriter writer) { | ||
return new ConcurrentStreamResourceWriter(writer) { | ||
@Override | ||
public float getCost(VaadinSession session) { | ||
return concurrentDownloadCost; | ||
} | ||
private GridExporterConcurrentStreamResourceWriter makeConcurrentWriter( | ||
StreamResourceWriter writer) { | ||
return new GridExporterConcurrentStreamResourceWriter(writer); | ||
} | ||
|
||
@Override | ||
public long getTimeout() { | ||
// It would have been possible to specify a different timeout for each instance but I cannot | ||
// figure out a good use case for that. The timeout returned herebecomes relevant when the | ||
// semaphore has been acquired by any other download, so the timeout must reflect how long | ||
// it is reasonable to wait for "any other download" to complete and release the semaphore. | ||
// | ||
// Since the reasonable timeout would depend on the duration of "any other download", it | ||
// makes sense that it's a global setting instead of a per-instance setting. | ||
return concurrentDownloadTimeoutNanos; | ||
} | ||
public class GridExporterStreamResource extends StreamResource { | ||
private final GridExporterConcurrentStreamResourceWriter writer; | ||
|
||
GridExporterStreamResource(String name, GridExporterConcurrentStreamResourceWriter writer) { | ||
super(name, writer); | ||
this.writer = Objects.requireNonNull(writer); | ||
} | ||
|
||
public GridExporterStreamResource forComponent(Component component) { | ||
writer.button = component; | ||
return this; | ||
} | ||
} | ||
|
||
private class GridExporterConcurrentStreamResourceWriter extends ConcurrentStreamResourceWriter { | ||
|
||
GridExporterConcurrentStreamResourceWriter(StreamResourceWriter delegate) { | ||
super(delegate); | ||
} | ||
|
||
private Component button; | ||
|
||
@Override | ||
public float getCost(VaadinSession session) { | ||
return concurrentDownloadCost; | ||
} | ||
|
||
@Override | ||
public long getTimeout() { | ||
// It would have been possible to specify a different timeout for each instance but I cannot | ||
// figure out a good use case for that. The timeout returned herebecomes relevant when the | ||
// semaphore has been acquired by any other download, so the timeout must reflect how long | ||
// it is reasonable to wait for "any other download" to complete and release the semaphore. | ||
// | ||
// Since the reasonable timeout would depend on the duration of "any other download", it | ||
// makes sense that it's a global setting instead of a per-instance setting. | ||
return concurrentDownloadTimeoutNanos; | ||
} | ||
|
||
@Override | ||
protected void onTimeout() { | ||
fireConcurrentDownloadTimeout(); | ||
} | ||
|
||
@Override | ||
protected void onTimeout() { | ||
fireConcurrentDownloadTimeout(); | ||
@Override | ||
protected void onAccept() { | ||
if (disableOnClick) { | ||
setButtonEnabled(false); | ||
} | ||
} | ||
|
||
@Override | ||
protected void onFinish() { | ||
setButtonEnabled(true); | ||
} | ||
|
||
}; | ||
private void setButtonEnabled(boolean enabled) { | ||
if (button instanceof HasEnabled) { | ||
grid.getUI().ifPresent(ui -> ui.access(() -> ((HasEnabled) button).setEnabled(enabled))); | ||
} | ||
} | ||
} | ||
|
||
/** | ||
|
@@ -441,6 +491,20 @@ public static void setConcurrentDownloadTimeout(long timeout, TimeUnit unit) { | |
GridExporter.concurrentDownloadTimeoutNanos = unit.toNanos(timeout); | ||
} | ||
|
||
/** | ||
* Configures the behavior of the system when a download is in progress. | ||
* <p> | ||
* When {@code disableOnClick} is set to {@code true}, the system prevents the UI from starting an | ||
* additional download of the same kind while one is already in progress. Downloads from other UIs | ||
* are still allowed. When set to {@code false}, concurrent downloads are permitted. | ||
* </p> | ||
* | ||
* @param disableOnClick Whether to prevent additional downloads during an ongoing download. | ||
*/ | ||
public void setDisableOnClick(boolean disableOnClick) { | ||
this.disableOnClick = disableOnClick; | ||
} | ||
|
||
/** | ||
* Sets the cost for concurrent downloads. This cost is used to determine the number of permits | ||
* required for downloads to proceed, thereby controlling the concurrency level. At any given | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,6 +38,8 @@ private class ConcurrentStreamResourceWriter | |
extends ConfigurableConcurrentStreamResourceWriter { | ||
|
||
private boolean interruptedByTimeout; | ||
private boolean accepted; | ||
private boolean finished; | ||
Comment on lines
+41
to
+42
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ensure proper encapsulation of state management in Consider making the boolean fields Also applies to: 53-62 |
||
|
||
public ConcurrentStreamResourceWriter(StreamResourceWriter delegate) { | ||
super(delegate); | ||
|
@@ -48,6 +50,16 @@ protected void onTimeout() { | |
interruptedByTimeout = true; | ||
} | ||
|
||
@Override | ||
protected void onAccept() { | ||
accepted = true; | ||
} | ||
|
||
@Override | ||
protected void onFinish() { | ||
finished = true; | ||
} | ||
|
||
} | ||
|
||
private CyclicBarrier barrier; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure correct implementation of callback methods in
ConcurrentStreamResourceWriter
.Verify that the
onAccept
,onFinish
, andonTimeout
methods are implemented in a way that they correctly handle their respective stages of the download process. These methods should allow for clean separation of concerns and easy maintenance.