FilePickerControl features like new event and fixes upload behavior between page update #3875
Conversation
…and fixed the files control across session (page refresh on web)
Reviewer's Guide by SourceryThis pull request implements improvements to the FilePickerControl component, addressing issues with file upload behavior during page refreshes and introducing a new event for completed uploads. The changes primarily affect the file_picker.dart and file_picker.py files, modifying the internal file management structure and adding new functionality for upload status feedback. File-Level Changes
Tips
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @andersou - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider refactoring the
buildmethod into smaller, more focused methods to improve readability and maintainability. - The global
filesvariable could lead to issues in concurrent scenarios. Consider keeping this as part of the component's state instead.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| @@ -78,12 +76,12 @@ class FilePickerControl extends StatefulWidget { | |||
| State<FilePickerControl> createState() => _FilePickerControlState(); | |||
| } | |||
|
|
|||
| List<PlatformFile> files = []; | |||
There was a problem hiding this comment.
suggestion: Consider moving the 'files' list into the state class
Global variables can lead to unexpected behavior and make the code harder to maintain. It would be better to keep this list as part of the _FilePickerControlState class.
| List<PlatformFile> files = []; | |
| class _FilePickerControlState extends State<FilePickerControl> { | |
| List<PlatformFile> files = []; | |
| // Rest of the class implementation... | |
| } |
There was a problem hiding this comment.
In the state class, the files variable becomes unstable during page refreshes.
There was a problem hiding this comment.
It sounds like the instability of the files variable during page refreshes might be due to how the state is being managed or preserved across refreshes. One approach to address this could be to ensure that the state is properly initialized or restored when the component is rebuilt.
You might want to consider using a state management solution or a mechanism to persist the state across refreshes, such as using a StatefulWidget with a State that properly handles initialization and disposal. Additionally, you could explore using local storage or a similar method to save and restore the files list if it needs to persist across page reloads.
If you can provide more details about how the state is currently being managed, I might be able to offer more specific suggestions.
| @@ -114,6 +115,7 @@ def __init__( | |||
| self, | |||
| on_result: Optional[Callable[[FilePickerResultEvent], None]] = None, | |||
| on_upload: Optional[Callable[[FilePickerUploadEvent], None]] = None, | |||
| on_upload_finished: Optional[Callable[[ControlEvent], None]] = None, | |||
There was a problem hiding this comment.
issue (complexity): Consider whether the new state and event handler are essential.
The recent changes have increased the complexity of the FilePicker class. The addition of a new state (UPLOADING_FILES) and event handler (on_upload_finished) adds more logic to manage, which can make the code harder to maintain. The on_upload_finished handler seems redundant as a simple pass-through, potentially adding unnecessary complexity. Consider whether these additions are essential. If they are, aim to integrate them with minimal redundancy and straightforward logic. I've refactored the code to ensure on_upload_finished is only added if provided, and encapsulated event data conversion for better readability. This approach reduces complexity and improves maintainability.
|
Thanks for your contribution. |
|
I'm wondering how files selected with FilePicker stay alive between page refreshes? Is it the standard behavior of modern browsers? I mean if I do a simple page with file input control, choose a file and then refresh the page would it work? |
|
Hi Feodor, if you try to upload a file, it works for the first time. If you refresh the Page, the file uploader doesnt work correctly. Tomorrow i will make a example to reproduce this behavior. |
|
Can anyone test file_picker on google tv streamer? It is no longer working at all on that platform. I cannot figure out if this is a google issue or a recent update to android APIs. Watching adb logcat reveals nothing interesting, although I can post those logs. Ironically it is not just my app, I see exact same logcat messages for wireguard app not working selecting config files to create a tunnel. |
Description
The FilePicker component exhibits strange behaviors when the webpage is refreshed. The internal "files" structure, which is responsible for managing previously selected files, becomes null during the component's update process. This issue affects the file upload functionality, causing it to fail.
Additionally, after refreshing the webpage, the upload process does not execute as expected, leading to further problems with file uploads.
New Feature Added: A new functionality has been introduced: an event for completed uploads. This event should trigger correctly once the upload process is finalized, providing feedback on the status of the upload.
Feel free to adjust the details according to your specific requirements!
Test Code
# Test code for the review of this PRType of change
Checklist:
I signed the CLA.
My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
My changes generate no new warnings
New and existing tests pass locally with my changes
I have made corresponding changes to the documentation (if applicable)
Summary by Sourcery
Enhance the FilePicker component by adding a new event for completed uploads and fixing the file upload behavior during page refresh. Refactor the file upload logic to improve the component's reliability.
New Features:
Bug Fixes:
Enhancements: