-
Notifications
You must be signed in to change notification settings - Fork 19
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
Update Menu programatically #13
Conversation
File I'm testing with: https://gist.github.com/captncraig/9756f746dd882cac3d64ac39f56f92ff |
Works on windows. Now. At a loss where to start with the gtk version. Maybe someone else can take over? |
Might be as simple as Possibly also |
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 the PR. This looks good. See minor review comments.
@@ -184,3 +184,12 @@ func (n Notification) Display() { | |||
|
|||
C.display_notification(notificationId, cTitle, cBody, img, C.double(n.Timeout.Seconds())) | |||
} | |||
|
|||
//UpdateMenu will remove all menu items and add the new menu items |
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.
There should be a space after the //
. See https://dmitri.shuralyov.com/idiomatic-go#comments-for-humans-always-have-a-single-space-after-the-slashes for rationale.
//UpdateMenu will remove all menu items and add the new menu items | ||
func UpdateMenu(newMenu []MenuItem) { | ||
menuItems = newMenu | ||
C.clear_menu_items() |
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.
I would swap the two lines above. I think either way, the behavior is the same, but logically, it makes sense to clear items first (C.clear_menu_items()
), and only then add new items:
C.clear_menu_items()
menuItems = newMenu
for id, item := range newMenu {
...
It's fine not to implement OSes that you don't normally use and don't have an easy way to test with. Someone else who uses them can do that work. Just leave a I only support macOS side myself. |
Addressed those comments. |
Matches the same comments above and below.
Make it clear this functionality isn't implemented here.
For consistency with the rest of the documentation here.
This reads better in context of all other comments.
Thanks! I've followed up with some minor consistency, style and wording improvements. LGTM. |
Added osx code to clear menu. Still needs windows/linux