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

fix(bgnotify): use terminal-notifier args properly #12467

Merged
merged 1 commit into from
Jun 4, 2024

Conversation

davidlj95
Copy link
Contributor

@davidlj95 davidlj95 commented Jun 2, 2024

Standards checklist:

  • The PR title is descriptive.
  • The PR doesn't replicate another PR which is already open.
  • I have read the contribution guide and followed all the instructions.
  • The code follows the code style guide detailed in the wiki.
  • The code is mine or it's from somewhere with an MIT-compatible license.
  • The code is efficient, to the best of my ability, and does not waste computer resources.
  • The code is stable and I have tested it myself, to the best of my abilities.
  • N/A If the code introduces new aliases, I provide a valid use case for all plugin users down below.

Changes:

bgnotify notifications don't work in macOS when using latest version of terminal-notifier

After trying out the command calling terminal-notifier used to send notifications manually, could reproduce the issue. Removing -sender arg seems fixed the issue.

After reading terminal-notifier's README seems this is actually expected:

-sender ID
[...]
Because of this it is important to note that you cannot combine this with options like -execute and -activate which depend on the sender of the notification to be ‘terminal-notifier’ to perform its work.

Using therefore only -activate argument so that the terminal is opened when tapping the notification

  • Remove -sender flag. Use -activate only as instructed per terminal-notifier docs

Other comments:

-sender and -activate can't be used together
@ohmyzsh ohmyzsh bot added the Area: plugin Issue or PR related to a plugin label Jun 2, 2024
@carlosala carlosala self-assigned this Jun 3, 2024
@carlosala
Copy link
Member

carlosala commented Jun 3, 2024

Hey! Is there any changelog or something, or you know which is the first version of terminal-notifier that requires it? It'd be great to keep compatibility with older versions.

@davidlj95
Copy link
Contributor Author

davidlj95 commented Jun 3, 2024

Hey! Is there any changelog or something, or you know which is the first version of terminal-notifier that requires it? It'd be great to keep compatibility with older versions.

After git blame on the README, seems that the docs about not using -sender and -activate together dates back to 2013. Which appears to be v1.5.2 . Latest is v2.0.0 but anyway more than ten years since this was documented as not correct.

Checked when did the usage of -activate and -sender started appearing all together in this plugin. It dates back to 2015.

Seems the issue also exists in the t413/zsh-background-notify repo where this plugin comes from. With an analogous PR offering same fix

My guess is it has been working until now despite not being the correct way of doing it. And maybe some macOS update dropped support for that. Just guessing though

@davidlj95
Copy link
Contributor Author

PD: The wording in the PR description about when using latest version was because I'm just trying this plugin in macOS after not using this plugin for a long time (and last time I used it in Arch Linux). And installed latest version of terminal-notifier. But don't know if installing previous version 1.8.0 of 2017 would work.

I'm guessing it wouldn't anyway as looks like some kind of macOS API's issue. But again just a guess

@carlosala carlosala merged commit e53edd7 into ohmyzsh:master Jun 4, 2024
2 checks passed
@carlosala
Copy link
Member

Nice! It seems it's old enough to change it directly.
Thanks for the contribution!

@davidlj95 davidlj95 deleted the fix-bgnotify-macos branch June 4, 2024 13:57
ttelford pushed a commit to ttelford/oh-my-zsh that referenced this pull request Jun 26, 2024
naveen-u pushed a commit to naveen-u/ohmyzsh that referenced this pull request Jul 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: plugin Issue or PR related to a plugin
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants