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 more params to notify function #1146

Closed
wants to merge 4 commits into from

Conversation

Dubjunk
Copy link

@Dubjunk Dubjunk commented May 15, 2019

When merged this pull request will:

  • title
  • Added queue and lifetime params

I've noticed that the queue prevent to display "urgent notifications" which you can't avoid.
With the provided lifetime and queue parameter you're able to clear the queue and therefore display directly an urgent notification.

@jonpas
Copy link
Member

jonpas commented May 15, 2019

I am not sure I like this. Priority notifications yes, but this will just make everyone force show. We should clearly specify to use this rarely or something like that.

@commy2
Copy link
Contributor

commy2 commented May 16, 2019

I noticed the same problem.

Is the function still backwards compatible with the changed params?

My idea was to have a priority value (NUMBER) for each notification and that changes the position in queue including potentially changing the displayed notification (maybe pusing the previous one to be displayed again down the queue).

@Dubjunk
Copy link
Author

Dubjunk commented May 16, 2019

It's not backwards compatible.
A priority value would be nice too but in my case I don't want to show the "old" notify.

Maybe change the queue param to Bool, Number
On true -> normal queue
On false -> force show and empty the queue
Number -> add a priority to it

@TheMagnetar
Copy link

If it's not backward compatible make it a new function.

@commy2
Copy link
Contributor

commy2 commented May 16, 2019

Maybe change the queue param to Bool, Number

Sounds good, except that I would make those two different params (arguments).

@jonpas
Copy link
Member

jonpas commented Jul 3, 2019

I would prefer "don't stop queue" parameter instead of "force show", meaning if another notification comes in, it instantly overwrites it. Also solves the problem of mods forcing away important notifications with less important ones, but at the same time hides specifically marked less important ones right away.

I like lifetime.

@jonpas
Copy link
Member

jonpas commented Jul 3, 2019

Replaced by #1175.

@jonpas jonpas closed this Jul 3, 2019
@commy2 commy2 added this to the 3.12 milestone Jul 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants