-
Notifications
You must be signed in to change notification settings - Fork 277
fix: icon color when idle #2266
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
Conversation
Signed-off-by: Adam Setch <[email protected]>
Signed-off-by: Adam Setch <[email protected]>
|
|
||
onMainEvent(EVENTS.ICON_ERROR, () => { | ||
onMainEvent(EVENTS.UPDATE_ICON_COLOR, (_, notificationsCount: number) => { | ||
if (!mb.tray.isDestroyed()) { | ||
mb.tray.setImage(TrayIcons.error); | ||
} | ||
}); | ||
if (notificationsCount < 0) { | ||
setErrorIcon(); | ||
return; | ||
} | ||
|
||
onMainEvent(EVENTS.ICON_ACTIVE, () => { | ||
if (!mb.tray.isDestroyed() && shouldUseUnreadActiveIcon) { | ||
} | ||
}); | ||
if (notificationsCount > 0) { | ||
setActiveIcon(); | ||
return; | ||
} | ||
|
||
onMainEvent(EVENTS.ICON_IDLE, () => { | ||
if (!mb.tray.isDestroyed()) { | ||
setIdleIcon(); | ||
} | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a naive quick skim of this code; but the logic around setErrorIcon
looks weird. Is that correct?
Might just be the diff making things look more confusing; but currently it sort of looks like the error icon will be used if notificationsCount < 0
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, nevermind. Reading through the rest of it I see that -1
is used for an error state, so this makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking a look.
That's correct, at the moment a negative number is used to flag that the app is in error state.
Some further room for improvement
"webpack-merge": "6.0.1" | ||
}, | ||
"packageManager": "pnpm@10.18.0", | ||
"packageManager": "pnpm@10.17.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was the downgrade intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, timing issue
No description provided.