dustinkredmond / FXTrayIcon

Tray Icon implementation for JavaFX applications. Say goodbye to using AWT's SystemTray icon, instead use a JavaFX Tray Icon.
MIT License
324 stars 25 forks source link

addMenuItem after tray is shown throws exception #71

Closed mrgreywater closed 11 months ago

mrgreywater commented 1 year ago
var tray = new FXTrayIcon(primaryStage, ...);
tray.show();
tray.addExitItem("Exit");
tray.addMenuItem(new MenuItem("Item"));

throws

Exception in thread "AWT-EventQueue-0" java.lang.IllegalArgumentException: index less than zero.
    at java.desktop/java.awt.Menu.insert(Menu.java:323)
    at com.dustinredmond.fxtrayicon@4.0.1/com.dustinredmond.fxtrayicon.FXTrayIcon.addMenuItemPrivately(FXTrayIcon.java:998)
    at com.dustinredmond.fxtrayicon@4.0.1/com.dustinredmond.fxtrayicon.FXTrayIcon.lambda$addMenuItem$9(FXTrayIcon.java:970)
dustinkredmond commented 1 year ago

Should be an easy enough fix, let me check on this one.

EasyG0ing1 commented 1 year ago

@mrgreywater I tried the exact same code that you posted and I don't get the error. Are you using version 4.0.1 of the library? Actually Im not sure if my code is EXACTLY the same since I can't tell which constructor you used as you have it ellipsed out at the end.

mrgreywater commented 1 year ago

@EasyG0ing1 The problem is this line here: https://github.com/dustinkredmond/FXTrayIcon/blob/a3fc68bec00cccb87ffde278d4b866bc630991e3/src/main/java/com/dustinredmond/fxtrayicon/FXTrayIcon.java#L999

if addExitMenuItem is true, the menu is shown but no items in the popupMenu exist, then this.popupMenu.getItemCount() - 1 is negative, when it should be zero.

I edited the example to reflect the addExitItem() call. I'll double check the example tomorrow when I'm back at work.

EasyG0ing1 commented 1 year ago

@mrgreywater

I updated two methods that had the same code in it and pushed it into my existing pull request. The new methods will have this in them now:

if (addExitMenuItem && shown) {
    int index = this.popupMenu.getItemCount() - 1;
    index = (index < 0) ? 0 : index;
    this.popupMenu.insert(AWTUtils.convertFromJavaFX(menuItem), index);
}
EasyG0ing1 commented 1 year ago

@mrgreywater Scratch that, I changed the code to this since this makes much more sense in terms which index to use when adding an item to a list or an array:

if(addExitMenuItem && shown) {
    int index = this.popupMenu.getItemCount();
    this.popupMenu.insert(awtMenu, index);
}
dustinkredmond commented 1 year ago

IDE made the diff a little messy, but the changes themselves look great. Going to merge the pull request that you opened to address both of these. Do we have a test case for this specific issue? I think it may be worth adding if we don't.

EasyG0ing1 commented 1 year ago

@dustinkredmond You just want something added to the test classes that demonstrate the bugs as being fixed?

I can also add a test class that demonstrates how to access the TrayIcon object. There is really a good use case for that with Macs because right-clicking on the icon with a mac doesn't do the same thing as it does in Windows ... Im actually working on a class right now where I can build two different menus and have the code swap them out based on the right or left click with the mouse... could be handy to be able to offer different menu contexts ... left clicking - say for accessing points of interest within the program and right clicking for things like utility functions or app settings or just anything different than simply navigating the main parts of the app.

So I'll get cracking on some new test classes and post another PR when they're done ... but that shouldn't delay deployment since those are just examples for devs... right? ☺

EasyG0ing1 commented 1 year ago

Added and tested the test program for this issue. Works perfectly