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: implement showNotificationsCountInTray setting #780

Closed

Conversation

adufr
Copy link
Contributor

@adufr adufr commented Feb 16, 2024

Related issues

#750

Context

Introduce a new setting (disabled by default), that uses the tray.setTitle() API to show the notifications count next to the tray icon.

image

Discussion

There are quite a lot of changes because I had to pass settings into a few methods so that the updateTrayIcon() method could inherit them, as it now requires them.

It's also quite the mess in the ipcMain.on('update-icon') event handler, as I had to make sure the icon was refreshed whenever you enable/disable the setting, without losing the notifications count, which is not passed to the function it's not in the scope. (That's why props of updateTrayIcon() are optional)

If you have suggestions to make the implementation easier, please let me know.

I tried implementing some tests, I wanted to use the tray.getTitle() method to add more, but it looks like there's nowhere I could access the tray in the tests...

@adufr adufr force-pushed the feat/show-notifications-count-in-tray branch from 77a5481 to 872f2b7 Compare March 25, 2024 07:57
@adufr
Copy link
Contributor Author

adufr commented Mar 25, 2024

Hello @setchy, sorry for tagging you but this PR has been here for 45 days and didn't receive a single comment 😢

I think this could be a really great addition to Gitify ; please let me know what you think!

@setchy
Copy link
Member

setchy commented Mar 25, 2024

I'll take a look during the week @adufr - we have been quite preoccupied with getting v5 out.

@adufr
Copy link
Contributor Author

adufr commented Mar 25, 2024

I'll take a look during the week @adufr - we have been quite preoccupied with getting v5 out.

Yeah I can imagine, congrats on that! You've done quite some work and the app looks much better now! 😁

@setchy
Copy link
Member

setchy commented Mar 25, 2024

On initial glance, it seems like a lot of files have changes. Is there a simpler path here to access the settings?

@adufr
Copy link
Contributor Author

adufr commented Mar 25, 2024

@setchy I agree, but I didn't find any other way to make it work 😕

@setchy setchy added the enhancement New feature or enhancement to existing functionality label Mar 27, 2024
@adufr
Copy link
Contributor Author

adufr commented Mar 29, 2024

I fixed all conflicts following 5.1.0 release

@setchy
Copy link
Member

setchy commented Mar 30, 2024

Thanks @adufr - will take a look once #941 is merged

@adufr
Copy link
Contributor Author

adufr commented Mar 30, 2024

Do not hesitate to ping me once it's merged, I can probably rewrite the feature

@setchy
Copy link
Member

setchy commented Apr 1, 2024

@adufr - with #941 merged, I took a look at this feature - see #945 (didn't want to disrupt your branch).

Functionally, it seems to match. Appreciate you taking a look. I've noted a few of the to-dos on the PR

@adufr
Copy link
Contributor Author

adufr commented Apr 2, 2024

superseded by #945

@adufr adufr closed this Apr 2, 2024
@adufr adufr deleted the feat/show-notifications-count-in-tray branch July 1, 2024 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or enhancement to existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants