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.

Sign up for GitHub

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

Jump to bottom

[Notifier] Support for desktop notifications via jolicode/JoliNotif #57683

Merged
merged 1 commit into from
Aug 27, 2024
Merged

[Notifier] Support for desktop notifications via jolicode/JoliNotif #57683

merged 1 commit into from
Aug 27, 2024

Conversation

ahmedghanem00
Copy link
Contributor

Q A
Branch? 7.2
Bug fix? no
New feature? yes
Deprecations? no
Issues Fix #51485
License MIT

Per the linked issue, this PR will provide the ability to display desktop notifications using the Symfony-Notifier-Component, via the underlying package jolicode/JoliNotif in the meantime, or via any other underlying package that may come or exist in the future (after creating the appropriate bridge and attach it to the DesktopChannel of course 😀).

Two additional PRs will also be initiated against symfony/docs && symfony/recipes respectively, once the code review has some positive progress here.

Thanks.

@carsonbot carsonbot added this to the 7.2 milestone Jul 8, 2024
@OskarStark OskarStark requested a review from fabpot July 9, 2024 05:32
@ahmedghanem00 ahmedghanem00 force-pushed the feature/notifier-desktop-notitications branch from cb50c31 to 8b85aff Compare July 9, 2024 10:07
nicolas-grekas
nicolas-grekas reviewed Jul 10, 2024
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are some CS comments.
I'm also wondering if we need another channel type or if we could go with the push channel.
WDYT?

src/Symfony/Component/Notifier/Bridge/JoliNotif/JoliNotifOptions.php Outdated Show resolved Hide resolved
src/Symfony/Component/Notifier/Bridge/JoliNotif/JoliNotifTransport.php Outdated Show resolved Hide resolved
src/Symfony/Component/Notifier/Bridge/JoliNotif/JoliNotifTransport.php Outdated Show resolved Hide resolved
src/Symfony/Component/Notifier/Bridge/JoliNotif/JoliNotifTransport.php Outdated Show resolved Hide resolved
src/Symfony/Component/Notifier/Bridge/JoliNotif/README.md Outdated Show resolved Hide resolved
src/Symfony/Component/Notifier/Bridge/JoliNotif/composer.json Outdated Show resolved Hide resolved
src/Symfony/Component/Notifier/Bridge/JoliNotif/composer.json Outdated Show resolved Hide resolved
src/Symfony/Component/Notifier/Channel/DesktopChannel.php Outdated Show resolved Hide resolved
src/Symfony/Component/Notifier/Channel/DesktopChannel.php Outdated Show resolved Hide resolved
src/Symfony/Component/Notifier/Channel/DesktopChannel.php Outdated Show resolved Hide resolved
@ahmedghanem00
Copy link
Contributor Author

ahmedghanem00 commented Jul 10, 2024 •
edited
Loading

Here are some CS comments. I'm also wondering if we need another channel type or if we could go with the push channel. WDYT?

I actually prefer to create a new channel for this, as sending notifications here relies on executable binaries (some of which exist natively in the OS, and some are shipped portably with the underlying package). So, there are no API calls or network activity involved. And this is quite different for me (at least) from what the PushChannel usually does.

Maybe naming it as DesktopChannel is not the best or most convenient name. But, I think this new bridge, or any similar bridge that may come in the future, should be grouped and tied to a different channel.

@OskarStark
Copy link
Contributor

OskarStark commented Jul 10, 2024 •
edited
Loading

Maybe naming it as DesktopChannel is not the best or most convenient name. But, I think this new bridge, or any similar bridge that may come in the future, should be grouped and tied to a different channel.

Maybe LocalChannel, OfflineChannel or SystemChannel?

@ahmedghanem00
Copy link
Contributor Author

SystemChannel sounds more accurate, as Desktop phrase might understood by some as the desktop-push-notification offered by some online services.

I'll wait for some time before updating to make sure there are no objections or points raised.

@nicolas-grekas
Copy link
Member

Are there any technical implications related to using Push or Desktop for channels? (Desktop looks fine to me if we need to go with another channel)

@ahmedghanem00
Copy link
Contributor Author

No, at least for the current bridge we're adding. I've tested it with PushChannel and it worked normally. Going for another channel was just for consistency as this bridge behaving slightly different from the other bridges.

fabpot
fabpot approved these changes Jul 26, 2024
View reviewed changes
Copy link
Member

@fabpot fabpot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ahmedghanem00 Can you fix the 2 small changes I've added?

src/Symfony/Component/Notifier/Bridge/JoliNotif/JoliNotifOptions.php Outdated Show resolved Hide resolved
src/Symfony/Component/Notifier/Bridge/JoliNotif/JoliNotifOptions.php Outdated Show resolved Hide resolved
src/Symfony/Component/Notifier/Bridge/JoliNotif/JoliNotifTransport.php Outdated Show resolved Hide resolved
@OskarStark
Copy link
Contributor

Can you please open a docs PR? Thanks

@ahmedghanem00
Copy link
Contributor Author

Can you please open a docs PR? Thanks

Will do it

This was referenced Jul 29, 2024
Merged
Merged
@OskarStark
Copy link
Contributor

Can you please check the failing related tests? Thanks

@ahmedghanem00
Copy link
Contributor Author

I've checked but didn't catch anything related. Most of them are because of this PR has not been merged yet.

@OskarStark
Copy link
Contributor

image

That's weird

@xabbuh
Copy link
Member

xabbuh commented Aug 6, 2024

jolicode/jolinotif would probably have to be added to the require-dev section of the root composer.json file.

@ahmedghanem00 ahmedghanem00 force-pushed the feature/notifier-desktop-notitications branch 2 times, most recently from 37688c7 to 81f99b9 Compare August 9, 2024 04:34
@ahmedghanem00
Copy link
Contributor Author

Invalid handler service "texter.messenger.desktop_handler": class or interface "Symfony\Component\Notifier\Message\DesktopMessage" declared in your tag attribute "handles" not found.

Not sure why this class does not get detected during the remaining test (8.2, low-deps).

@OskarStark
Copy link
Contributor

Can you please create a docs PR and a recipe PR? Thanks

@ahmedghanem00
Copy link
Contributor Author

Can you please create a docs PR and a recipe PR? Thanks

They are already created above :)

@xabbuh
Copy link
Member

xabbuh commented Aug 11, 2024

Not sure why this class does not get detected during the remaining test (8.2, low-deps).

The reason is that on this job an older version of the Notifier component is installed. Then, the desktop notifier is not present. We need to make sure that inside the FrameworkExtension we remove service definitions for which the necessary classes do not exist.

@ahmedghanem00 ahmedghanem00 mentioned this pull request Aug 16, 2024
Open
@fabpot
Copy link
Member

fabpot commented Aug 23, 2024

@ahmedghanem00 Can you rebase to get rid of the merge commit?

@ahmedghanem00 ahmedghanem00 force-pushed the feature/notifier-desktop-notitications branch from a1e9bd2 to cef6fc1 Compare August 23, 2024 12:48
@ahmedghanem00
Copy link
Contributor Author

@fabpot Done?

stof
stof reviewed Aug 23, 2024
View reviewed changes
src/Symfony/Component/Notifier/Bridge/JoliNotif/JoliNotifTransportFactory.php
*/
final class JoliNotifTransportFactory extends AbstractTransportFactory
{
private const SCHEME_NAME = 'jolinotif';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should the scheme be joli-notif to be consistent with the package name ?

Copy link
Contributor Author

@ahmedghanem00 ahmedghanem00 Aug 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually feel that it's more readable without the dash, I also have seen many similar bridges following the same pattern in naming the scheme without applying the kebab-case.

src/Symfony/Component/Notifier/Bridge/JoliNotif/README.md Show resolved Hide resolved
@fabpot fabpot force-pushed the feature/notifier-desktop-notitications branch from 3f67b0c to bcd4677 Compare August 27, 2024 06:48
@fabpot
Copy link
Member

fabpot commented Aug 27, 2024

Thank you @ahmedghanem00.

@fabpot fabpot merged commit 825d9bd into symfony:7.2 Aug 27, 2024
6 of 10 checks passed
@fabpot fabpot mentioned this pull request Aug 27, 2024
Closed
@ahmedghanem00 ahmedghanem00 deleted the feature/notifier-desktop-notitications branch August 27, 2024 18:28
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Aug 28, 2024
@javiereguiluz
minor #20076 [Notifier] Create DesktopChannel and JoliNotif bridg…
4203d31 
…e (ahmedghanem00)

This PR was squashed before being merged into the 7.2 branch.

Discussion
----------

[Notifier] Create `DesktopChannel` and `JoliNotif` bridge

Updating docs according to symfony/symfony#57683

Fixes #20168

Commits
-------

45ba59f [Notifier] Create `DesktopChannel` and `JoliNotif` bridge
@fabpot fabpot mentioned this pull request Oct 27, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stof stof stof left review comments

@OskarStark OskarStark OskarStark approved these changes

@fabpot fabpot fabpot approved these changes

@nicolas-grekas nicolas-grekas Awaiting requested review from nicolas-grekas

Assignees
No one assigned
Labels
Feature Notifier Status: Reviewed
Projects
None yet
Milestone
7.2
Development

Successfully merging this pull request may close these issues.

[Notifier] Support for desktop notifications via jolicode/JoliNotif
7 participants
@ahmedghanem00 @OskarStark @nicolas-grekas @xabbuh @fabpot @stof @carsonbot