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

Support using the notifications API #9

Open
tdulcet opened this issue May 17, 2021 · 5 comments
Open

Support using the notifications API #9

tdulcet opened this issue May 17, 2021 · 5 comments
Labels
enhancement New feature or request

Comments

@tdulcet
Copy link

tdulcet commented May 17, 2021

Provide the option of using the notifications API to display notifications. This is needed for extensions that do not have a popup (like Unicodify) or that need to display notifications when both the popup and options page are not open. This could also be used to show errors to the user instead of silently failing.

Additionally, you could provide the option of using the future notification bar API in Thunderbird (see Bug 1674002).

@rugk rugk added the enhancement New feature or request label May 17, 2021
@rugk
Copy link
Member

rugk commented May 17, 2021

Yeah, a good idea. Though I’m not sure whether it needs that. I'm using notifications in Mastodon Simplified Federation and just used the API as it is.

Could you describe the use case a little more?
IIRC the module here was mainly to have message types with different (standardized) appearances. Notifications of course always look the same, except that we could prefix the message with “Error: “ or so – which I’d however avoid as it makes the messages not really more readable and is just not userfriendly.

Additionally, you could provide the option of using the future notification bar API in Thunderbird (see Bug 1674002).

Could we tackle that in a new issue at least? … it’s yet another feature I guess. 🙃

@tdulcet
Copy link
Author

tdulcet commented May 17, 2021

Could you describe the use case a little more?

Your RandomTips library uses this to show all its notifications, although it is not very useful for add-ons like Unicodify. If the user never opens the options page (probably many users), they will not see any of the tips. If you do not want to update this library, I would be fine if the RandomTips library instead had the option of using the notifications API directly.

I was also thinking for Unicodify, we could show an error if the text transformation fails for some reason. Same for the Awesome Emoji Picker, if inserting the emoji into the page fails, we could show a message that it was copied to the clipboard. Although, we could of course use the notifications API directly for these two specific use cases.

IIRC the module here was mainly to have message types with different (standardized) appearances. Notifications of course always look the same, except that we could prefix the message with “Error: “ or so – which I’d however avoid as it makes the messages not really more readable and is just not userfriendly.

You can set an icon for each notification to use your SVGs. You can also prefix the title of each message with an emoji to replicate those SVGs.

Could we tackle that in a new issue at least? … it’s yet another feature I guess. 🙃

Well it has not yet been added to Thunderbird. I only mentioned it because it will be an almost exact replacement for this library and the preferred method to show all notifications in Thunderbird.

@rugk
Copy link
Member

rugk commented Aug 22, 2022

Your RandomTips library uses this to show all its notifications, although it is not very useful for add-ons like Unicodify. If the user never opens the options page (probably many users), they will not see any of the tips.

That's a very good point and I'm, having the same issue for my Mastodon extension: rugk/mastodon-simplified-federation#24
However, i did not prioritize it there given I see no big use of tips there and the user base is fairly small.

But if that is implemented here, I guess, that could be used there, too.

Anyway, the questions stands whether MessageHandler or RandomTips may be better suitable. Some arguments:

  • We could replace the Notifications.js module some add-ons have with a proper implementation here.
    Likely, this should be kept as a separate entry point, also because you wanna pass a title etc. which is not supported by the usual HTML messages.
    Also note that translations with parameters(substitutions are not really supported here, which kinda surprises me.
  • In an implementation, HTML messages should IMHO have preference. However, when no HTML message-box could be found (because there is not HTML e.g.), it should fallback to a browser notification.

-> I guess that is argument enough to make it it's own TinyWebExt/Notifications module or so.
Actually, thinking about it, these are enough arguments for it to be used as a separate module. When showing a notification you as a developer usually know better where it should be shown or so. In some circumstances (such as a popup closing or so), it may even be needed to know where it is shown, respectively you want to always show a notification there.
Furthermore, one needs a special permission for notification, so blindly falling back here again, can create problems.
So I guess this is better to be implemented in the RandomTips module to automatically fallback to n

So some acceptance criteria:

  • extract notification module into TinyWebExt module, best base don https://github.com/rugk/mastodon-simplified-federation/blob/master/src/common/modules/Notifications.js, so it can detect add-on name (using that by default) and icon automatically.
  • merge that implementation with the one from Unicodify to allow passing of translation parameters
  • finally, use that to implement an automatic fallback for the RandomTips library, so when the HTML element to show permissions is not there, it uses the notification instead. (Or maybe a global toggle/different entry function would be better here? But I doubt it.)
  • Text boxes or so may need to be adjusted, as these again are not there.
  • RandomTips lib should check for the notification permission and stop/show an error if it is not there when it tries to show such a notification.
  • in the RandomTips lib document that one may need the new permission if the fallback is intended to be used

@rugk
Copy link
Member

rugk commented Aug 22, 2022

Given what I said I move this to RandomTips library.

@rugk rugk transferred this issue from TinyWebEx/MessageHandler Aug 22, 2022
@tdulcet
Copy link
Author

tdulcet commented Aug 23, 2022

When showing a notification you as a developer usually know better where it should be shown or so.

As you explained in rugk/mastodon-simplified-federation#24, the tips should only be shown in response to an explicit user action. For Unicodify, maybe after the user does one of the Unicode font conversions. However, unlike for the popup and options page where the tips are typically shown frequently, I would argue that desktop notifications should be shown very rarely and maybe only once. The user should also be able to completely disable the desktop notifications. In addition, for add-ons like the Awesome Emoji Picker that already have an action popup, they would probably never need to show tips in a desktop notification.

RandomTips lib should check for the notification permission

I believe it could just check if browser.notifications is defined.

Note that many of your tips of course have a button that performs some action, such as opening the respective AMO/ATN reviews page so the user can rate/review the add-on. However, those existing Notifications.js modules do not support the user clicking on the notifications.

I have a PR for Unicodify that will show a notification after it is updated, where clicking the notification will open the release notes/changelog on AMO/ATN. This should show the changes that would be needed to the Notifications.js module and may be a good time to perform the refactor. (It is just waiting on rugk/unicodify#72.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants