Skip to content

fix: only show showNotificationsCountInTray setting on MacOS #1031

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

Merged

Conversation

adufr
Copy link
Contributor

@adufr adufr commented Apr 17, 2024

Looks like the Electron method we're using for the showNotificationsCountInTray setting is MacOS only 🙈
image

@setchy
Copy link
Member

setchy commented Apr 17, 2024

@adufr - we should also update the following, right?

gitify/src/context/App.tsx

Lines 117 to 122 in 23c1bae

if (settings.showNotificationsCountInTray && count > 0) {
updateTrayTitle(count.toString());
} else {
updateTrayTitle();
}
}, [settings.showNotificationsCountInTray, notifications]);

@adufr
Copy link
Contributor Author

adufr commented Apr 17, 2024

Mmh I don't know, the setting is false by default, so this shouldn't be a problem..?

@setchy
Copy link
Member

setchy commented Apr 17, 2024

Mmh I don't know, the setting is false by default, so this shouldn't be a problem..?

what if someone has changed it already ;) now that you're hiding the setting, they won't be able to disable it

@setchy
Copy link
Member

setchy commented Apr 17, 2024

I'm also torn between hiding the setting, or disabling it and having a tooltip explaining what this is macOS only...

@adufr
Copy link
Contributor Author

adufr commented Apr 17, 2024

what if someone has changed it already ;) now that you're hiding the setting, they won't be able to disable it

k but how do we handle this case? should we return early in this useEffect if the platform isn't MacOS?
and I don't know how electron behaves if you call a feature that isn't available on your platform but I think that it must fail silently so even if some people enabled it I don't think this causes any issues

I'm also torn between hiding the setting, or disabling it and having a tooltip explaining what this is macOS only...

I don't really see the point of having a setting that you will never be able to turn on 😅

@bmulholland
Copy link
Collaborator

Yeah, IMHO best practice here would be to hide the setting AND force it to false wherever it's used (overriding anyone who sets it to true so far, because of a future bug, etc)

@setchy setchy merged commit 8ce79fe into gitify-app:main Apr 17, 2024
@adufr adufr deleted the fix/notifications-count-in-tray-macos-only branch April 18, 2024 06:25
@setchy setchy added the bug Something isn't working label Jul 13, 2024
@setchy setchy added this to the Release 5.4.0 milestone Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants