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

Fixed alerts not counting down #2167

Closed
wants to merge 3 commits into from

Conversation

daniel-samson
Copy link
Contributor

@daniel-samson daniel-samson commented Sep 8, 2016

Description

These fixes make the alerts count down so that the user gets the notification reminder.

relates to issue #2082

Motivation and Context

When a user set up a reminder on calls or meetings they do not receive a desktop notification.

How To Test This

Create a popup reminder in calls and meetings.
Wait for the notification.

If you want to check the counting down you can run the following in the browser console:

setInterval(function() { console.log(alertList[0].time) }, 1000);

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Final checklist

  • My code follows the code style of this project found here.
  • My change requires a change to the documentation.
  • I have read the How to Contribute guidelines.

@KhyberPass
Copy link
Contributor

Hi @daniel-samson, I was having some issues with alerts and came across your proposed fix.
I did some work on alerts some time ago when they were not working for me in #766 so have looked into this before.
In general alerts are working for me in 7.7.4 but have some issues.
Looking at your proposed fix to the current issue I am not too sure if it is fixing anything, but could be missing something. Instead of using the setTimeout() to count up a single variable and compare against the time in each alert the proposal counts down each alert.
The change does make it easier to see what is going on and debug. Does it fix any issues?

The biggest issue with the current way the alerts work is setTimeout(), in Chrome when the tab is inactive then setTimeout() does not trigger every second, this causes the alert to popup at the wrong time or not at all. If you use the debug below, switch to another tab for a bit and come back you will see that the count up (secondsSinceLoad) is not correct.
console.log("Alerts:", alertList.length, "-", alertList[0].time, "Time:", secondsSinceLoad);
I am looking into a better way of doing this to make it more reliable.
Not too sure if you are doing the same too?
Nick

@daniel-samson
Copy link
Contributor Author

@KhyberPass Sorry for the delay. The this fix is to try and force the time left to count down. So that if it passes the threshold it displays the notification. Perhaps we should change the setTimeout to setInterval instead.

@KhyberPass
Copy link
Contributor

Hi,
Yes, I can see where you are going with this but not too sure if it will change anything.

I have looked at alerts as they do not always work for me. One of the main problems is to do with setTimeout() that calls checkAlerts() every second.

setTimeout() or setInterval () is not very reliable. On Chrome, for example, if the tab is not selected setTimeout() does not run with the same priority and therefore the countdown loses time and alerts show late or not at all. There are a number of ways to fix this that I have read about, the best one would be to have the alert time as epoch sent from the server rather than a countdown and check this on the browser side with a calibrated current time.

Also there are some improvements in Reminder.php that can be made. Currently a check is done on the event time not taking into account the reminder time i.e. an event happens at 10:30 with a 15 min reminder. The check is done on 10:30, this means that between 10:15 and 10:30 a negative countdown time is sent to the browser javascript code.

There is also a very small possibility that alerts can be missed with the current implementation. IF an alert in the browser if just about to trigger and a page is loaded by the time the server side query is done the reminder time could have just ticked over and the alert would not be generated.

I am currently doing a fix to Reminders for #2246. To do this I have written some unit tests for Reminders. When this is done I was going to look at improving Alerts.

Nick

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