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

Allow passing local file path to ui.download #1118

Merged
merged 7 commits into from
Aug 5, 2023

Conversation

rodja
Copy link
Member

@rodja rodja commented Jul 2, 2023

This PR enhances the ui.download element to also work with local file paths (like ui.image etc)

@rodja rodja added the enhancement New feature or request label Jul 2, 2023
@rodja rodja added this to the 1.3.0 milestone Jul 2, 2023
@rodja rodja requested a review from falkoschindler July 2, 2023 06:40
Copy link
Contributor

@falkoschindler falkoschindler left a comment

Choose a reason for hiding this comment

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

I reviewed the PR:

  • For consistency I chose to use helpers.is_file for checking if src is a local file.
  • I moved the definition of DOWNLOAD_DIR out of test_helpers.py, because this file isn't meant to be a place for "test helpers", but rather a file testing the helpers.py file.
  • I don't approve the PR, yet, because there is a potential memory leak: For every download there is a new route added to the FastAPI app. Now there are two cases:
    1. There are different file, e.g. captured images. Every download of the current image creates a new route which is never cleaned up, even if the image is eventually removed from memory.
    2. There is some button like a "Download the datasheet of our great product" triggering the download of a local PDF. Each client clicking the button will create another route with the same path. For a well visited website this might eventually be a significant leak.
      So I wonder if there's an alternative to avoid accumulating routes.

@rodja
Copy link
Member Author

rodja commented Jul 6, 2023

You are right. ui.download is not a normal element. We need to think a bit more about how we approach this.

@falkoschindler falkoschindler marked this pull request as draft July 6, 2023 12:40
@falkoschindler falkoschindler modified the milestones: 1.3.0, Later Jul 7, 2023
@rodja
Copy link
Member Author

rodja commented Aug 3, 2023

Maybe we can create a unique route if the download is executed and remove it after the file has been served. Kind of a one-time-download-route. We do something similar in https://github.com/zauberzeug/nicegui/blob/main/examples/download_text_as_file/main.py.

@falkoschindler
Copy link
Contributor

falkoschindler commented Aug 3, 2023

@rodja I implemented such single-use download routes. Have a look. 🙂

Test code:

ui.button('Download', on_click=lambda: ui.download('website/static/logo.png'))

l = ui.label()
ui.timer(1, lambda: l.set_text(len(app.routes)))

@falkoschindler falkoschindler marked this pull request as ready for review August 3, 2023 17:53
@falkoschindler falkoschindler modified the milestones: Later, 1.3.8 Aug 3, 2023
@rodja
Copy link
Member Author

rodja commented Aug 4, 2023

Super cool :-)

@falkoschindler falkoschindler merged commit 6ffe77f into main Aug 5, 2023
5 checks passed
@falkoschindler falkoschindler deleted the download_local_file branch August 5, 2023 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants