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

[Feature Request] Restructure Builder class to support extended FXTrayIcon classes. #25

Closed NexRX closed 2 years ago

NexRX commented 2 years ago

I'm trying to extend FXTrayIcon to allow (in addtion) for a custom Menu when righitng clicking.

I can do that fine with some reflection to get the trayIcon (all though would be nice if it was protected :D) However, supporting the builder in my extended even with some nasty reflection just isn't do-able i think because it always returns a FXTrayIcon constructed as such instead of my custom class. I'd have to copy your wonderful code and that just doesn't feel right.

I tried settings it with reflection in the builder but its final so no bueno.

Would it be possible to Generify builder? or make the trayIcon field (in the builder) protected and not final?

Love the work btw! <3 🥇

EasyG0ing1 commented 2 years ago

@Nex-Coder

I don't see why you couldn't just build your custom class like this, then add your methods as needed... I certainly don't see anything unethical about doing it this way... after all, it is open source licensing and I'm sure Dustin wouldn't have any issues at all with it.

But doing it like this basically mirrors the constructors giving you the same interface only with an extended class.

import com.dustinredmond.fxtrayicon.FXTrayIcon;
import javafx.event.ActionEvent;
import javafx.event.EventHandler;
import javafx.stage.Stage;
import java.awt.*;
import java.net.URL;

public class MyTrayIcon extends FXTrayIcon {

    public MyTrayIcon(Stage parentStage, URL iconImagePath, int iconWidth, int iconHeight) {
        super(parentStage, iconImagePath, iconWidth, iconHeight);
    }

    public MyTrayIcon(Stage parentStage, URL iconImagePath) {
        super(parentStage, iconImagePath);
    }

    public MyTrayIcon(Stage parentStage, Image icon) {
        super(parentStage, icon);
    }

    public static class Builder {
        private final MyTrayIcon trayIcon;
        private       boolean    showMenu = false;
        private final String fileName1 = "FXIconRedWhite.png";
        URL icon1 = getClass().getResource(fileName1);
        public Builder(Stage parentStage, URL iconImagePath, int iconWidth, int iconHeight) {
            trayIcon = new MyTrayIcon(parentStage, iconImagePath, iconWidth, iconHeight);
        }
        public Builder(Stage parentStage, URL iconImagePath) {
            if (System.getProperties().getProperty("os.name").contains("Mac")) {
                trayIcon = new MyTrayIcon(parentStage, iconImagePath, 26, 26);
            }
            else {
                trayIcon = new MyTrayIcon(parentStage, iconImagePath);
            }
        }
        public Builder(Stage parentStage) {
            trayIcon = new MyTrayIcon(parentStage, icon1, 26, 26);
        }
        public Builder menuItem(String label, EventHandler<ActionEvent> eventHandler) {
            javafx.scene.control.MenuItem menuItem = new javafx.scene.control.MenuItem(label);
            menuItem.setOnAction(eventHandler);
            trayIcon.addMenuItem(menuItem);
            return this;
        }

        public Builder menuItem(javafx.scene.control.MenuItem menuItem) {
            trayIcon.addMenuItem(menuItem);
            return this;
        }

        public Builder menuItems(javafx.scene.control.MenuItem ... menuItems) {
            for (javafx.scene.control.MenuItem menuItem : menuItems) {
                trayIcon.addMenuItem(menuItem);
            }
            return this;
        }

        public Builder separator() {
            trayIcon.addSeparator();
            return this;
        }

        public Builder toolTip(String tooltip) {
            trayIcon.setTooltip(tooltip);
            return this;
        }

        public Builder withTitle() {
            trayIcon.addTitleItem(true);
            return this;
        }

        public Builder applicationTitle(String applicationTitle) {
            trayIcon.setApplicationTitle(applicationTitle);
            trayIcon.addTitleItem(true);
            return this;
        }

        public Builder addExitItem() {
            trayIcon.addExitItem(true);
            return this;
        }

        public Builder onAction(EventHandler<ActionEvent> e) {
            trayIcon.setOnAction(e);
            return this;
        }

        public Builder addTitleItem(boolean addTitleMenuItem) {
            trayIcon.addTitleItem(addTitleMenuItem);
            return this;
        }
        public Builder show() {
            this.showMenu = true;
            return this;
        }
        public MyTrayIcon build() {
            if (showMenu) trayIcon.show();
            return trayIcon;
        }
    }
}
EasyG0ing1 commented 2 years ago

@Nex-Coder - I also don't see any issues with making trayIcon inside the Builder class protected instead of final. I can't think of any problems it would introduce, but I'll wait for Dustin's comments on that and if he agrees, I'll submit a pull request.

NexRX commented 2 years ago

Copying is an option if the creator is fine. I wouldn't mind doing that so thanks! :)

But! If we could have a protected (non-final) trayIcon in the builder, that would let us define our own Custom FXTrayIcon instance. and assign it in the constructor!

If we could get that, that would be great <3

EasyG0ing1 commented 2 years ago

@Nex-Coder - I actually wrote the Builder class and I'm fine with it. ;-) But it wouldn't hurt to get Dustin's blessing as well - to be thorough.

dustinkredmond commented 2 years ago

@EasyG0ing1 & @Nex-Coder

You have my blessing. 👍🏻

I have no issue with making the instance protected instead of private.

I'm open to review pull requests. Thank you both!

NexRX commented 2 years ago

Lovely, i've made a pull request @dustinkredmond for the change. Thanks guys :)

NexRX commented 2 years ago

pull request: https://github.com/dustinkredmond/FXTrayIcon/pull/26

NexRX commented 2 years ago

Pull merged so I'll close this, thanks guys! :)

EasyG0ing1 commented 2 years ago

@Nex-Coder

Hi,

I've been trying to figure out how you are utilizing the Builders trayIcon object through an extended class ... and I think I need an example. Would you be willing to post the class you wrote?

Mike

NexRX commented 2 years ago

@EasyG0ing1 I haven't written any code yet because there wasn't a way decent way (other than copying the source code for builder) to change what trayIcon instance is (and thus what FXTrayIcon is built).

This mean when i extend your guys class FXTrayIcon to add some custom features like an alternative right click menu based on something other than menu items (i.e. panes and other controls), I can't just extend Builder and replace trayIcon (from the constructor) with my extended version of FXTrayIcon. But now I can becaused its protected and I'm able to change what the (extend) instance of FXTrayIcon is built :)

NexRX commented 2 years ago

In short, after calling super() in the extended builders constructor, I can basically call trayIcon = new ExtendedFXTrayIcon(...)

and overwrite build() return with ExtendTrayIcon.

EasyG0ing1 commented 2 years ago

@Nex-Coder I'm curious to see that in action (or in written code) and the reason I ask, is because as I've been learning about Builder style classes, I've realized that I didn't quite design this builder class properly.

The correct way to do a Builder class is to have the class simply accept the different options from the user, then the Builder calls a private constructor of the parent class, passing itself into the constructor as the only argument. From there, the private parent constructor extracts the values that were given to the builder and proceeds to instantiate itself based on the options provided to Builder.

In the case of FXTrayIcon, this would mean that the Builder class would never even have a trayIcon object at all and the instantiation of that object would remain with the parent class (FXTrayIcon), which is the proper way to keep things simple and concise and to make sure that the subclass (Builder) is not intruding on the main function of the parent class.

What I'm concerned about, is if I re-structure Builder so that it conforms to a properly defined builder style ... you'll not have access to the trayIcon object except through the main class since Builder won't actually be having any instances of objects that do any work... it will only have objects that set the parameters for the main class.

This is why I wanted to see the code ... so that if I understood what you were doing, perhaps there would be a way to continue doing it even with Builder structured properly.

Basically, the main difference in the Builder class would end up looking something like this (this is a very shortened version of it, but it illustrates the point well enough and shows you that the trayIcon object wouldn't exist in Builder and you can see why):

    public static class Builder {
        private final Stage parentStage;
        private final URL iconImagePath;
        private final int iconWidth;
        private final int iconHeight;

        public Builder(Stage parentStage, URL iconImagePath, int iconWidth, int iconHeight) {
            this.parentStage = parentStage;
            this.iconImagePath = iconImagePath;
            this.iconWidth = iconWidth;
            this.iconHeight = iconHeight;
        }

        public FXTrayIcon build() {
            icon = loadImageFromFile(defaultIconPath,iconWidth,iconHeight);
            return new FXTrayIcon(this);
        }
    }

    private FXTrayIcon(Builder build) {
        this(build.parentStage, build.icon);
    }

Obviously, the full Builder class will still be convenient because it will still take all of the options that you can pass into it now, the difference is, it will be the private FXTrayIcon constructor that will ultimately put all of that together.

EasyG0ing1 commented 2 years ago

@Nex-Coder

Here is a zip file that has a compiled version of what the Builder class should be along with the .java file that I compiled into the .jar file.

If you're using a POM file to add FXTrayIcon to your project, then just comment out that dependency and then add a library manually using this jar file and you can experiment with it and see how it will work for your extended class.

Mike FXTrayIconNewBuilder-Fixed.zip

NexRX commented 2 years ago

@EasyG0ing1 Thanks for the info. I'm busy at work right now. But once i get some time later (today) I'll load it up and give some feedback. I do agree with your views on the Builder classes better way of styling. I was already aware of some styling method for builders but everyone has their own view on what is a better way to right X code.

When it comes down to it, It is up to you guys to pick the implementation that suites your work :) But its good to see some restructuring like this for the community <3

EasyG0ing1 commented 2 years ago

@Nex-Coder

Well, my thinking is that since the current Builder class didn't actually return the proper FXTrayIcon class in the final build() statement ... now that it will, it might actually make your use of an extended class easier since there will not be two different instances of trayIcon ... only the one instance from the parent class as it should be.

Anyways, let me know what you find when you get the time to check it out.

Have fun working ☺

Mike

By the way, I'm replacing that zip file, there was an error in the class when trying to use the built-in icon graphic but It's fixed now.

NexRX commented 2 years ago

Hey, Sorry for getting back so late. Had a pretty spooky halloween ;) But ive now had the chance to have a look at the new implemtnation and it sadly doesn't allow construction of the extended FXTrayIcon classes.

Some reasons why:

The ways i see overcoming this is either providing (protected?) getters for the fields so we can construct our own instances of FXTrayIcon when overriding build(). This could also allows us the implement our own constructor that takes a builder.

or on the other hand

the previous protected trayIcon also would work but i think we could both agree that the previous solution works better overall.

EasyG0ing1 commented 2 years ago

@Nex-Coder

This is from the book, Effective Java, Best practices for the Java Platform. Written by Joshua Bloch (pages 12 - 13):

Instead of making the desired object directly, the client calls a constructor (or static factory) with all of the required parameters and gets a builder object. Then the client calls setter-like methods on the builder object to set each optional parameter of interest. Finally, the client calls a parameterless build method to generate the object, which is typically immutable. The builder is typically a static member class (Item 24) of the class it builds. Here’s how it looks in practice:

public class NutritionFacts {
    private final int servingSize;
    private final int servings;
    private final int calories;
    private final int fat;
    private final int sodium;
    private final int carbohydrate;

    public static class Builder {
        // Required parameters
        private final int servingSize;
        private final int servings;

        // Optional  parameters -  initialized to default values
        private  int calories       =  0;
        private  int fat            =  0;
        private  int sodium         =  0;
        private  int carbohydrate   =  0;

        public Builder(int servingSize, int servings) {
            this.servingSize = servingSize;
            this.servings     = servings;
        }

        public Builder calories(int val) { 
            calories = val;
            return this; 
        }
        public Builder fat(int val) {
            fat = val;
            return this; 
        }
        public Builder sodium(int val) {
            sodium = val;
            return this;
        }
        public Builder carbohydrate(int val) {
            carbohydrate = val;
            return this;
        }
        public NutritionFacts build() {
            return new NutritionFacts(this);
        }   
    }

    private NutritionFacts(Builder builder) {
        servingSize   = builder.servingSize;
        servings      = builder.servings;
        calories      = builder.calories;
        fat           = builder.fat;
        sodium        = builder.sodium;
        carbohydrate  = builder.carbohydrate;
    }
}

Have you written any example code yet? i'd like to see what you're trying to do with at least some pseudocode of some type...

NexRX commented 2 years ago

Yeah, I did try some examples. I'll try again when I get some time later and share the examples, working again. :D

But i kept getting errors like "Cannot cast FXTrayIcon to NexTrayIcon" etc. Both extending and using the base builder. :)

NexRX commented 2 years ago

Here we are @EasyG0ing1! So, for example. If i try to use this extended builder (in my extend class of FXTrayIcon)...

    public static class Builder extends FXTrayIcon.Builder {

        public Builder(Stage parentStage, URL iconImagePath, int iconWidth, int iconHeight) {
            super(parentStage, iconImagePath, iconWidth, iconHeight);
        }

        public Builder(Stage parentStage, URL iconImagePath) {
            super(parentStage, iconImagePath);
        }

        public Builder(Stage parentStage) {
            super(parentStage);
        }

        @Override public NexTrayIcon.Builder menuItem(String label, EventHandler<ActionEvent> eventHandler) { return (Builder) super.menuItem(label, eventHandler); }
        @Override public NexTrayIcon.Builder menuItem(javafx.scene.control.MenuItem menuItem) { return (Builder) super.menuItem(menuItem); }
        @Override public NexTrayIcon.Builder menuItems(javafx.scene.control.MenuItem... menuItems) { return (Builder) super.menuItems(menuItems); }
        @Override public NexTrayIcon.Builder separator() { return (Builder) super.separator(); }
        @Override public NexTrayIcon.Builder toolTip(String tooltip) { return (Builder) super.toolTip(tooltip); }
        @Override public NexTrayIcon.Builder addTitleItem(boolean addTitleMenuItem) { return (Builder) super.addTitleItem(addTitleMenuItem); }
        @Override public NexTrayIcon.Builder applicationTitle(String appTitle) { return (Builder) super.applicationTitle(appTitle); }
        @Override public NexTrayIcon.Builder addExitMenuItem() { return (Builder) super.addExitMenuItem(); }
        @Override public NexTrayIcon.Builder onAction(EventHandler<ActionEvent> event) { return (Builder) super.onAction(event); }
        @Override public NexTrayIcon.Builder show() { return (Builder) super.show(); }

        @Override
        public NexTrayIcon build() {
            this.isMac = System.getProperty("os.name").toLowerCase(Locale.ENGLISH).contains("mac");
            if (this.iconWidth == 0 || this.iconHeight == 0) {
                if (this.isMac) {
                    this.iconWidth = 26;
                    this.iconHeight = 26;
                } else {
                    this.iconWidth = 16;
                    this.iconHeight = 16;
                }
            }

            if (this.useDefaultIcon) {
                this.icon = FXTrayIcon.loadImageFromFile(this.defaultIconPath, this.iconWidth, this.iconHeight);
            } else {
                this.icon = FXTrayIcon.loadImageFromFile(this.iconImagePath, this.iconWidth, this.iconHeight);
            }

            return new NexTrayIcon(this);
        }
    }

and construct like so

   NexTrayIcon icon = new NexTrayIcon.Builder(stage, url).build();

It will not work because the fields inside the super class builder are all private with no way to access them and thus no way of passing the fields to the private constructor in my extended FXTrayIcon called NexTrayIcon. :)

Admittedly, the concept of private constructors is new to me so maybe im goofing up somewhere. Nether the less I get

error: isMac has private access in Builder this.isMac = System.getProperty("os.name").toLowerCase(Locale.ENGLISH).contains("mac");

on compile.

Obviously, whenever I've implemented my custom features for FXTrayIcon, i could add any related properties to my extended Builder for the constructor, but I just want to show the basics of course.

NexRX commented 2 years ago

Okay, so this has been a bit of a learning experience for me. And I now know that all i really needed to do in my builder was this...

        @Override
        public NexTrayIcon build() {
            return new NexTrayIcon(this);
        }

and my private constructor can then look like this

    private NexTrayIcon(Builder builder) {
        super(builder);
        this.customField = builder.customField; // my custom fields assigned here
    }

but this still leave one last problem. The constructor in super is still private :D

I think the only change that needs to be done in your new implementation to support extended builders is a going from a private consturctor of FXTrayIcon to a protected one.

When private, it ensures derivaed classes cannot construct from it. But with protected classes, derivaed classes are support and no encapsulation is broken :)

Problem solved unless you have any problems

dustinkredmond commented 2 years ago

I'm not averse to making the constructor protected. I will try to review this today. Thanks!

NexRX commented 2 years ago

Great @dustinkredmond!

I've setup a fork with those two changes (newbuilder & protected constrctor).

Also. I added a protected getter for the AWT trayIcon. This would just be a nice bonus as corrently I'm using reflection to get and use it for custom things. I plan add stuff like animated trayIcon, custom right clcik menu, etc. So it would be really great if we could get this too! Reflection works but its less than idea haha.

I've made a fork here if you like the sound of all that :) i can open a pull request :)

EasyG0ing1 commented 2 years ago

@dustinkredmond - I looked at his fork, and I made trayIcon protected as well as the private constructor for the Builder class in my existing pull request. It looks like those were the only two changes he made.

@Nex-Coder you can see the branch where I implemented your changes here, take a look and see if it's right.

NexRX commented 2 years ago

@EasyG0ing1 Yeah that is all I wished for thanks! Only difference being i made a protected final getter for trayIcon. But its exactly the same result. That satisfies everything an extended implementation needs on my end.

EasyG0ing1 commented 2 years ago

@Nex-Coder I added the getter to the branch as well. It is locked and loaded in the current pull request.

EasyG0ing1 commented 2 years ago

@Nex-Coder - Here is an artifact that includes all of the changes if you want to test it.

FXTrayIcon.zip

NexRX commented 2 years ago

I can confirm the new changes permit extended classes!!! Tested some building and changes in super() calls are also visable in the current this().

Feel free to close this thread once your done tracking/pushed to mavern. <3

EasyG0ing1 commented 2 years ago

The ball is in @dustinkredmond 's court right now. Everything is done, just needs to be merged and (hopefully) published.

EasyG0ing1 commented 2 years ago

@Nex-Coder You mentioned something about animated icons in the tray ... I'd like to see that in action...

:-)

NexRX commented 2 years ago

Sure. I can work this stuff in a fork if you guys like and contribute the features you guys want :) Not exactly sure how far I will take it, but I know I'd like some custom right click parents/controls and semi animated icons. Like Discords microphone activatity TrayIcon.

EasyG0ing1 commented 2 years ago

@Nex-Coder Well I know that FXTrayIcon is a native AWT control because there STILL does not exist, a native JavaFX System Tray Icon object, in spite of it being officially requested and receiving enough votes for its implementation into JavaFX back in ... 2015 I believe. It even had a developer assigned to it ... but it seems that person had better things to do.

Anyways, The reason I mention the AWT fact of our reality is that where Macs are concerned, right-clicking is an exercise in futility. HOWEVER, we can dual click on it and get some action from it that way ... Windows is another story, you can right click on it all day in Windows ... if fact, I don't think there is any difference between a right-click in Windows vs a left-click ... which is fun ...

If you start developing something that begins to have a shape to it, let me know... perhaps you could just post it on your Github page and I could clone it from there and have a look.

My email, by the way, is sims.mike@gmail.com