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

Add examples for Notifications from async operations #222

Merged
merged 9 commits into from
Feb 18, 2024

Conversation

h0lg
Copy link
Contributor

@h0lg h0lg commented Feb 11, 2024

Provides reproducable examples for the exceptions mentioned in #221 on the NotificationPage in the Gallery.

It also includes some questions regarding the recent changes towards the concept of CmdMsg, which I don't understand, seemingly replacing Cmd<'msg>, which I think I understand by now.

@h0lg h0lg marked this pull request as draft February 11, 2024 19:33
@edgarfgp
Copy link
Member

Thanks for this PR. I will go through it during the week and make some notes where we can add more docs or samples.

samples/Gallery/Pages/ClipboardPage.fs Outdated Show resolved Hide resolved
@@ -50,6 +81,8 @@ module NotificationsPage =
match msg with
| ShowManagedNotification ->

//TODO this changes global state! i.e. after receiving this message, all notifications appear bottom right. is that intended?
Copy link
Member

Choose a reason for hiding this comment

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

Bear in mind that all of this is a port for the Avalonia Catalog samples, so I just wanted to test in the control works for us. have not really put too much though on making this side-effect free per se. Just adding some samples

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. This was just something where the behaviour surprised me. Not so much after seeing the assignment NotificationManager = FabApplication.Current.WindowNotificationManager in the source - but it still raised the question of how I'd customize the position of just one message without doing the same for all following messages. Would I have to remember the original Position and reset it after model.NotificationManager.Show?

Copy link
Member

Choose a reason for hiding this comment

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

So to control the position of a single Notification you can use:

let controlNotificationsRef = ViewRef<WindowNotificationManager>()

let update msg model =
    match msg with
    | ControlNotificationsShow ->
            // You can put in a Cmd If you want
            controlNotificationsRef.Value.Show(Notification("Control Notifications", "This notification is shown by the control itself."))
            model, []

// We can use the WindowNotificationManager a widget to be able to have a different WindowNotificationManager than FabApplication.Current.WindowNotificationManager
 // Allowing you control ie the Position of a single notification
WindowNotificationManager(controlNotificationsRef)
     .position(model.NotificationPosition)
     .dock(Dock.Top)
     .maxItems(3)

@h0lg
Copy link
Contributor Author

h0lg commented Feb 12, 2024

@edgarfgp Thank you for taking the time to respond :) Your suggestion worked for one of the added scenarios examples. I have converted your feedback into hopefully helpful comments and updated my questions for you to have a look at when you get to it.

| ShowAsyncStatusNotifications -> model, [NotifyAsyncStatusUpdates]

| NotifyInfo message ->
model.NotificationManager.Show(Notification(message, "", NotificationType.Information))
Copy link
Member

Choose a reason for hiding this comment

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

Just had a second look at this. We can use the Dispatcher.UIThread.Post here

Dispatcher.UIThread.Post(fun _ -> 
    model.NotificationManager.Show(Notification(message, "", NotificationType.Information)
)

and remove it from the the notifyAsyncStatusUpdates and notifyOneAsync and will fix cover both use cases

Edit: It is not recommended to access UI-related stuff from either init or update for that exact same reason
The correct approach is to push back the access to those UI stuff in Cmd, and then make sure those Cmd run on the UI thread if required. See my last commit

Copy link
Member

Choose a reason for hiding this comment

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

Ok so finally had some time to check this sample and made some improvements based on your question in this PR

@@ -50,6 +81,8 @@ module NotificationsPage =
match msg with
| ShowManagedNotification ->

//TODO this changes global state! i.e. after receiving this message, all notifications appear bottom right. is that intended?
Copy link
Member

Choose a reason for hiding this comment

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

So to control the position of a single Notification you can use:

let controlNotificationsRef = ViewRef<WindowNotificationManager>()

let update msg model =
    match msg with
    | ControlNotificationsShow ->
            // You can put in a Cmd If you want
            controlNotificationsRef.Value.Show(Notification("Control Notifications", "This notification is shown by the control itself."))
            model, []

// We can use the WindowNotificationManager a widget to be able to have a different WindowNotificationManager than FabApplication.Current.WindowNotificationManager
 // Allowing you control ie the Position of a single notification
WindowNotificationManager(controlNotificationsRef)
     .position(model.NotificationPosition)
     .dock(Dock.Top)
     .maxItems(3)

@edgarfgp edgarfgp marked this pull request as ready for review February 18, 2024 17:44
Copy link
Member

@edgarfgp edgarfgp left a comment

Choose a reason for hiding this comment

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

Thanks for this PR. Improved the sample and took some notes to write some docs to clarify the CmdMsg and the new Program functions.

@edgarfgp edgarfgp merged commit 8694564 into fabulous-dev:main Feb 18, 2024
1 check passed
@h0lg h0lg deleted the async-notify branch February 27, 2024 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants