-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Application crash when fast clicking the folders inside the file dialog #4260
Comments
Thanks for this |
This change adds a RWMutex to synchronize access to the fileDialog.data file list. The refreshDir function clears and rebuilds fileDialog.data, meanwhile, several other functions are indexing into it. This allowed for scenarios where fileDialog.data was either nil or smaller than expected when indexing into it. Fixes #4260
I still got a crash in the file dialog in my app while I was doing some legit file searching
|
@andydotxyz
This is on the develop branch
|
EDIT: Never mind, we are not using data binding in the file dialog. Sorry for the incorrect comment. |
You can ignore my previous message. It was wrong |
For historical record and to aid debugging it is better if we can get the SHA for the most recent commit - as the develop branch moves. |
hello @andydotxyz , it's super hard to replicate but here is another successful attempt.
|
It looks like this is a bit different than the original issue based on the trace. Maybe something to do with binding in fileItemRenderer label. |
I have been able to reproduce this with the same error output by going into my /tmp directory and scrolling up and down very quickly. |
I think I have fixed it. @alexballas would you mind trying out my change to see if it fixes the issue you are seeing? https://github.com/JordanGoulder/fyne/tree/fix_fileitem_crash |
Hello @JordanGoulder, thanks for that. This indeed seems to have helped. However, are we confident that with this fix we're not just hiding the issue? How come an extra refresh somewhere is responsible for a panic? |
I think you generally shouldn't refresh objects within the layout function. As far as I see it, the text is refreshed within the Refresh() method (from |
Refreshing objects, although not recommended in some cases where methods like SetText() handle it, should still not cause panics. If there is an architectural and documented process where explicit Refresh calls can cause panics, then fine. But I still think we're hiding the real issue here. |
I might be wrong but as far as I know, the layout function is called within a lock and the refresh tries to use the same lock sometimes causing a panic. It is not necessary refresh that causes a panic per say. Also, the layout function should just layout objects not update their contents. If there is missing documentation around this we should update it. |
Checklist
Describe the bug
When I fast click the folders inside the file selection widget, the app crashes
How to reproduce
Screenshots
2023-09-17_20-37-18.mp4
Example code
No code. Replicated with Fyne Demo
Fyne version
v2.4.0
Go compiler version
1.21.1
Operating system and version
Ubuntu 18.04
Additional Information
No response
The text was updated successfully, but these errors were encountered: