On Fri, 10 Jul 2026 11:47:41 GMT, Marius Hanl <[email protected]> wrote:

>> These days (well since Java 9) there is a new facade that seems to be 
>> preferred over JUL logging as it is easier to integrate: 
>> `System.getLogger(loggerName)`
>> 
>> Also, I don't think we really reinvented the wheel here; we just made a 
>> facade for unit testing specifically (as it is hard to hook into JUL logging 
>> I think for this, given it is asynchronous). We could also directly get the 
>> relevant JUL logger here and not go through the facade and the result would 
>> be the same (just can't be unit tested as easily then).
>> 
>> I can surround this with a "is level warning" check, if you think it would 
>> be a performance issue (this log shouldn't show up at all, and if it does, 
>> there may be something seriously broken with your listeners that would have 
>> resulted I think in a `StackOverflowError` in the system that this PR 
>> replaces).
>> 
>> As I think this message should never appear in 99.99% of FX applications, 
>> I'm not worried about the formatting overhead; and if it does appear, the 
>> correct action would be to fix it, not ignore it.
>
> Good question. There also is: 
> [JDK-8200236](https://bugs.openjdk.org/browse/JDK-8200236).
> 
> The advantage: The System.Logger has a `log(Level level, Supplier<String> 
> msgSupplier)` method. The `Supplier` is only 'used' when the log level 
> matches.
> 
> And in general I don't think we should keep `PlatformLogger`, as also noted 
> in the ticket.

I suppose for a warning it is a non issue, we can keep this code as is.

I was just ranting.  Even `System.Logger` seems sub-optimal to me: the 
`Supplier` method is a good idea, but have a level argument instead of 
dedicated methods like `warn()` and `debug()` ?  Bad facade, bad.  `</rant>`

-------------

PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r3559863504

Reply via email to