Split MessageOptions via interfaces

[Lakritzator]

This issue is just an advice, not a bug. It's indirectly related to #32

Library version

ToastNotifications 2.3.4

Expected behaviour

Set MessageOptions.NotificationClickAction and this will be called when I click a notification.

Actual behaviour

MessageOptions.NotificationClickAction is not used in ToastNotifications (but in ToastNotifications.Messages it is), so setting this doesn't do anything

Steps to reproduce behaviour

• Don't use ToastNotifications.Messages!
• Supply message options for a notification.
• Show the notification
• Click notification.

Code to reproduce behaviour

Nothing special, you probably already have this..

Questions

Hi,

I was looking into more details of your nice library, see how I can improve my usage of it.
So I noticed the MessageOptions having (among others) FreezeOnMouseEnter / UnreezeOnMouseEnter, which might be useful to specify.

Than I noticed that many properties on the MessageOptions are not used inside ToastNotifications but only in ToastNotifications.Messages, which makes the library harder to understand and use.

I would create an interface e.g. INotificationOptions, with the base properties, and implement this as NotificationOptions in ToastNotifications. Than in ToastNotifications.Messages you create a new interface IMessageOptions, which extends INotificationOptions with the additional properties you need in there. (The naming was just an proposal, change to your liking!)

Additionally you should consider to use more properties, MessageOptions is okay, but for instance I wanted to extend NotificationDisplayPart with a possibility to set the MessageOptions. But as there is only a GetOptions(), I needed to override this AND implement a way to set it:

    public abstract class ToastView : NotificationDisplayPart
    {
        /// <summary>
        /// Specify MessageOptions for your toast 
        /// </summary>
        public MessageOptions Options { get; set; }

        /// <inheritdoc />
        public override MessageOptions GetOptions() => Options;
    }

instead of just adding the "set"...

Not using properties also reduces the possibilities to bind and have INotifyPropertyChang(ing/ed) events. And using interfaces, almost everywhere, make your and my life a lot easier.

Just my 5 cents.

I'd be more than happy to make a PR, but I haven't finished my review 😄

Best wishes,
Robin

[rafallopatka]

Hi, thank you for suggestions, it is a good idea to refactor this in way you suggest.

[rafallopatka]

I've made some changes according to your suggestions, I tried to make it a bit easier to mantain and easier to extend by removing some duplications and unnecesery parts.

[Lakritzator]

Cool, I will check them out 👍

[Lakritzator]

I had a quick look, it's really great to see interfaces in your changes 👍

Still I am confused: in INotificationConfiguration (and BaseNotificationConfiguration) you have information (e.g. ShowCloseButton) which is only used by the package ToastNotifications.Messages. I advice you to place the information you only use in ToastNotifications.Messages also only in there.

In fact, the Message in the INotification is also for your ToastNotifications.Messages, although it's used here;

ToastNotifications/Src/ToastNotifications/Lifetime/TimeAndCountBasedLifetimeSupervisor.cs

Line 162 in 20fd855

.Where(x => x.Value.Notification.DisplayPart.GetMessage() == msg)

I have been setting the Message value, but I don't use ToastNotifications.Messages, so it's pretty much for nothing.

Another thing is when I look at the NotificationDisplayPart, I see the Notification (INotification) and Configuration (INotificationConfiguration), where the INotification has a INotificationConfiguration in it again.
So I could do:
NotificationDisplayPart.Configuration.ShowCloseButton = true
Or
NotificationDisplayPart.Notification.Configuration.ShowCloseButton = true
Which one is correct?

And worse, INotification has a NotificationDisplayPart in it..
So I could do this:
NotificationDisplayPart.Notification.DisplayPart.Notification.DisplayPart.Notification.DisplayPart.Notification.DisplayPart 😄

Actually, with the recent changes, I can't get my stuff working again, 😢
This might just be something on my side, I have some tight coupling with your code to make it work in am MVVM way...

Anyway, if you have time I advice you to take a step back and redesign your model. Otherwise you just keep refactoring it, without seeing the big picture.

I would love to support you with this, but I currently can't keep up with my job / the work for Greenshot and my kid, so before I get to it, it's probably weeks later. (I have some holidays coming up)

[rafallopatka]

Hi, Thank you for your reply. I will try to follow your suggestions :) I agree that there are still too many relations between Notifications, DisplayPort and Configuration, and there are plenty of things which are unnecessary or not used in the core library, should be moved to specific plugins. I will try to redesign this and try to not destroy existing functionality. :)

Until this time maybe stick to v2.3.4 to avoid unnecessary changes.

[rafallopatka]

I rolled back nugget v 2.4.0