-
Notifications
You must be signed in to change notification settings - Fork 4
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 listener to TrayMenuController #11
Conversation
Add listener to support refreshing of popup menu on events that aren't internally generated (i.e. mouse events)
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.
Could you add some JavaDoc (just a line or two, explaining what implementers are expected to do with the given listener).
Edit: Also I just noticed onBeforeShow
is a terrible name, as it suggests this has something to do with showTrayIcon(...)
. I see two options:
- Either set up the listener during
showTrayIcon(...)
(add a new nullable param) - Or rename this method to
onBeforeOpenMenu(...)
I've committed changes for option 2). |
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.
Looks good, however can you please undo the xml file, as it is unrelated?
@infeo While I'm fine with this, we should at least discuss whether it is a more consistent API design to set all listeners during showTrayIcon
. Or whether this mixed approach is acceptable? See listener A vs listener B. We could also opt for setting all listeners in separate methods and remove defaultAction
from showTrayIcon()
. I leave this up to you, since you will have to live with it for the foreseeable future 😉
done! |
Add listener to support cryptomator/cryptomator#2489