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

feat: Add wsl-send-notify for WSL based Linux #101

Merged
merged 3 commits into from
Apr 24, 2024

Conversation

TheoD02
Copy link

@TheoD02 TheoD02 commented Apr 2, 2024

Hello

Added notification functionality for Windows Subsystem for Linux (WSL). Allows notifications to be sent using the wsl-notify-send command, with support for defining title, body.

I implemented it quickly, ready for review :)

Related to this issue

Copy link
Member

@pyrech pyrech left a comment

Choose a reason for hiding this comment

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

Could you add some documentation about this new notifier? And also explain where does this notifier comes from (from a security and license point of view)

src/Notifier/WSLTerminalNotifier.php Outdated Show resolved Hide resolved
@TheoD02 TheoD02 force-pushed the feat/add_wsl-send-notify branch 3 times, most recently from 38dcf7b to caa9eca Compare April 3, 2024 16:32
@pyrech
Copy link
Member

pyrech commented Apr 24, 2024

I ensured the binary is the one from the linked project and added a readme and the license from the project. Thanks @TheoD02 for the PR 🙏

@pyrech pyrech merged commit 18aa721 into jolicode:main Apr 24, 2024
4 checks passed
@pyrech
Copy link
Member

pyrech commented Apr 25, 2024

@TheoD02 I see that you added the new WslNotifySendNotifier in the Unix notifiers list. But on WSL, the factory looks for the Windows notifiers. So i doubt it works. Can you confirm whether this needs to be changed?

@TheoD02
Copy link
Author

TheoD02 commented Apr 25, 2024

u added the new WslNotifyS

Hello,

Yes indeed, I don't remember why I put it in Unix 😯

Need to change that to Windows, actually it's incorrect

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

3 participants