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] 3.0.0 crash in sheet View #147

Closed
zjinhu opened this issue Oct 17, 2024 · 14 comments · Fixed by #160
Closed

[BUG] 3.0.0 crash in sheet View #147

zjinhu opened this issue Oct 17, 2024 · 14 comments · Fixed by #160
Assignees
Labels

Comments

@zjinhu
Copy link

zjinhu commented Oct 17, 2024

No description provided.

@FulcrumOne
Copy link
Contributor

Hey,

Could you send me the details about the crash (what exactly crashed and how to reproduce it)

Thanks,
Tomasz

@FulcrumOne
Copy link
Contributor

Hey @zjinhu,

Sorry for interrupting, but any further description would be extremely helpful - I tried to debug your problem, but unfortunately no crash appeared.

@zjinhu
Copy link
Author

zjinhu commented Oct 21, 2024

crash

@Carlegk
Copy link

Carlegk commented Oct 21, 2024

image Hey, When a sheet is open, the app crashes. However, on pages without a sheet, there are no issues.

@FulcrumOne
Copy link
Contributor

thanks. I have a suspicion - I know I made a mistake and all the calculations are done in the same thread (which I plan to fix early next month); it is possible that in this case the function calculating the height of the popup overloads the thread by looping. And this is my guess (however I didn't observe this behavior when developing the update).
To confirm this, I would have to ask you to produce a runable code that I can use to observe exactly the same behavior.

Thank you again for your report and have a good day,
Tomasz

@Carlegk
Copy link

Carlegk commented Oct 22, 2024

Hey,
This is the error demonstration after I simplified my code. I may not have written it correctly. Thank you.

import SwiftUI
import MijickPopups

struct ContentView: View {
    @StateObject private var viewModel: ViewModel = ViewModel()
    
    var body: some View {
        VStack {
            Image(systemName: "globe")
                .imageScale(.large)
                .foregroundStyle(.tint)
            Text("Hello, world!")
        }.onAppear{
            viewModel.testError()
        }
    }
}

extension ContentView{
    class ViewModel: ObservableObject{
        @Published var errorManager: ErrorManager = ErrorManager()
        
        func testError(){
            // no crash
            // ErrorPopupView().present()
            
            // no crash
            // errorManager.triggerError()
            
            // crash
            Task {
                errorManager.triggerError()
            }
        }
    }
}

class ErrorManager: ObservableObject {
    func triggerError() {
        ErrorPopupView().present()
    }
}

struct ErrorPopupView: CentrePopup {
    var body: some View {
        createContent()
    }
    
    func createContent() -> some View {
        Button(action: {
            dismissAllPopups()
        }){
            Text("dismiss")
        }
    }
}

@FulcrumOne
Copy link
Contributor

Thanks @Carlegk,

At first glance, it seems that this is indeed a problem with the main thread; I'll investigate this more thoroughly over the weekend. But if my suspicions prove true, a fix for this bug is on my schedule for early November (super sorry for that, but I'm busy with other things right now)

@FulcrumOne FulcrumOne changed the title 3.0.0 crash in sheet View [BUG] 3.0.0 crash in sheet View Oct 28, 2024
@FulcrumOne FulcrumOne self-assigned this Nov 3, 2024
@FulcrumOne FulcrumOne moved this to Backlog in Popups Roadmap Nov 3, 2024
@FulcrumOne
Copy link
Contributor

Hey! Here's a quick update about the problem:

As I suspected, there is a problem with threads; since Xcode 16, the View protocol has gained an @MainActor attribute by default, therefore all methods declared in the View are @MainActor.

Popup conforms to View, which means that all its methods (including .present()) are @MainActor, and should therefore be called from the main thread, hence this crash.

The solution is kinda easy - just make sure that these methods are called from the main thread, a.k.a. use DispatchQueue.main.async methods, BUT I would rather advise you to wait a few days and I will provide a more complex solution to this problem (so eventually you won't have to change anything in your code).

Thanks for your patience,
Tomasz

@Kejiasir
Copy link

Kejiasir commented Nov 5, 2024

@FulcrumOne Hello, I have the same problem. After I updated to version 3.0.2, the app crashed immediately when the pop-up view appeared.
20241105142404

@FulcrumOne
Copy link
Contributor

@FulcrumOne Hello, I have the same problem. After I updated to version 3.0.2, the app crashed immediately when the pop-up view appeared.

20241105142404

Hey, see the previous post in which I explained everything 😉

@fayharinn
Copy link

fayharinn commented Nov 6, 2024

Hi, I'm using DispatchQueue.main.async to call MyView.present(), there is still a crash
Thread 1: Fatal error: Unexpectedly found nil while implicitly unwrapping an Optional value
updatePopupAction(popup.settingHeight(newHeight))

To reproduce :

  • Call a sheet
  • make it disappear by tapping a button (do not swipe to make it disappear)
  • tape on a button that uses .present()
    => crash

@bukira
Copy link

bukira commented Nov 7, 2024

I have the same crash but iOS 15 and 16 only, working fine in iOS 17, and 18

@FulcrumOne
Copy link
Contributor

Hi,

I'm currently working on this and it should be ready within a few days. I'll keep you in the loop.

Have a nice day,
Tomasz

@FulcrumOne
Copy link
Contributor

Hey!

A brief update on the situation - I have completed most of the work on the upcoming update (4.0.0) and you can test its beta version by switching to the branch patch-4.0.0 in your projects.

To solve the problem, I had to make sure that the present() and dismiss() methods are called from the main thread - to do this I unfortunately had to mark them as async, which results in you having to present the popup with code like:

Task { 
    await YourPopup().present()
}

I am very sorry about this, but alas, it was the only solution. I confirmed it with the code delivered by @Carlegk and it works (btw. I am very grateful for this example as it has shortened my work considerably).

Let me know if it resolves your problems and have a nice day,
Tomasz

PS. I plan to publish this version by the end of this week, preferably on Saturday.

@FulcrumOne FulcrumOne mentioned this issue Nov 17, 2024
@FulcrumOne FulcrumOne linked a pull request Nov 17, 2024 that will close this issue
FulcrumOne added a commit that referenced this issue Nov 17, 2024
perf:
- Improved library performance due to better task distribution between threads (#153)

fix:
- Fixed an issue with calling the library from non-main threads (#147)
- Fixed an issue that made it impossible to interact with the app when the overlay was set to clear (#150)
- Fixed a problem with the stacked popups background color
- Improved vertical popup animations

refactor:
- Renamed and changed some public API properties (see 3.x.x -> 4.0.0 Migration Guide for more information)
- Refactored several smaller files
@github-project-automation github-project-automation bot moved this from In review to Done in Popups Roadmap Nov 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

6 participants