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

Bug: Opening and closing modals quickly may lead to a crash #11804

Closed
ferrariofilippo opened this issue Mar 22, 2023 · 3 comments · Fixed by #11928
Closed

Bug: Opening and closing modals quickly may lead to a crash #11804

ferrariofilippo opened this issue Mar 22, 2023 · 3 comments · Fixed by #11928
Labels
bug Something isn't working 👀

Comments

@ferrariofilippo
Copy link
Contributor

Description

First, this is an extreme case but nonetheless it's a bug and it should be fixed.
If you keep on opening and closing a modal using shortcuts, Files will eventually crash.

We could fix this using RichCommands.
I suggest adding a property (maybe private bool isReady?) which tells whether the command is ready to be executed (it would be true only if the modal has been closed). We would read this property when evaluating IsExecutable. We should apply it to all the command which open modals in Files.

The crash probably happens because we try to open the modal while it's being closed

Steps To Reproduce

  1. Select a file
  2. Hit Delete key
  3. Instantly hit Esc
  4. Hit Delete again
  5. Repeat until it crashes

Requirements

  • Fix crash that occurs when opening and closing modals quickly

Files Version

Files Preview 2.4.54.0

Windows Version

10.0.22621.1413

Log file

debug.log

@ferrariofilippo ferrariofilippo added the bug Something isn't working 👀 label Mar 22, 2023
@yaira2
Copy link
Member

yaira2 commented Mar 29, 2023

I'm seeing a number of related crashes in AppCenter.

@ferrariofilippo
Copy link
Contributor Author

First of all, if we need a quick fix we can add a try-catch in the ShowAsync() of each modal.

In my opinion, to fix the bug we should do as above in the report (introduce an isReady property). This property would be set to false at the beginning of ExecuteAsync() and then to true at the end. I was thinking of creating a "Wrapper" function to reduce code duplication. What are your thoughts?

@yaira2
Copy link
Member

yaira2 commented Mar 31, 2023

Let's do this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working 👀
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants