Notification queue/cap (MaxNotifications)

[corradedied]

Firing many notifications fast caused unbounded screen clutter with no way to remove old ones — Library.Notifications is an unordered dictionary, so "oldest" was unknowable.

What changed

• Library.NotificationQueue — ordered array parallel to the existing dict, so oldest is always [1]
• Library.MaxNotifications (default 10) — on each Notify insert, removes from the front of the queue until under cap. 0 = unlimited. Persist notifications are exempt and will remain untouched.
• Data:Destroy() — removes the entry from the queue on natural expiry so dead references don't block future evictions
• Templates.Window + CreateWindow — wires the setting into the window config so it can be set at the call site:

Library:CreateWindow({ MaxNotifications = 5 })

Unload path clears NotificationQueue alongside Notifications.

[mstudio45]

@corradedied Implement Window:SetMaxNotifications please.

[corradedied]

@corradedied Implement Window:SetMaxNotifications please.

Sure, I'll start doing that now.

[mstudio45]

@corradedied Implement Window:SetMaxNotifications please.

Sure, I'll start doing that now.

I don't think this feature needs to get implemented, I don't really see the use case for this and I just see downsides to this.
And I don't really like how it is yielding the code.

Also this code down below only shows notifications 6 to 15 so the queue management does not work properly:

for i = 1, 15 do
	Library:Notify({
		Title = "Notification number:",
		Description = i,
		Time = 5
	})
end

[corradedied]

@corradedied Implement Window:SetMaxNotifications please.

Sure, I'll start doing that now.

I don't think this feature needs to get implemented, I don't really see the use case for this and I just see downsides to this. And I don't really like how it is yielding the code.

Also this code down below only shows notifications 6 to 15 so the queue management does not work properly:

for i = 1, 15 do
	Library:Notify({
		Title = "Notification number:",
		Description = i,
		Time = 5
	})
end

I'll look into making the queue management actually work properly. On the "yielding" complaint - I don't see an actual coroutine yield anywhere in SetMaxNotifications or the removal path (Destroy() only uses non-blocking
TweenService:Create():Play() and task.delay, neither of which blocks the caller). If you saw something specific yielding, let me know where and I'll take another look.

[FlamesW]

@corradedied Is that okay to add an 'X' close button with its custom callback?

[corradedied]

@corradedied Is that okay to add an 'X' close button with its custom callback?

Yeah, I could add a close button to the notifications but how would a custom close callback be used?

[FlamesW]

@corradedied Its simple you add in callback that triggers when u click

Library:Notify({
    Title = "mspaint",
    Description = "Hello world!",
    Time = 4,
    CloseButton = true, -- // Adds the close button.
    SyncWithTime = true, -- // This is less relevant but makes the callback able to trigger when the notification disappears due to timeout
    Callback = function(Notification)

    end
})

Also I recommend to set time to 9e9 when its not specified and CanClose is true

[FlamesW]

You're right it rarely even serves a purpose but its better, this library has proven to be very flexible and this is sheer proof you can do anything with it
Also adding kind of mini dialog on notification would be amazing like Accept/Decline type notifications

[mstudio45]

@corradedied Its simple you add in callback that triggers when u click

Library:Notify({
    Title = "mspaint",
    Description = "Hello world!",
    Time = 4,
    CloseButton = true, -- // Adds the close button.
    SyncWithTime = true, -- // This is less relevant but makes the callback able to trigger when the notification disappears due to timeout
    Callback = function(Notification)

    end
})

Also I recommend to set time to 9e9 when its not specified and CanClose is true

Persist = true exists so you dont need to use math.huge for Time. I see the "X" button more useful then notification queue in my opinion, but I would only enable the "X" button if its Persistent and has an Callback function.

[FlamesW]

@corradedied You're right, but its important to add a support for X button to set presist to true if time is not specified also what's a notification queue? A limited amount of notifications appearing over screen?

[FlamesW]

@corradedied Is that okay to talk with you over discord? Its easier that way

[corradedied]

@corradedied Is that okay to talk with you over discord? Its easier that way

Sure, add corradeknight on Discord.

[corradedied]

@corradedied You're right, but its important to add a support for X button to set presist to true if time is not specified also what's a notification queue? A limited amount of notifications appearing over screen?

Notifications still display on screen normally and the value of Window.MaxNotifications determines how much notifications can be visible at once. If the user sends a notification beyond the set MaxNotifications value, that notification will be queued and will be shown when a past notification dismisses.

[corradedied]

@corradedied Is that okay to add an 'X' close button with its custom callback?

@mstudio45 I'm probably going to be making a feature like this to allow the user to close a Notification early with an optional callback.
I have a few questions regarding this feature and how you want this implementation to work:

• Should all Notifications get a Callback option, regardless if they are able to closed or Persistent?
  Would you rather:

1. The "X" (close) button be automatically enabled if the Notification is set as Persistent and has a Callback and not enabling the "X" button for Notification with set Time.
2. Have a value that can toggle the close button and change the callback firing behavior each notification?
3. Or however else you want this implemented. Below is a quick example of how the functionality is laid out currently.

Library:Notify({
    Title = "mspaint",
    Description = "Hello world!",
    Time = 4,
    Closable = true, -- adds the close ("X") button
    CallbackOnAutoDismiss = true, -- fire Callback on auto-dismiss/timeout, not manual close. false will make it fire the Callback only when the user clicks the "X", never on auto-dismiss
    Callback = function(Notification)
        print(Notification)
    end
})

[FlamesW]

@corradedied

I think they should all get callback actually its better

Library:Notify({
    Title = "mspaint",
    Description = "Hello world!",
    Time = 4,
    Closable = true, -- adds the close ("X") button
    CallbackOnAutoDismiss = true, -- fire Callback on auto-dismiss/timeout, not manual close. false will make it fire the Callback only when the user clicks the "X", never on auto-dismiss
    Callback = function(Notification, OnClose)
        print(Notification)
    end
})

And option 2 is better because its more flexible
But ensure if Time is not defined then set Presist = true regardless and if Presist is defined but set to false force it to presist because why should we make it disappear if we didn't set a time for it to and we already using close button variant, also you should ensure :Destroy or the responsible function for closing off the notification to have the code that triggers all the callbacks and has the 'auto dismiss' feature implemented in it

Hey I just thought how about adding a second parameter called OnClose if that works then we dont need CallbackOnAutoDismiss state and its cleaner that way.. also keep in mind Notification parameter is for handling with the notification properties like

print(Notification.Title)

As for OnClose we can simply do

if OnClose then

end

I'm not sure if it can even work out but hey just an idea as for anything implemented outside is already triggering on load cause notification appeared but anything that was implanted inside OnClose only triggers when notification is destroyed or closed via button

[corradedied]

@corradedied

I think they should all get callback actually its better

Library:Notify({
    Title = "mspaint",
    Description = "Hello world!",
    Time = 4,
    Closable = true, -- adds the close ("X") button
    CallbackOnAutoDismiss = true, -- fire Callback on auto-dismiss/timeout, not manual close. false will make it fire the Callback only when the user clicks the "X", never on auto-dismiss
    Callback = function(Notification, OnClose)
        print(Notification)
    end
})

And option 2 is better because its more flexible But ensure if Time is not defined then set Presist = true regardless and if Presist is defined but set to false force it to presist because why should we make it disappear if we didn't set a time for it to and we already using close button variant, also you should ensure :Destroy or the responsible function for closing off the notification to have the code that triggers all the callbacks and has the 'auto dismiss' feature implemented in it

Hey I just thought how about adding a second parameter called OnClose if that works then we dont need CallbackOnAutoDismiss state and its cleaner that way.. also keep in mind Notification parameter is for handling with the notification properties like

print(Notification.Title)

As for OnClose we can simply do

if OnClose then

end

I'm not sure if it can even work out but hey just an idea as for anything implemented outside is already triggering on load cause notification appeared but anything that was implanted inside OnClose only triggers when notification is destroyed or closed via button

Sorry for the late reply. I've been taking a break but here is the PR that implements this: Add closable notifications with dismiss-aware callbacks

[deividcomsono]

The problem is what about new notifcations when there are multiple persistent notifications or long time notifications? The new notification will just get stuck and will only appear a good time after the event happened, which can confuse the user.

The notification can also be stuck (out-of-screen) with the current system, but the notification just won't be visible. By the time old notifications get destroyed this notification would already be destroyed, not confusing the user with delayed warnings.

The idea is good but it will also add a layer of confusion for the user, so I won't be accepting queues.

I think a good solution would be adding a "Priority" for notifications, basically if one notification is urgent, you can do Priority = 0, and if another is just normal, you do Priority = 1, so every urgent notification is above the normal notifications.
If you or @mstudio45 have other ideas, send here, I'll be checking as soon as possible.

(Also, Missing the ChangeStep in Library:Notify is not a good thing, because if it's stuck, then it won't change the step and can skip steps for the user when it is visible. (e.g. changing to step 2 when not visible, then changing to step 3 when visible. that would make the notification show step 0 but then suddenly skip to step 3). That can easily be fixed by setting a Data.Step.)