State management breaks on quick opening/closing of the popup

vamsii777 commented 8 months ago

Describe the Bug:

When interacting with the LNPopupUI in my SwiftUI application, specifically when the audio is playing and I either drag down the popup or click the close button on the popup, the tab view and popup view turn black. The audio continues to play as expected, but the visual elements of the popup and tab view do not render correctly.

To Reproduce:

Steps to reproduce the behavior:

Start audio playback in the app using LNPopupUI.
Interact with the popup by either dragging it down or clicking the close button.
Observe that the tab view and popup view turn black while the audio continues to play.

Expected Behavior:

The popup and tab view should maintain their proper visual appearance when interacted with, without turning black, while allowing the audio to continue playing.

Screenshots:

https://github.com/LeoNatan/LNPopupUI/assets/38012177/ae5ab829-4a38-4b9b-b96a-10d91f5fdec1

Additional Context:

The issue occurs in a SwiftUI application.
Using LNPopupUI for popup management.
The problem seems related to view rendering and state management in conjunction with LNPopupUI.
The issue is consistently reproducible under the described conditions.

Environment:

SwiftUI version: 5
LNPopupUI version: 1.7.0
iOS version: 17.1.2
Xcode version: 15.0.1 (15A507)
Device/Emulator type and iOS version: iPhone 11

LeoNatan commented 8 months ago

Hello, This looks like a user state management bug. Please try to reproduce the issue on a clean example project (or the included one with this repo) and attach here. Thanks

LeoNatan commented 8 months ago

Ping.

Thanks

vamsii777 commented 8 months ago

Yes, I will follow your instructions by replicating the issue on a clean sample project and will attach it.

vamsii777 commented 8 months ago

Hey @LeoNatan,

Attached is an example project LNPopupNetworkExample.zip demonstrating the issue where dragging down the popup while playing on network audio stream causes it to vanish unexpectedly.

Thank you for looking into this.

LeoNatan commented 8 months ago

Hello @vamsii777 I took a short look at the demo project. It's not a small one. While I haven't investigated the ins and outs of it, and I am afraid I won't be able to, I do find that my instinct was correct; it is a state management issue. For example, removing the isPopupOpen: $audioPlaybackViewModel.isPopupOpen portion in ContentView.swift is enough to remove the issue (but might break your logic). I did notice, in your code you set isPopupOpen in multiple places. What I suspect that might be happening is that the underlying framework, LNPopupController, is not dealing with multiple changes in state as gracefully as it should, and something breaks. However, this is not a priority for the framework, and debugging these issues are very difficult, especially with a complex project as your example one. Before SwiftUI support was introduced with LNPopupUI, the LNPopupFramework framework considered state changes while transitioning to be user error. This was no longer possible in SwiftUI due to the nature of the beast, and I tried making everything as cancellable and reversible as possible, but it's not an easy task, and there seem to be some issues left.

I suggest leaving the popup open/close to the user, and at most, closing the popup when your playback ends. But it seems to me, at least from a cursory look, that you set it to closed and then open multiple times in very short times, creating the issue. If you must, use the onOpen and onClose to track the state of popup, and queue your state changes according to those, but, again, I do not suggest doing that (especially not in something like SwiftUI+LNPopupUI).

I will keep this issue open, but not sure if/when I will be able to improve it.

LeoNatan commented 8 months ago

So, I've been looking at how SwiftUI solves UIKit transitions, because it seems UIKit has the same issues as LNPopupController. For example, attempting to present and dismiss a view controller right away causes a crash. Yet, in SwiftUI, the system finishes the presentation and then dismisses.

So that gave me an idea to implement a similar queueing system in LNPopupController (and thus in LNPopupUI).

For example, the following code:

- (void)_start
{
    __weak __typeof(self) weakSelf = self;

    [self _firstContentController];
    [self _presentBar];
    [self _dismissBar];
    [self _presentBar];
    [self _dismissBar];
    [self _presentBar];
    [self _openPopup];
    [self _secondContentController];
    [self _presentBar];
    [self _dismissBar];
    [self _presentBarAndOpen];
    [self _closePopup];
    [self _openPopup];
    [self _dismissBarCompletionHandler:^{
        [weakSelf _afterSecond:^{
            [weakSelf _start];
        }];
    }];
}

creates the following behavior:

https://github.com/LeoNatan/LNPopupUI/assets/2270433/8f8d20b5-8853-4880-9bab-72a48bbf32e8

That's not to say that this solves your issue, because if your code remains as it is, your users will see a lot of transitions they shouldn't, but I am happy with this result as a stop gap between nothing (current implementation) and a complete interruptibility (which I also tried, but so far haven't had a satisfactory result).

vamsii777 commented 8 months ago

Acknowledged the recent developments and insights on the state management issue with LNPopupController. The proposed queueing system, inspired by SwiftUI's handling of UIKit transitions, seems like a strategic solution to address the rapid state changes and their impact on rendering. This approach could provide a more stable and consistent user experience, effectively resolving the visual anomalies observed during popup interactions. Eager to see how this implementation enhances the framework's robustness in upcoming releases.

LeoNatan commented 8 months ago

I hope to release a new version soon. Want to test locally to try and catch any regressions that might have been introduced. Nothing so far.

LeoNatan commented 8 months ago

Hello, I released LNPopupController 2.18.0 yesterday, which includes what we discussed before. Please update your dependencies, and try it out. Thanks

LeoNatan commented 8 months ago

Hello, when investigating #31 (which was a similar issue to this), I added an improvement on top of the previous release, which now makes things much better. Please try it out.

LeoNatan / LNPopupUI