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

Overlay HUD + Alert #454

Merged
merged 21 commits into from
Jul 3, 2023
Merged

Overlay HUD + Alert #454

merged 21 commits into from
Jul 3, 2023

Conversation

nikola-milicevic
Copy link
Contributor

@nikola-milicevic nikola-milicevic commented Apr 24, 2023

Issue: https://radixdlt.atlassian.net/browse/ABW-1151

The base for showing HUD and Alerts in single/centralised place.

  • Created a separate Overlay Window, with HUD and Alert components.
  • Created an OverlayWindowClient to be medium through which the Main Window and Overlay Window communicate.
  • Hooked errorQueue to show an error alert anytime there is an error.
  • Hooked pasteboardClient to show Copied HUD anytime something is copied.
RPReplay_Final1688027790.MP4

@nikola-milicevic nikola-milicevic self-assigned this Apr 24, 2023
Copy link
Contributor

@GhenadieVP GhenadieVP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quite neat, a good solution.
left some comments for clarification.

@nikola-milicevic nikola-milicevic force-pushed the feature/ABW-1288-actions-for-addresses branch from 290ec68 to 4fde957 Compare April 25, 2023 11:20
@nikola-milicevic nikola-milicevic force-pushed the feature/ABW-1389-notification-banner branch from 383a7b9 to 05688f5 Compare April 25, 2023 11:21
@nikola-milicevic nikola-milicevic force-pushed the feature/ABW-1288-actions-for-addresses branch from 4fde957 to dcd66b0 Compare April 25, 2023 13:24
@nikola-milicevic nikola-milicevic force-pushed the feature/ABW-1389-notification-banner branch from 05688f5 to 3e6ac1a Compare April 25, 2023 13:26
@nikola-milicevic nikola-milicevic force-pushed the feature/ABW-1288-actions-for-addresses branch from e8055fb to 6e87f8b Compare April 26, 2023 13:50
@nikola-milicevic nikola-milicevic force-pushed the feature/ABW-1389-notification-banner branch from 3e6ac1a to ffbafbc Compare April 26, 2023 13:54
@nikola-milicevic nikola-milicevic force-pushed the feature/ABW-1288-actions-for-addresses branch 2 times, most recently from 535caf5 to 5e12956 Compare April 26, 2023 14:14
@nikola-milicevic nikola-milicevic force-pushed the feature/ABW-1389-notification-banner branch from ffbafbc to 44b43da Compare April 26, 2023 14:14
@nikola-milicevic nikola-milicevic force-pushed the feature/ABW-1288-actions-for-addresses branch from 5e12956 to 5d50ea4 Compare April 26, 2023 14:30
@nikola-milicevic nikola-milicevic force-pushed the feature/ABW-1389-notification-banner branch from 44b43da to 8f60e53 Compare April 26, 2023 14:31
@kugel3
Copy link
Contributor

kugel3 commented Apr 28, 2023

This is very important and useful functionality, but I think it should be implemented differently. First of all I don't think it's desirable that the view controller is responsible for dismissing itself through animation completions. Architecturally it makes sense for the client to handle this, especially when we start considering a queue of notifications of different types, including errors.

This also removes any reason to keep this in UIKit. When the client is in charge, this can easily be done in SwiftUI. Starting out it can just be a transition, later, with a queue, we might need to look at TimeLineView.

@nikola-milicevic nikola-milicevic force-pushed the feature/ABW-1389-notification-banner branch from a80a68b to 1513860 Compare May 1, 2023 10:19
@nikola-milicevic nikola-milicevic force-pushed the feature/ABW-1288-actions-for-addresses branch 2 times, most recently from 55a0fea to 5fa8839 Compare May 1, 2023 10:35
@nikola-milicevic nikola-milicevic force-pushed the feature/ABW-1389-notification-banner branch from 1513860 to 865d938 Compare May 1, 2023 10:40
@nikola-milicevic nikola-milicevic added the DO NOT MERGE Merging is blocked or prohibited due to missing specs or other higher priority PRs label May 1, 2023
@nikola-milicevic nikola-milicevic force-pushed the feature/ABW-1389-notification-banner branch from 865d938 to 995189e Compare May 1, 2023 13:48
@nikola-milicevic
Copy link
Contributor Author

I don't think it's desirable that the view controller is responsible for dismissing itself through animation completions. Architecturally it makes sense for the client to handle this, especially when we start considering a queue of notifications of different types, including errors.

Agreed, and that's how it is implemented - view controller is not dismissing itself. If you take closer look at the code, BannerView knows nothing about animation, BannerViewController displays BannerView (in viewDidAppear) and dismisses it. After dismissal of BannerView is complete, BannerViewController notifies the parent - the client - that the dismissal animation is complete, and then it's the client's responsibility to take the next step.

Currently it simply removes the window used for presentation. I would expand this feature further so the client actually encapsulates an array of notifications, each notification contains presentation animation type, which is propagated one by one to the child, implemented in UIKit or SwiftUI, whatever gives better result overall. Each notification has unique ID, and upon animation completion relevant notification element is removed from the notification queue. When there's nothing left in the queue, the client removes the presentation window.

So we would have to think a bit what do we want to display, how do we want that to look, and how do we interact with it. This is out of scope of this PR, and more importantly it's not something that would make the current implementation irrelevant.

This also removes any reason to keep this in UIKit. When the client is in charge, this can easily be done in SwiftUI. Starting out it can just be a transition, later, with a queue, we might need to look at TimeLineView.

The client is already in charge, it doesn't have anything to do with UI framework.

@nikola-milicevic
Copy link
Contributor Author

Blocked by #450

@kugel3
Copy link
Contributor

kugel3 commented May 1, 2023

BannerViewController notifies the parent - the client - that the dismissal animation is complete,

This is what I'm referring to. I didn't mean that it physically removes itself.

@nikola-milicevic
Copy link
Contributor Author

BannerViewController notifies the parent - the client - that the dismissal animation is complete,

This is what I'm referring to. I didn't mean that it physically removes itself.

Dismissal of banner view (the child) is separate from dismissal of the parent (view controller) which is controlled by the client. It's pretty standard parent - child relationship.

How would you design this feature?

@kugel3
Copy link
Contributor

kugel3 commented May 2, 2023

Like I wrote above, I think it's fine to have a view model/reducer control this, especially when the time comes to present a queue of notifications, some of which may be errors, and they may have different priorities. Then it will no longer be reasonable to listen to animation completions, it has to be centrally controlled. But it sounds like your argument against SwiftUI is that you want to use animation completions.

@nikola-milicevic nikola-milicevic force-pushed the feature/ABW-1288-actions-for-addresses branch from 5c8a190 to 04fc27f Compare May 2, 2023 07:53

// MARK: - OverlayWindowClient
/// This client is the intermediary between Main Window and the Overlay Window.
public struct OverlayWindowClient: Sendable {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably need a better name than Overlay..., couldn't make something better.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe Foreground?

@@ -0,0 +1,13 @@
import UIKit
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#if canImport(UIKit) ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me double check if it actually is needed, was brought in from the old implementation.

),
then: { HUD.View(store: $0) }
)
.task { viewStore.send(.task) }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

await .... finish()?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This task is never meant to finish, the OverlayView is always shown, never ever dismissed.

@@ -0,0 +1,33 @@
import SwiftUI
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#if canImport(UIKit)

Copy link
Contributor

@maciek-rdx maciek-rdx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool stuff and a lot of good code 👌

Let me know what do you think about the suggestions that I raised in comments though 🙏

Copy link
Contributor

@maciek-rdx maciek-rdx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! I'm particularly happy aboyt that one 🎉

I left you a small comment, cause it seems that the unsafe subscript is still there.

Sources/Features/AppFeature/Overlay/Overlay+Reducer.swift Outdated Show resolved Hide resolved
@GhenadieVP GhenadieVP merged commit 15136d1 into main Jul 3, 2023
2 checks passed
@GhenadieVP GhenadieVP deleted the feature/ABW-1389-notification-banner branch July 3, 2023 09:34
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants