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 notification support to timers #1064

Open
christianlupus opened this issue Jul 2, 2022 · 12 comments · May be fixed by #1409
Open

Add notification support to timers #1064

christianlupus opened this issue Jul 2, 2022 · 12 comments · May be fixed by #1409
Assignees
Labels
enhancement New feature or request Frontend Issue or PR related to the frontend code javascript Pull requests that update Javascript code

Comments

@christianlupus
Copy link
Collaborator

christianlupus commented Jul 2, 2022

For the RecipeTimer components, we could enable the desktop notifications to notify the user of end of timer.


Depends on #1067

@christianlupus christianlupus added enhancement New feature or request javascript Pull requests that update Javascript code Frontend Issue or PR related to the frontend code labels Jul 2, 2022
@MarcelRobitaille
Copy link
Collaborator

MarcelRobitaille commented Jul 22, 2022

I started poking at this issue. I'm not sure if you intended to use browser notifications or integrate with the Nextcloud Notification app. I looked at their docs, and I don't see any example to trigger it from the browser. Their example is from the backend, is overly complicated for this IMO. I think normal browser notifications would be better, at least for now.

Edit: I wonder if this replaces alert() or do we keep both? We could use alert() only if we don't have notification permission, or we could keep both.

https://github.com/MarcelRobitaille/cookbook/tree/1064-notifications

@MarcelRobitaille MarcelRobitaille self-assigned this Jul 22, 2022
@christianlupus
Copy link
Collaborator Author

christianlupus commented Jul 22, 2022

I think the desktop notifications are better for now than the nextcloud notifications as a first step.

The nextcloud notifications would have the benefit that all devices get the notification and the original does not need to be online. I am thinking of a slow cooking fish which takes hours or even days. Your tablet/computer will not run all the time without closing the page. I would postpone that however for now. It will Twitter also a new API backend. We could also ask in a poll if this feature was even considered helpful.

Regarding the desktop notifications, i think it makes sense to encapsulate things in its own js file. There is already a stub pre-implemented in

cookbook/src/js/helper.js

Lines 128 to 149 in a96c973

function notify(title, options) {
if (!("Notification" in window)) {
return
}
if (Notification.permission === "granted") {
// eslint-disable-next-line no-unused-vars
const notification = new Notification(title, options)
} else if (Notification.permission !== "denied") {
Notification.requestPermission((permission) => {
if (!("permission" in Notification)) {
Notification.permission = permission
}
if (permission === "granted") {
// eslint-disable-next-line no-unused-vars
const notification = new Notification(title, options)
} else {
// eslint-disable-next-line no-alert
alert(title)
}
})
}
}
. you can add further files there for simpler code structure 😉.

@MarcelRobitaille
Copy link
Collaborator

I didn't know about that function. Sorry. Isn't it better to request notification permissions before starting the timer though? If you go to a different page, you might not see the request to allow notifications when the timer goes off

@christianlupus
Copy link
Collaborator Author

Sure, the function was never used and is not finished. I would suggest to break up the functionality into multiple parts (request notification permission, send notification, and optionally create notification timer). The request for permission should be triggered while starting the timer. Then the user has a real chance to consent.

If you go to a different page, you might not see [...]

We have however a different understanding of what should be implemented: A page can be unloaded for various reasons (user closes tab, browser is closed by OS's energy saving options, user uses multiple tabs and old tabs get shut down, ...). As soon as that happens, and frontend JS code is aborted including all timers. So, no notifications can be generated anymore.

Are you intending to create a ServiceWorker for the app to keep running in the background or something in this direction?

@MarcelRobitaille
Copy link
Collaborator

We have however a different understanding of what should be implemented: A page can be unloaded for various reasons (user closes tab, browser is closed by OS's energy saving options, user uses multiple tabs and old tabs get shut down, ...). As soon as that happens, and frontend JS code is aborted including all timers. So, no notifications can be generated anymore.

I meant go to another tab, but leave the nextcloud tab running in the background. I think it's reasonable to expect a tab to stay open if there's a timer running. Alternatively, we could integrate with the Nextcloud Notification app and try to send push notifications, but we'd need some backend and Nextcloud notifications are flaky in my experience.

@MarcelRobitaille
Copy link
Collaborator

Sure, the function was never used and is not finished. I would suggest to break up the functionality into multiple parts (request notification permission, send notification, and optionally create notification timer). The request for permission should be triggered while starting the timer. Then the user has a real chance to consent.

Maybe we could do a sort of factory function that returns alert if the notification API is not available or if the request to allow them is declined, and new Notification if everything is good.

@christianlupus
Copy link
Collaborator Author

I meant go to another tab, but leave the nextcloud tab running in the background. I think it's reasonable to expect a tab to stay open if there's a timer running. Alternatively, we could integrate with the Nextcloud Notification app and try to send push notifications, but we'd need some backend and Nextcloud notifications are flaky in my experience.

Yes, but at least on my Android Firefox, tabs in the background tend to get stopped soon. Unfortunately, you will not get notified or see the effect. I just wanted to bring that to the mind.

For long-running processes (!) the NC notifications might work. Sending out a notification will alert all connected devices. However, they might take some time as there is some polling involved, internally as far as I know. So, this is not useful for time spans < 30 or even < 60 min. Starting at a certain duration, the jitter might be neglectable.

With "we'd need some backend" you mean an API endpoint plus some PHP code within the app, am I right? Yes, that is true but should be doable.

Maybe we could do a sort of factory function that returns alert if the notification API is not available or if the request to allow them is declined, and new Notification if everything is good.

True, that will encapsulate the critical part (notification API) completely. Just a remark to consider when defining the API of the module: Keep #842 in mind. It might be feasible to pass some context parameters around.

@christianlupus christianlupus added this to the Release 0.9.14 milestone Jul 25, 2022
@MarcelRobitaille MarcelRobitaille changed the title Ad notification support to timers Add notification support to timers Jul 26, 2022
@christianlupus
Copy link
Collaborator Author

christianlupus commented Oct 19, 2022

Depends on #1067

This must be in the initial post of the issue, @MarcelRobitaille. Otherwise, it will not work. Can you edit the posts in general? In this post I edited myself

@MarcelRobitaille
Copy link
Collaborator

@christianlupus Sorry, I didn't know how that bot worked. Is the following needed as well like you have it?

---

I do have permission to edit initial posts. In the future, do you mind if I edit comments to add things like that? I am a bit hesitant to edit somebody else's comment.

@christianlupus
Copy link
Collaborator Author

Yes, that was my concern as well. The three dashes are not needed. There must just be a line matching a regex (line is depends on # or Blocked by #). The three dashes are there, so the altering of the comment is not too invasive. We could also as a manual line by us (added by @).

@christianlupus
Copy link
Collaborator Author

As this is dependent on another PR to be merged, let's postpone it to the next feature release.

@github-actions
Copy link

This PR/issue depends on:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Frontend Issue or PR related to the frontend code javascript Pull requests that update Javascript code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants