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

Replace legacy Growl notifications with macOS UserNotificationCenter #434

Open
wants to merge 1 commit into
base: horos
Choose a base branch
from

Conversation

derreisende77
Copy link

Horos minimum OS requirement is 10.11 which has OS X User Notification support. Growl hasn´t been updated for a long time and isn´t necessary.
This PR replaces Growl notifications with calls to NSUserNotificationCenter and removes the framework from the application.

Note that for some unknown reason I wasn´t able to modify the preferences Growl text. This needs to be changed if this PR gets accepted.

@fvpolpeta
Copy link
Collaborator

fvpolpeta commented Nov 18, 2018

Hi @derreisende77

Thanks a lot for your pull request, I will check how to change that in Preferences. You mean the label itself right?

Also, could you please confirm - according to the NSUserNotificatioCenter API - if we should have a release after the line below:

AppController.m - 3286
[[NSUserNotificationCenter defaultUserNotificationCenter] scheduleNotification:notification];
[notification release];

The way it was, I'm not sure this notification was getting released. In other words, some leak that we have the chance to fix in advance.

Thanks

Fauze

@derreisende77
Copy link
Author

Yes I mean the label itself.

I have never seen any code calling [notification release] after scheduling it. So I THINK it will get release after it got displayed by the system. And I couldn't find any references in the docs as well.

@fvpolpeta
Copy link
Collaborator

Ok, thanks. I will check.

It may be hard to find references since it's no longer an active practice manage memory manually in Apps.

Cheers

@aglv
Copy link
Contributor

aglv commented Nov 19, 2018

Yes, release the notification. @derreisende77 maybe you were working in ARC projects?
On my side I'm using [[[NSUserNotification alloc] init] autorelease] and no problem.

@aglv
Copy link
Contributor

aglv commented Nov 19, 2018

I'm still a bit skeptical about this change. Growl has many more notification delivery options (SMS, email, ...) and some users may miss that.

@derreisende77
Copy link
Author

Growl is basically unmaintained code that currently still works. Latest release I have seen is 4 years old.
Yes I do use ARC in projects but so far I have found one Qt sample that uses Autorelease. All other sample code never released. But as you said it might be due to ARC

@fvpolpeta
Copy link
Collaborator

Hi @derreisende77 and @aglv

I don't promise to do this quickly, but I believe both can co-exist behind an abstraction notification.

I understand arguments Growl is old, but I also understand that there are other places in the code that requires attention and the fact that some users may still rely on Growl due to other means.

I will try to see the extension of this work and then make it available in Horos 4.0.0. No timeframe defined yet as we are waiting for some bug resolution in Mojave.

Your feedback and help is much appreciated.

Cheers

Fauze

@jensen0914
Copy link
Collaborator

Work done to solve notarization issues required removing Growl framework and replacing with UserNotifications framework. See pull request #554 (commit f9825a3)

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.

4 participants