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 Windows support to system notifier #25

Merged
merged 4 commits into from
May 17, 2024

Conversation

jmartinn
Copy link

This pull request introduces Windows notification support for the System Notifier, extending functionality in addition to the existing support for macOS and Linux. This enhancement addresses the existing issue of platform-specific notifications and aims to provide a seamless user experience across all major operating systems.

Changes Proposed:

  • Windows Notifications: Implement notifications using native PowerShell commands without requiring external dependencies. So we can use it out of the box.

Rationale:

The current implementation of the notification system only supports macOS, using AppleScript, and Linux. This pull request aims to provide an inclusive solution that accommodates users on Windows An alternative approach was considered (using the BurntToast PowerShell module for Windows; see #15), but it was not quite accurate in my opinion due to its dependency requirements which could complicate the setup process for users. My proposed solution uses native system capabilities to ensure broader compatibility and ease of use.

Implementation Details:

-- Windows Notification
elseif util.get_os() == util.OS.Windows then
    os.execute(string.format([[
        PowerShell -Command "Add-Type -AssemblyName System.Windows.Forms;
        $notify = New-Object System.Windows.Forms.NotifyIcon;
        $notify.Icon = [System.Drawing.SystemIcons]::Information;
        $notify.BalloonTipIcon = [System.Windows.Forms.ToolTipIcon]::Info;
        $notify.BalloonTipText = 'Timer #%d, %s%s';
        $notify.BalloonTipTitle = 'Timer done!';
        $notify.Visible = $true;
        $notify.ShowBalloonTip(10000);" ]],
        self.timer.id,
        util.format_time(self.timer.time_limit),
        repetitions_str
    ))

Conclusion:

This update ensures that all users, regardless of their operating system, can benefit from the same level of functionality when using the Pomodoro timer plugin. By leveraging native system features, we maximize compatibility and maintain ease of use without additional dependencies.

@jmartinn
Copy link
Author

Note: This would completely close / solve #3

@epwalsh epwalsh linked an issue May 17, 2024 that may be closed by this pull request
Copy link
Owner

@epwalsh epwalsh left a comment

Choose a reason for hiding this comment

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

@jmartinn this is great! Thanks a lot. Can you just update the note in the README on line 121 that still says the sys notifier is only available on MacOS natively.

@jmartinn
Copy link
Author

jmartinn commented May 17, 2024

@epwalsh sure! Sorry for the late reply btw. I've changed the line you've mentioned and deleted the line 122 which contained the url to track the issue.

Copy link
Owner

@epwalsh epwalsh left a comment

Choose a reason for hiding this comment

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

LGTM!

@epwalsh epwalsh merged commit ed72657 into epwalsh:main May 17, 2024
5 checks passed
@jmartinn jmartinn deleted the system-notifier branch July 27, 2024 11:00
@zehua0417
Copy link

Thank you very much for the PR you provided

But I noticed a small issue. Did you forget to add Windows to SystemNotifier.supported_oss?

Additionally, I found that simplifying the PowerShell command to a single line string is necessary for it to work properly on my computer. That is:

SystemNotifier.supported_oss = { util.OS.Darwin, util.OS.Linux, util.OS.Windows }

-- ....

elseif util.get_os() == util.OS.Windows then
    os.execute(string.format(
      [[
        PowerShell -Command "Add-Type -AssemblyName System.Windows.Forms; $notify = New-Object System.Windows.Forms.NotifyIcon; $notify.Icon = [System.Drawing.SystemIcons]::Information; $notify.BalloonTipIcon = [System.Windows.Forms.ToolTipIcon]::Info; $notify.BalloonTipText = 'Timer #%d, %s%s'; $notify.BalloonTipTitle = 'Timer done!'; $notify.Visible = $true; $notify.ShowBalloonTip(10000);"
      ]],
      self.timer.id,
      util.format_time(self.timer.time_limit),
      repetitions_str
    ))

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.

Add System Notifier support for Linux and Windows
3 participants