Skip to content

Fix Avalonia notification in 2 steps only without strong coupling - #85

Open
maxensas wants to merge 4 commits into
Platonenkov:devfrom
maxensas:dev
Open

Fix Avalonia notification in 2 steps only without strong coupling#85
maxensas wants to merge 4 commits into
Platonenkov:devfrom
maxensas:dev

Conversation

@maxensas

@maxensas maxensas commented May 26, 2026

Copy link
Copy Markdown

Following up on issue #82, I tried using Avalonia notifications, but it didn't work.
Rather than opening a new issue to explain what goes wrong, I found solutions to several issues I encountered.

Step 1 and 2 for use case.

  1. Setup DI service
sc.AddAvaloniaNotifications(cfg =>
{
    cfg.SuccessBackgroundColor = NotificationColor.FromHex("#FF252525");
    cfg.ErrorBackgroundColor = NotificationColor.FromHex("#FF252525");
    cfg.SuccessIconColor = NotificationColor.LimeGreen;
    cfg.ErrorIconColor = NotificationColor.OrangeRed;
    cfg.DefaultExpirationTime = TimeSpan.FromSeconds(5);
})
  1. Call Show method using INotificationService interface
    we shouldn't know anything about manager that add a framework dependency if called outside..
sp.GetRequiredService<INotificationService>()
   .Show(new() { Title = "title", Message = "message", Type = NotificationType.Success, ShowCloseButton = false });
  1. What comes with the PR ?
  • Fix default position without parent window associated.
  • Fix DI / service use (no need to instantiate a manager before first show call)
  • UseOverlayWindow as default like WPF imp (debatable, can be in the notif request aswell)
  • Automatically set the main window as host if instanciated.

This also fix my implementation of your package in my Xiletrade project :
https://github.com/maxensas/xiletrade/blob/dev/src/Xiletrade.UI.Avalonia/Program.cs

  1. Possible area for improvement : refactor Dispatcher.UIThread calls and make async Tasks.
if (Dispatcher.UIThread.CheckAccess())
{
    InitOverlay();
}
else
{
    Dispatcher.UIThread.Post(InitOverlay);
}

This allow _overlayWindow to be instancied when calling Show method (it works because already in UI thread but would be better to await using Task)

…rst show call) + UseOverlayWindow as default like WPF imp (debatable) + automatically set the main window as host if instanciated + Fix default position without parent window.
@Platonenkov

Copy link
Copy Markdown
Owner

Hi @maxensas, thanks for the PR — the direction is right (decouple consumers from AvaloniaNotificationManager and let them work through INotificationService only), and the ApplyPositionOnScreen fix is a nice catch. A few things to address before we can merge:

1. Side effects in the manager constructor

public AvaloniaNotificationManager(INotificationConfiguration config, INotificationEventService events)
{
    ...
    var appLifetime = (IClassicDesktopStyleApplicationLifetime)Application.Current!.ApplicationLifetime!;
    var win = appLifetime?.MainWindow;
    if (win is not null)
    {
        SetHost(win);
    }
    UseOverlayWindow = true;
}

Problems:

  • Hard cast to IClassicDesktopStyleApplicationLifetime will throw InvalidCastException on ISingleViewApplicationLifetime (mobile/browser Avalonia targets). The service becomes unresolvable in DI for those.
  • Application.Current!.ApplicationLifetime! — double null-forgiving will NRE if DI is built before StartWithClassicDesktopLifetime(...).
  • Setting UseOverlayWindow = true via the setter inside the constructor triggers EnsureOverlayWindow() → showing a Window from the DI service constructor. That's an anti-pattern; we shouldn't open windows while resolving services.
  • In your own sample DI is built inside MainWindow's constructor, so appLifetime.MainWindow is still null at that point (Avalonia assigns it after the ctor returns). The if (win is not null) SetHost(win) branch is effectively dead code in the typical flow — it only "works" because of the ApplyPositionOnScreen fix that lets the overlay position itself without a parent.

Please move the auto-host logic out of the constructor. Either:

  • lazy-initialize on the first Show / first access to ActiveHost (resolve Application.Current.ApplicationLifetime defensively with as + null checks, support both classic-desktop and single-view), or
  • expose it as an IHostedService / a small startup helper that hooks MainWindowOpened and then calls SetHost.

The UseOverlayWindow = true default is fine — keeping it consistent with the WPF side. Just don't set it through the property in the ctor; assign the backing field directly so no window gets shown during DI build.

2. Sample: drop the static service provider

private static IServiceProvider _serviceProvider;

Please make it an instance field, and cache the resolved services (INotificationService, AvaloniaNotificationManager) in instance fields after BuildServiceProvider() — same as before the PR. Resolving from the container on every click and using a static SP isn't necessary and breaks the moment a second window is introduced.

3. Style

var is fine in this repo, no need to change that. Please do keep the existing using ordering convention though (using System.*; first, then Avalonia.*, then project namespaces) — currently the touched files flip that order.

4. Stray TODO

// this pattern can be used to refactor for action/func and async Tasks

Please remove this comment (or open a follow-up issue if you want to track the refactor).

Once these are addressed I'm happy to merge. Thanks again!

@maxensas

Copy link
Copy Markdown
Author

All right, thanks for the fast review i will polish this when i catch up some time.

@Platonenkov

Copy link
Copy Markdown
Owner

Thanks for the quick turnaround @maxensas — the WIP commit covers most of it:

  • Constructor side-effects — nicely done. The lazy Initialize() + the platform-agnostic TopLevel property (handling both IClassicDesktopStyleApplicationLifetime and ISingleViewApplicationLifetime) is exactly what was needed, and factoring the dispatcher calls into DelegateActionToUiThread is a good bonus.
  • Static service provider — gone, and since the registration is TryAddSingleton, the computed Notification/Manager accessors resolve the same instance, so that's fine functionally.
  • TODO comment — removed.

using ordering isn't a blocker, don't worry about it.

One remaining thing to fix before merge:

Initialize() overwrites an explicit UseOverlayWindow choice

private void Initialize()
{
    if (_initialized) return;
    var topLevel = TopLevel;
    if (topLevel is not null) SetHost(topLevel);
    UseOverlayWindow = true;   // <-- here
    _initialized = true;
}

Because this assigns through the property, a consumer who deliberately sets manager.UseOverlayWindow = false (in-window mode) before the first Show() gets silently flipped back to true on that first call — the setter doesn't touch _initialized, so there's no guard. That's a behavior regression for the in-window use case.

Suggested fix: set the default on the backing field in the constructor instead, and drop the assignment from Initialize():

public AvaloniaNotificationManager(INotificationConfiguration config, INotificationEventService events)
{
    _config = config;
    _events = events;
    _useOverlayWindow = true;   // default matches WPF, no window shown during DI build
}

private void Initialize()
{
    if (_initialized) return;
    var topLevel = TopLevel;
    if (topLevel is not null) SetHost(topLevel);
    _initialized = true;
}

This keeps the true default you want while respecting an explicit override.

Minor (optional): Initialize() isn't thread-safe — concurrent first Show() calls from different threads could run it twice. Probably not worth a lock for typical UI usage, but flagging it.

Once the UseOverlayWindow overwrite is sorted I'm good to merge.

@maxensas

Copy link
Copy Markdown
Author

Hi @Platonenkov, regarding the initialization method, I knew I was making a mistake, hence the explicit WIP name of the commit. Better ideas after a good night's sleep :)

Apologies if I took some freedom with the refactoring regarding the dispatcher access. My PR now seems consistent with the prerequisites. Tell me if it doesnt.

For desktop apps, I usually use DI with MVVM, hence my following suggestion. It's generally best to set up services upstream after app launch and have a single provider per application, hence the static one.

Since this is a sample project, we can be flexible, but if you want to manage multiple views in a next iteration, you'll need to refactor accordingly.

Thanks for your time !

@Platonenkov

Copy link
Copy Markdown
Owner

Hi @maxensas — to make this concrete, I prepared the remaining fixes as a patch against the current PR head (b369dd6). Apply with git am < pr85-fixup.patch on your dev branch, or cherry-pick ba6b896 from the diff below — whatever fits your flow. Push back to this PR and we're done.

What the patch changes

src/Notification.Avalonia/AvaloniaNotificationManager.cs

  1. SetHost split. SetHost(TopLevel host, bool init = false) → public SetHost(TopLevel host) + private EnsureHostInitialized(). The public API no longer leaks the internal lazy-init flag.
  2. _initializedHost no longer latches on failure. Previously SetHost(null, init: true) set _initializedHost = true before the null-check return, so if TopLevel was null at first Show() (DI built before MainWindow was assigned), the manager went silently dead for the rest of the process. The new EnsureHostInitialized() only flips the flag after _host is successfully created; otherwise it retries on the next access.
  3. Race fix. EnsureHostInitialized() is guarded by a lock (_initLock) with double-check, closing the race where two concurrent first-Show() calls could create two AvaloniaNotificationHost instances and orphan the first one's cards.
  4. Position persistence. Setting manager.Position = X before the first Show() used to be silently lost (the setter touched _host/_overlayWindow.Host directly, both null, then lazy init created a host with the config default). The patch adds _pendingPosition and threads it through the getter, SetHost, and EnsureOverlayWindow.
  5. Dismiss / DismissAll no longer pop an empty overlay. With the new _useOverlayWindow = true default, calling service.DismissAll() defensively at startup went through ActiveHostEnsureOverlayWindow() → an empty topmost NotificationOverlayWindow flashed (and could show up in Alt+Tab). The patch adds a private ExistingHost accessor that returns the current host without triggering lazy init or overlay creation; Dismiss/DismissAll use it.
  6. BindToParent symmetry. BindToParent is already null-safe (early-returns on null), so it's now called unconditionally — matches the unconditional ApplyPositionOnScreen call you already did and avoids the case where overlay was positioned on its own screen but never bound to the parent.

src/Notification.Avalonia/Extensions/ActionExtensions.cs

  1. InvokeOnUiThreadAsync no longer swallows exceptions. Dispatcher.UIThread.Post(async () => await action()) drops the inner Task → any exception inside Func<Task> became an unobserved task exception, surfacing only on GC. The patch returns a real Task propagated via TaskCompletionSource: synchronous fast-path on the UI thread, otherwise Post with try/catch and SetException.

Verification

  • Solution builds clean (dotnet build Notification.Wpf.sln -c Debug): 0 errors, 0 new warnings.
  • Notification.Core.Tests: 95/95 pass.
  • Notification.Console.Tests: 16/16 pass.

(No Avalonia test project in the repo, so these fixes weren't unit-tested directly — happy to follow up with an Avalonia.Headless.XUnit project covering #4 and #5 if you want.)

Patch

From ba6b8964d91074321c522f2874803673fdd87e8a Mon Sep 17 00:00:00 2001
From: Aleksandr Platonenkov <platonenkov87@gmail.com>
Date: Mon, 8 Jun 2026 12:32:54 -0300
Subject: [PATCH] fix(avalonia): address review findings in
 AvaloniaNotificationManager and ActionExtensions
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* SetHost: split overloaded SetHost(TopLevel, bool init) into a clean public
  SetHost(TopLevel) and a private EnsureHostInitialized() — removes the public
  API magic flag and stops _initializedHost from latching when TopLevel is
  not yet available (used to silently disable the manager forever if first
  Show() ran before MainWindow was assigned).

* EnsureHostInitialized: guarded by a lock to close the race where two
  concurrent first Show() calls could create two AvaloniaNotificationHost
  instances and orphan the first one's cards.

* Position: setter now persists a pending value so configuring
  manager.Position before the first Show() is no longer silently lost when
  the lazy host is created later; SetHost and EnsureOverlayWindow honor it.

* Dismiss / DismissAll: routed through a new ExistingHost helper that does
  NOT trigger lazy init nor EnsureOverlayWindow — a defensive DismissAll()
  no longer pops an empty topmost overlay window.

* EnsureOverlayWindow: BindToParent is null-safe (early-returns on null),
  so it is now called unconditionally — matches the unconditional
  ApplyPositionOnScreen call and avoids the case where overlay was
  positioned but never bound when _parentWindow was null.

* InvokeOnUiThreadAsync: now returns a Task and propagates exceptions via
  TaskCompletionSource instead of the previous fire-and-forget
  Dispatcher.UIThread.Post(async () => await action()) which silently
  swallowed any exception thrown by the Func<Task> as an unobserved task.
---
 .../AvaloniaNotificationManager.cs            | 110 +++++++++++++-----
 .../Extensions/ActionExtensions.cs            |  35 +++++-
 2 files changed, 112 insertions(+), 33 deletions(-)

diff --git a/src/Notification.Avalonia/AvaloniaNotificationManager.cs b/src/Notification.Avalonia/AvaloniaNotificationManager.cs
index ccaa87c..7059b67 100644
--- a/src/Notification.Avalonia/AvaloniaNotificationManager.cs
+++ b/src/Notification.Avalonia/AvaloniaNotificationManager.cs
@@ -28,6 +28,8 @@ namespace Notification.Avalonia
         private NotificationOverlayWindow _overlayWindow;
         private bool _useOverlayWindow;
         private bool _initializedHost;
+        private NotificationPosition? _pendingPosition;
+        private readonly object _initLock = new object();
 
         /// <summary>
         /// Initializes a new instance of the <see cref="AvaloniaNotificationManager"/> class.
@@ -52,27 +54,53 @@ namespace Notification.Avalonia
 
         /// <summary>
         /// Attach the notification manager to a TopLevel (Window or similar).
-        /// Must be called before showing notifications.
+        /// Optional — called automatically on first show via auto-discovery
+        /// (<see cref="Application.Current"/>'s main window/view).
         /// </summary>
-        public void SetHost(TopLevel host, bool init = false)
+        public void SetHost(TopLevel host)
         {
-            _initializedHost = true;
-            if (init)
-            {
-                host = TopLevel;
-                if (host is null)
-                {
-                    return;
-                }
-            }
-            
+            if (host is null)
+                throw new ArgumentNullException(nameof(host));
+
             _parentWindow = host as Window;
 
+            NotificationPosition position =
+                _pendingPosition ?? _config?.MessagePosition ?? NotificationPosition.BottomRight;
+
             _host = new AvaloniaNotificationHost(host)
             {
                 MaxItems = (int)(_config?.MaxOverlayWindowCount ?? 5),
-                Position = _config?.MessagePosition ?? NotificationPosition.BottomRight
+                Position = position
             };
+
+            _initializedHost = true;
+        }
+
+        /// <summary>
+        /// Lazily discovers the main window/view via <see cref="Application.Current"/>
+        /// and attaches the manager to it. Idempotent and side-effect-free if the
+        /// platform top-level is not yet available — retries on next access.
+        /// </summary>
+        private void EnsureHostInitialized()
+        {
+            if (_initializedHost)
+                return;
+
+            lock (_initLock)
+            {
+                if (_initializedHost)
+                    return;
+
+                TopLevel topLevel = TopLevel;
+                if (topLevel is null)
+                {
+                    // Do not latch the flag — retry on next access once the
+                    // application top-level becomes available.
+                    return;
+                }
+
+                SetHost(topLevel);
+            }
         }
 
         /// <summary>
@@ -111,10 +139,7 @@ namespace Notification.Avalonia
         {
             get
             {
-                if (!_initializedHost)
-                {
-                    SetHost(null, init: true);
-                }
+                EnsureHostInitialized();
                 if (_useOverlayWindow)
                 {
                     EnsureOverlayWindow();
@@ -124,6 +149,21 @@ namespace Notification.Avalonia
             }
         }
 
+        /// <summary>
+        /// Returns the currently active host WITHOUT triggering lazy init or
+        /// creating an overlay window. Used by Dismiss/DismissAll so a no-op
+        /// dismiss cannot pop an empty overlay window.
+        /// </summary>
+        private AvaloniaNotificationHost ExistingHost
+        {
+            get
+            {
+                if (_useOverlayWindow)
+                    return _overlayWindow?.Host;
+                return _host;
+            }
+        }
+
         /// <summary>
         /// Gets the top Level Avalonia control, platform agnostic (desktop or mobile/browser).
         /// </summary>
@@ -150,12 +190,19 @@ namespace Notification.Avalonia
         {
             get
             {
-                AvaloniaNotificationHost host = ActiveHost;
-                return host?.Position ?? NotificationPosition.BottomRight;
+                if (_host != null)
+                    return _host.Position;
+                if (_overlayWindow?.Host != null)
+                    return _overlayWindow.Host.Position;
+                return _pendingPosition ?? _config?.MessagePosition ?? NotificationPosition.BottomRight;
             }
             set
             {
-                // Apply to both hosts so position is preserved when switching modes
+                // Remember the user's choice so it survives even if the position
+                // is set BEFORE the first Show() (i.e. before lazy host init runs).
+                _pendingPosition = value;
+
+                // Apply to both hosts so position is preserved when switching modes.
                 if (_host != null)
                     _host.Position = value;
                 if (_overlayWindow?.Host != null)
@@ -282,21 +329,24 @@ namespace Notification.Avalonia
         }
 
         /// <summary>
-        /// Dismiss a notification by ID.
+        /// Dismiss a notification by ID. No-op if no notifications have been shown
+        /// yet — does NOT trigger lazy host initialization or create an overlay window.
         /// </summary>
         public void Dismiss(Guid notificationId)
         {
-            ActiveHost?.Close(notificationId);
+            ExistingHost?.Close(notificationId);
             _events?.Raise(new NotificationLifecycleEventArgs(
                 notificationId, NotificationLifecycleStage.Dismissed, null, null));
         }
 
         /// <summary>
-        /// Dismiss all active notifications.
+        /// Dismiss all active notifications. No-op if no notifications have been
+        /// shown yet — does NOT trigger lazy host initialization or create an
+        /// overlay window.
         /// </summary>
         public void DismissAll()
         {
-            ActiveHost?.CloseAll();
+            ExistingHost?.CloseAll();
         }
 
         private void EnsureOverlayWindow()
@@ -309,7 +359,11 @@ namespace Notification.Avalonia
                 if (_overlayWindow != null)
                     return;
 
-                NotificationPosition position = _host?.Position ?? _config?.MessagePosition ?? NotificationPosition.BottomRight;
+                NotificationPosition position =
+                    _pendingPosition
+                    ?? _host?.Position
+                    ?? _config?.MessagePosition
+                    ?? NotificationPosition.BottomRight;
 
                 _overlayWindow = new NotificationOverlayWindow();
                 _overlayWindow.Host.MaxItems = (int)(_config?.MaxOverlayWindowCount ?? 5);
@@ -321,10 +375,8 @@ namespace Notification.Avalonia
                 };
 
                 _overlayWindow.ApplyPositionOnScreen(_parentWindow, position);
-                if (_parentWindow != null)
-                {
-                    _overlayWindow.BindToParent(_parentWindow);
-                }
+                // BindToParent is null-safe (early-returns on null) — call unconditionally.
+                _overlayWindow.BindToParent(_parentWindow);
 
                 _overlayWindow.Show();
             }).InvokeOnUiThread();
diff --git a/src/Notification.Avalonia/Extensions/ActionExtensions.cs b/src/Notification.Avalonia/Extensions/ActionExtensions.cs
index e7fd38a..f51a79f 100644
--- a/src/Notification.Avalonia/Extensions/ActionExtensions.cs
+++ b/src/Notification.Avalonia/Extensions/ActionExtensions.cs
@@ -28,16 +28,43 @@ public static class ActionExtensions
     }
 
     /// <summary>
-    /// Executes the asynchronous action on the UI thread.
+    /// Executes the asynchronous action on the UI thread and returns a Task
+    /// that completes (or faults) when the action finishes. Exceptions are
+    /// propagated to the caller via the returned Task instead of being
+    /// silently swallowed as unobserved task exceptions.
     /// </summary>
-    public static void InvokeOnUiThreadAsync(this Func<Task> action)
+    public static Task InvokeOnUiThreadAsync(this Func<Task> action)
     {
         if (action is null)
-            return;
+            return Task.CompletedTask;
+
+        if (Dispatcher.UIThread.CheckAccess())
+        {
+            try
+            {
+                return action() ?? Task.CompletedTask;
+            }
+            catch (Exception ex)
+            {
+                return Task.FromException(ex);
+            }
+        }
 
+        TaskCompletionSource<object> tcs = new TaskCompletionSource<object>();
         Dispatcher.UIThread.Post(async () =>
         {
-            await action();
+            try
+            {
+                Task inner = action();
+                if (inner != null)
+                    await inner.ConfigureAwait(false);
+                tcs.TrySetResult(null);
+            }
+            catch (Exception ex)
+            {
+                tcs.TrySetException(ex);
+            }
         });
+        return tcs.Task;
     }
 }
-- 
2.53.0.windows.3

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.

2 participants