search

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

Builder API inconsistent #42

Closed dwalluck closed 2 years ago

dwalluck commented 2 years ago

Why is URL iconImagePath required to be passed into the builder constructor and not via a method? If the idea is to put required arguments in the constructor, then this doesn't make sense because there's a setGraphic() method, but no corresponding graphic() method for the builder. I see that it is probably to match the existing non-builder constructors, but I still think there should be a graphic() method and possibly others for the size. In any case, it seems a good idea to have the builders constructors and methods match the original, if possible.

The API is setTooltip(), but toolTip() in the builder, whereas I think it should be tooltip(). It seems to be an inconsistency in AWT vs. JavaFX naming. I believe the AWT method uses a capital 'T', but JavaFX uses a lowercase 't' by convention.

There's a setTrayIconTooltip() API which does the same thing, but the builder doesn't have such a method.

dustinkredmond commented 2 years ago

Hi @dwalluck , the Builder API is still relatively new, we'll check on standardizing this to match the JavaFX convention.

dustinkredmond commented 2 years ago

@EasyG0ing1, would you have time to review this? Since it changes public API, it's not an immediate necessity, but would be a "nice-to-have" for the next major release.

EasyG0ing1 commented 2 years ago

@dustinkredmond - I just saw this, I will absolutely look into this and submit a PR accordingly.

EasyG0ing1 commented 2 years ago

@dwalluck - I just now caught the mention of the tooltip thing in the builder ... I did submit a PR for it though it seems a little nit-picky at this point ... I also wanted to point out that this is the published method in the officialjava.awt TrayIcon class...

Screen Shot 2022-05-11 at 7 14 34 AM

... just sayin' ... ☺

EasyG0ing1 commented 2 years ago

@dwalluck - Also, check out PR #47

dustinkredmond commented 2 years ago

Going with @EasyG0ing1's PR and closing this. Feel free to re-open if any further discussion.

dwalluck commented 2 years ago

@dwalluck - I just now caught the mention of the tooltip thing in the builder ... I did submit a PR for it though it seems a little nit-picky at this point ... I also wanted to point out that this is the published method in the officialjava.awt TrayIcon class... Screen Shot 2022-05-11 at 7 14 34 AM

... just sayin' ... ☺

Yes, I know.

One of my original "complaints" was that the AWT methods have different naming conventions from JavaFX, and it's a JavaFX library. To me, it makes sense to align with JavaFX naming conventions, not AWT.

On top of that, the builder methods didn't match the get/set methods.

But, yes, as it deals with naming you could call it "nit-picky".

EasyG0ing1 commented 2 years ago

@dwalluck - I mean on one side of the fence ... it's nit-picking ... but on the other side of the fence, when you're reading code and trying to make sense of it, the last thing you need, are brain obstacles on the screen that distract from the main goal - understanding the code!

I actually have been ACTIVELY forcing myself to use the word filename and not fileName ... because technically, it's one word. But even after months of catching myself and correcting myself, I still find that I sometimes use fileName without even thinking ... it's the little things that matter sometimes. ☺