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

Add SetStatusIcon #19

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@tmc

tmc commented Dec 29, 2017

@tmc tmc changed the title from Add SetIconImage to Add SetStatusIcon Dec 29, 2017

@tmc

This comment has been minimized.

Show comment
Hide comment
@tmc

tmc commented Jan 3, 2018

Ping

@dmitshur

Thanks for the PR, and sorry about the delay (I was busy, then sick, etc.).

I've left some minor questions and comments. Overall, this looks very reasonable, and we can merge after addressing these points.

switch m.(type) {
case *image.NRGBA:
invertImageNrgba(m.(*image.NRGBA))
}

This comment has been minimized.

@dmitshur

dmitshur Jan 4, 2018

Member

Why this change? Can the type be something other than *image.NRGBA? If so, what can it be? Is doing nothing in those cases the best thing to do?

@dmitshur

dmitshur Jan 4, 2018

Member

Why this change? Can the type be something other than *image.NRGBA? If so, what can it be? Is doing nothing in those cases the best thing to do?

// TODO: Implement.
void set_status_item_icon(struct image img) {}

This comment has been minimized.

@dmitshur

dmitshur Jan 4, 2018

Member

Remove extra blank line at the end, it's not needed.

@dmitshur

dmitshur Jan 4, 2018

Member

Remove extra blank line at the end, it's not needed.

@@ -56,6 +56,13 @@ func Initialize(title string, imageData []byte, items []MenuItem) {
}
}
// SetStatusIcon replaces the icon image data.
func SetStatusIcon(imageData []byte) {

This comment has been minimized.

@dmitshur

dmitshur Jan 4, 2018

Member

Given we already have trayhost.UpdateMenu to modify the menu items, it might make more sense to use more consistent naming.

How about UpdateIcon?

@dmitshur

dmitshur Jan 4, 2018

Member

Given we already have trayhost.UpdateMenu to modify the menu items, it might make more sense to use more consistent naming.

How about UpdateIcon?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment