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

feat: disable button while file is being downloaded #129

Merged
merged 1 commit into from
Sep 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,36 @@ public float getCost(VaadinSession session) {
*/
protected abstract void onTimeout();

/**
* Callback method that is invoked when a download is accepted.
* <p>
* This method is called at the start of the download process, right after the
* {@link #accept(OutputStream, VaadinSession) accept} method is invoked and it has been
* determined that the download can proceed. Subclasses should implement this method to perform
* any necessary actions before the download begins, such as initializing resources, logging, or
* updating the UI to reflect the start of the download.
* <p>
* Note that this method is called before any semaphore permits are acquired, so it is executed
* regardless of whether the semaphore is enabled or not.
* </p>
*/
protected abstract void onAccept();

/**
* Callback method that is invoked when a download finishes.
* <p>
* This method is called at the end of the download process, right before the
* {@link #accept(OutputStream, VaadinSession) accept} method returns, regardless of whether the
* download was successful, timed out, or encountered an error. Subclasses should implement this
* method to perform any necessary actions after the download completes, such as releasing
* resources, logging, or updating the UI to reflect the completion of the download.
* <p>
* Note that this method is always called, even if an exception is thrown during the download
* process, ensuring that any necessary cleanup can be performed.
* </p>
*/
protected abstract void onFinish();

Comment on lines +160 to +189
Copy link

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, and onTimeout 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.

/**
* Handles {@code stream} (writes data to it) using {@code session} as a context.
* <p>
Expand All @@ -175,33 +205,37 @@ public float getCost(VaadinSession session) {
*/
@Override
public final void accept(OutputStream stream, VaadinSession session) throws IOException {
onAccept();
try {
if (!enabled) {
delegate.accept(stream, session);
} else {

try {

int permits;
float cost = getCost(session);
synchronized (semaphore) {
permits = costToPermits(cost, semaphore.maxPermits);
}

if (!enabled) {
delegate.accept(stream, session);
} else {

try {

int permits;
float cost = getCost(session);
synchronized (semaphore) {
permits = costToPermits(cost, semaphore.maxPermits);
}

if (semaphore.tryAcquire(permits, getTimeout(), TimeUnit.NANOSECONDS)) {
try {
delegate.accept(stream, session);
} finally {
semaphore.release(permits);
if (semaphore.tryAcquire(permits, getTimeout(), TimeUnit.NANOSECONDS)) {
try {
delegate.accept(stream, session);
} finally {
semaphore.release(permits);
}
} else {
onTimeout();
throw new InterruptedByTimeoutException();
}
} else {
onTimeout();
throw new InterruptedByTimeoutException();
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
throw (IOException) new InterruptedIOException().initCause(e);
}
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
throw (IOException) new InterruptedIOException().initCause(e);
}
} finally {
onFinish();
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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<>();
Expand Down Expand Up @@ -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
Copy link

@coderabbitai coderabbitai bot Jun 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review the integration of UI components and concurrency settings in GridExporter.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai Please explain what you mean

Copy link

Choose a reason for hiding this comment

The 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 GridExporter class.

In the GridExporter class, the integration primarily revolves around managing the UI components, specifically the buttons for exporting documents, in conjunction with the concurrency settings that control the download processes.

Here's a breakdown of how it works:

  1. UI Component Integration: Each export button (for Excel, DOCX, PDF) is linked to a corresponding GridExporterStreamResource via the forComponent() method. This method associates a button with a specific download action, ensuring that the download is triggered when the button is clicked.

  2. Concurrency Management: The GridExporterStreamResource uses an instance of GridExporterConcurrentStreamResourceWriter to write the stream. This class extends ConcurrentStreamResourceWriter and is crucial for managing concurrency. It implements methods like onAccept() and onFinish():

    • onAccept(): Called when a download is initiated. This method disables the associated button to prevent further clicks and thus additional concurrent downloads.
    • onFinish(): Called once the download is complete. This re-enables the button, allowing further actions.
  3. Concurrency Settings: These settings are configured to manage how downloads are handled when multiple requests are made. For instance, settings like concurrentDownloadCost and concurrentDownloadTimeoutNanos help in defining the cost of a download and how long the system should wait before timing out a download request, respectively.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link

Choose a reason for hiding this comment

The 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 GridExporter class appears correct to you. If you have any more questions, need further clarifications, or if there's anything else I can assist you with, please don't hesitate to ask!

docLink.getElement().setAttribute("download", true);
footerToolbar
.add(new FooterToolbarItem(docLink, FooterToolbarItemPosition.EXPORT_BUTTON));
Expand Down Expand Up @@ -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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review new methods for generating stream resources in GridExporter.

Ensure that the methods getDocxStreamResource, getPdfStreamResource, and getExcelStreamResource are correctly implemented. They should integrate seamlessly with the rest of the application and handle all edge cases.

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

/**
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -115,5 +115,8 @@ public GridExporterBigDatasetDemo() throws EncryptedDocumentException, IOExcepti
exporter.setConcurrentDownloadCost(9);
// end-block

// Prevents additional downloads from starting while one is already in progress
exporter.setDisableOnClick(true);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ private class ConcurrentStreamResourceWriter
extends ConfigurableConcurrentStreamResourceWriter {

private boolean interruptedByTimeout;
private boolean accepted;
private boolean finished;
Comment on lines +41 to +42
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure proper encapsulation of state management in ConcurrentStreamResourceWriter.

Consider making the boolean fields private to ensure they are only modified within the class itself, enhancing encapsulation and reducing the risk of unintended side effects from external modifications.

Also applies to: 53-62


public ConcurrentStreamResourceWriter(StreamResourceWriter delegate) {
super(delegate);
Expand All @@ -48,6 +50,16 @@ protected void onTimeout() {
interruptedByTimeout = true;
}

@Override
protected void onAccept() {
accepted = true;
}

@Override
protected void onFinish() {
finished = true;
}

}

private CyclicBarrier barrier;
Expand Down
Loading